Bug 185897 - AX: setValue on contenteditable should preserve whitespace
Summary: AX: setValue on contenteditable should preserve whitespace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-22 18:17 PDT by Nan Wang
Modified: 2018-05-29 21:43 PDT (History)
13 users (show)

See Also:


Attachments
patch (4.22 KB, patch)
2018-05-22 18:25 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (4.00 KB, patch)
2018-05-22 18:26 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (4.04 KB, patch)
2018-05-22 19:46 PDT, Nan Wang
cfleizach: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.73 MB, application/zip)
2018-05-22 21:31 PDT, Build Bot
no flags Details
patch (6.17 KB, patch)
2018-05-25 18:03 PDT, Nan Wang
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (3.25 MB, application/zip)
2018-05-25 19:23 PDT, Build Bot
no flags Details
patch (6.92 KB, patch)
2018-05-25 19:45 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (6.68 KB, patch)
2018-05-25 21:21 PDT, Nan Wang
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2018-05-22 18:17:53 PDT
Currently accessibility setValue on contenteditable elements would collapse whitespaces.

<rdar://problem/36563892>
Comment 1 Nan Wang 2018-05-22 18:25:21 PDT
Created attachment 341053 [details]
patch
Comment 2 Nan Wang 2018-05-22 18:26:21 PDT
Created attachment 341054 [details]
patch

remove redundant header
Comment 3 chris fleizach 2018-05-22 19:27:22 PDT
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?
Comment 4 Nan Wang 2018-05-22 19:31:23 PDT
(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
Comment 5 Nan Wang 2018-05-22 19:46:00 PDT
Created attachment 341062 [details]
patch

update
Comment 6 Build Bot 2018-05-22 21:31:02 PDT
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
Comment 7 Build Bot 2018-05-22 21:31:13 PDT
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
Comment 8 Nan Wang 2018-05-23 00:26:30 PDT
The test failure shouldn't be related
Comment 9 chris fleizach 2018-05-23 08:09:14 PDT
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?
Comment 10 Nan Wang 2018-05-23 08:17:51 PDT
(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
Comment 11 Nan Wang 2018-05-23 11:38:43 PDT
Committed r232120: <https://trac.webkit.org/changeset/232120>
Comment 12 Darin Adler 2018-05-24 09:47:27 PDT
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.
Comment 13 Darin Adler 2018-05-24 09:50:20 PDT
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.
Comment 14 Nan Wang 2018-05-24 09:59:26 PDT
(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?
Comment 15 Darin Adler 2018-05-25 09:56:50 PDT
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.
Comment 16 Nan Wang 2018-05-25 10:55:00 PDT
(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.
Comment 17 Nan Wang 2018-05-25 18:03:50 PDT
Created attachment 341369 [details]
patch

Reopen and try to fix this the right way.
Comment 18 Nan Wang 2018-05-25 18:04:31 PDT
Reopen this bug
Comment 19 Build Bot 2018-05-25 19:23:08 PDT
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
Comment 20 Build Bot 2018-05-25 19:23:09 PDT
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
Comment 21 Nan Wang 2018-05-25 19:31:31 PDT
will work on the test
Comment 22 Nan Wang 2018-05-25 19:45:42 PDT
Created attachment 341377 [details]
patch

Updated test expectation for Mac wk1
Comment 23 chris fleizach 2018-05-25 20:12:22 PDT
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?
Comment 24 Darin Adler 2018-05-25 20:34:42 PDT
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.
Comment 25 Nan Wang 2018-05-25 21:21:19 PDT
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
Comment 26 Nan Wang 2018-05-25 21:21:54 PDT
Created attachment 341385 [details]
patch

update from review
Comment 27 Darin Adler 2018-05-28 17:27:28 PDT
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.
Comment 28 Nan Wang 2018-05-29 08:59:26 PDT
(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
Comment 29 Nan Wang 2018-05-29 09:19:13 PDT
Committed r232259: <https://trac.webkit.org/changeset/232259>
Comment 30 Darin Adler 2018-05-29 21:43:37 PDT
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.