AX: setValue on contenteditable should preserve whitespace

• Component: Accessibility
• Status: RESOLVED FIXED

[Nan Wang]

Currently accessibility setValue on contenteditable elements would collapse whitespaces.

<rdar://problem/36563892>

[Nan Wang]

Created attachment 341053 [details]
patch

[Nan Wang]

Created attachment 341054 [details]
patch

remove redundant header

[chris fleizach]

Comment on attachment 341054 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1787
> +            style.setWhiteSpace(PRE);

We don’t have to set this before we set inner text?

[Nan Wang]

(In reply to chris fleizach from comment #3)
> Comment on attachment 341054 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341054&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1787
> > +            style.setWhiteSpace(PRE);
> 
> We don’t have to set this before we set inner text?

I think the render tree update happens afterwards. But I can move this before that

[Nan Wang]

Created attachment 341062 [details]
patch

update

[EWS Watchlist]

Comment on attachment 341062 [details]
patch

Attachment 341062 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7772740

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 341068 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Nan Wang]

The test failure shouldn't be related

[chris fleizach]

Comment on attachment 341062 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1783
> +            RenderElement& renderParent = downcast<RenderElement>(renderer);

This is not really the renderParent right? It’s more like renderElement?

[Nan Wang]

(In reply to chris fleizach from comment #9)
> Comment on attachment 341062 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341062&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1783
> > +            RenderElement& renderParent = downcast<RenderElement>(renderer);
> 
> This is not really the renderParent right? It’s more like renderElement?

It’s the parent of the text node. This can be misleading, I’ll update before committing

[Nan Wang]

Committed r232120: <https://trac.webkit.org/changeset/232120>

[Darin Adler]

Comment on attachment 341062 [details]
patch

This change does not seem correct. Mutating RenderStyle doesn’t make a permanent change to the document. The style can change if something causes it to be invalidated and recomputed. So this might make things look right temporarily but it’s a fragile illusion. I don’t fully understand the goal here, but if the goal is to add non-collapsible spaces that will last then we need to mutate the DOM, affecting the style indirectly, not mutate style.

[Darin Adler]

To give good advice about real cases we need to understand what else is done with the text besides display it. So, for example, if we need to make sure the text is uploaded properly to a server, different techniques might be required.

Generally it’s not safe to just set the innerText of an arbitrary HTML element--could have all sorts of different effects on different webpages and might simply be incompatible with what the code on those pages expect--but I must assume that users of screen readers are getting value out of what the screen reader is doing here. But since I don’t know what the value is, it’s hard to form an opinion about what this code should be doing.

[Nan Wang]

(In reply to Darin Adler from comment #13)
> To give good advice about real cases we need to understand what else is done
> with the text besides display it. So, for example, if we need to make sure
> the text is uploaded properly to a server, different techniques might be
> required.
> 
> Generally it’s not safe to just set the innerText of an arbitrary HTML
> element--could have all sorts of different effects on different webpages and
> might simply be incompatible with what the code on those pages expect--but I
> must assume that users of screen readers are getting value out of what the
> screen reader is doing here. But since I don’t know what the value is, it’s
> hard to form an opinion about what this code should be doing.

The goal here is when users using braille input to type in values to a contenteditable we want to set the value to the corresponding element. The legacy behavior of setting innerText is problematic in some way and we plan to redesign the structure in a later timeframe.

This bug here is when the input contains some trailing whitespace, they would be ignored. We know in most cases the style won't change during editing but I can see your point. Do you have any suggestions on how to mutate the DOM in such case?

[Darin Adler]

If we’re trying to do the equivalent of typing, then we should be using code from the editing machinery. Besides handling spaces correctly, that will make things like undo work.

For example, when someone uses QuickType or an input method to type a sequence of characters all at once, that’s not done by directly mutating the DOM.

I only have a moment to write this comment, so I can’t point at exactly the right code to use but most of the relevant code is in the editing directory. Looking at execCommand can also be helpful.

[Nan Wang]

(In reply to Darin Adler from comment #15)
> If we’re trying to do the equivalent of typing, then we should be using code
> from the editing machinery. Besides handling spaces correctly, that will
> make things like undo work.
> 
> For example, when someone uses QuickType or an input method to type a
> sequence of characters all at once, that’s not done by directly mutating the
> DOM.
> 
> I only have a moment to write this comment, so I can’t point at exactly the
> right code to use but most of the relevant code is in the editing directory.
> Looking at execCommand can also be helpful.

Thanks for the suggestion. I'll look into that.

[Nan Wang]

Created attachment 341369 [details]
patch

Reopen and try to fix this the right way.

[Nan Wang]

Reopen this bug

[EWS Watchlist]

Comment on attachment 341369 [details]
patch

Attachment 341369 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7807356

New failing tests:
accessibility/mac/set-value-editable-types.html
accessibility/mac/AOM-event-accessiblesetvalue.html

[EWS Watchlist]

Created attachment 341376 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Nan Wang]

will work on the test

[Nan Wang]

Created attachment 341377 [details]
patch

Updated test expectation for Mac wk1

[chris fleizach]

Comment on attachment 341377 [details]
patch

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

> LayoutTests/accessibility/mac/set-value-editable-types-expected.txt:13
>  PASS successfullyParsed is true

Where are theses As coming from?

[Darin Adler]

Comment on attachment 341377 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1789
> +            Editor& editor = frame->editor();
> +            if (element.shouldUseInputMethod()) {
> +                editor.clearText();
> +                TypingCommand::insertText(node()->document(), string, 0);
> +            }

I think this should probably use one of the Editor::insertText functions instead.

>> LayoutTests/accessibility/mac/set-value-editable-types-expected.txt:13
>>  PASS successfullyParsed is true
> 
> Where are theses As coming from?

I think taht’s what a non-breaking space looks like when the encoding is misinterpreted.

[Nan Wang]

Comment on attachment 341377 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1789
>> +            }
> 
> I think this should probably use one of the Editor::insertText functions instead.

Ok

>>> LayoutTests/accessibility/mac/set-value-editable-types-expected.txt:13
>>>  PASS successfullyParsed is true
>> 
>> Where are theses As coming from?
> 
> I think taht’s what a non-breaking space looks like when the encoding is misinterpreted.

Yes they are the non breaking spaces

[Nan Wang]

Created attachment 341385 [details]
patch

update from review

[Darin Adler]

Comment on attachment 341385 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1787
> +                editor.clearText();
> +                editor.insertText(string, nullptr);

No need for the call to clearText; insertText will replace the selected text.

[Nan Wang]

(In reply to Darin Adler from comment #27)
> Comment on attachment 341385 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341385&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1787
> > +                editor.clearText();
> > +                editor.insertText(string, nullptr);
> 
> No need for the call to clearText; insertText will replace the selected text.

When I removed the clearText() call, the layout test result would contain the previous value

[Nan Wang]

Committed r232259: <https://trac.webkit.org/changeset/232259>

[Darin Adler]

Comment on attachment 341385 [details]
patch

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

>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1787
>>> +                editor.insertText(string, nullptr);
>> 
>> No need for the call to clearText; insertText will replace the selected text.
> 
> When I removed the clearText() call, the layout test result would contain the previous value

Thanks for trying it. At some point I would like to research why that is behaving that way, but for now I’m satisfied by the fact that you checked.