[Webkit-unassigned] [Bug 153361] AX: new lines in content editable elements don't notify accessibility

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=153361

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #269595|review?                     |review-
              Flags|                            |

--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 269595
  --> https://bugs.webkit.org/attachment.cgi?id=269595
patch.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269595&action=review

> 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);
> +}

Please don't add new code to create a legacy visible position.  Use firstPositionInOrBeforeNode instead.

> Source/WebCore/editing/EditCommand.cpp:242
> +    String text = node->nodeValue();

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.

> 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()) {

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.

> Source/WebCore/html/HTMLDivElement.h:44
> +
> +    bool m_isParagraphSeparator = false;

We certainly don't want to grow every div element by eight bytes.
r- because this will regress WebCore's memory usage.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160205/63790580/attachment.html>