<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: new lines in content editable elements don't notify accessibility"
href="https://bugs.webkit.org/show_bug.cgi?id=153361">bug 153361</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #269595 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: new lines in content editable elements don't notify accessibility"
href="https://bugs.webkit.org/show_bug.cgi?id=153361#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: new lines in content editable elements don't notify accessibility"
href="https://bugs.webkit.org/show_bug.cgi?id=153361">bug 153361</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=269595&action=diff" name="attach_269595" title="patch.patch">attachment 269595</a> <a href="attachment.cgi?id=269595&action=edit" title="patch.patch">[details]</a></span>
patch.patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=269595&action=review">https://bugs.webkit.org/attachment.cgi?id=269595&action=review</a>
<span class="quote">> Source/WebCore/editing/EditCommand.cpp:235
> +static VisiblePosition accessibilityVisiblePositionForNode(Node* node)
> +{
> + if (!node)
> + return VisiblePosition();
> + return is<Text>(node) ? Position(downcast<Text>(node), 0) : createLegacyEditingPosition(node, 0);
> +}</span >
Please don't add new code to create a legacy visible position. Use firstPositionInOrBeforeNode instead.
<span class="quote">> Source/WebCore/editing/EditCommand.cpp:242
> + String text = node->nodeValue();</span >
I don't see why nodeValue could ever be the correct value to use here.
Should this be the visible text?
Then we should be using TextIterator to get the text because nodeValue contains uncollapsed whitespace.
If we're making that change, then we can't synchronously fire these events as TextIterator requires up-to-date layout
and forcing layout upon each DOM mutation will be too expensive.
<span class="quote">> Source/WebCore/editing/EditCommand.cpp:250
> + // Don't consider linebreaks
> + if (text == AccessibilityLineBreakValue)
> + return "";
> +
> + // We do want line breaks for BR and P tags
> + if (!text.length()) {</span >
This logic doesn't make any sense. Whether \n matters or not depends on the computed style of the surrounding text.
\n could be a user visible line break with white-space: pre.
P tag could be turned into a inline element in which case it doesn't denote a line break all.
r- because of all those issues.
<span class="quote">> Source/WebCore/html/HTMLDivElement.h:44
> +
> + bool m_isParagraphSeparator = false;</span >
We certainly don't want to grow every div element by eight bytes.
r- because this will regress WebCore's memory usage.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>