RESOLVED FIXED 185897
AX: setValue on contenteditable should preserve whitespace
https://bugs.webkit.org/show_bug.cgi?id=185897
Summary AX: setValue on contenteditable should preserve whitespace
Nan Wang
Reported 2018-05-22 18:17:53 PDT
Currently accessibility setValue on contenteditable elements would collapse whitespaces. <rdar://problem/36563892>
Attachments
patch (4.22 KB, patch)
2018-05-22 18:25 PDT, Nan Wang
no flags
patch (4.00 KB, patch)
2018-05-22 18:26 PDT, Nan Wang
no flags
patch (4.04 KB, patch)
2018-05-22 19:46 PDT, Nan Wang
cfleizach: review+
ews-watchlist: commit-queue-
Archive of layout-test-results from ews204 for win-future (12.73 MB, application/zip)
2018-05-22 21:31 PDT, EWS Watchlist
no flags
patch (6.17 KB, patch)
2018-05-25 18:03 PDT, Nan Wang
ews-watchlist: commit-queue-
Archive of layout-test-results from ews116 for mac-sierra (3.25 MB, application/zip)
2018-05-25 19:23 PDT, EWS Watchlist
no flags
patch (6.92 KB, patch)
2018-05-25 19:45 PDT, Nan Wang
no flags
patch (6.68 KB, patch)
2018-05-25 21:21 PDT, Nan Wang
rniwa: review+
Nan Wang
Comment 1 2018-05-22 18:25:21 PDT
Nan Wang
Comment 2 2018-05-22 18:26:21 PDT
Created attachment 341054 [details] patch remove redundant header
chris fleizach
Comment 3 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?
Nan Wang
Comment 4 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
Nan Wang
Comment 5 2018-05-22 19:46:00 PDT
Created attachment 341062 [details] patch update
EWS Watchlist
Comment 6 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
EWS Watchlist
Comment 7 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
Nan Wang
Comment 8 2018-05-23 00:26:30 PDT
The test failure shouldn't be related
chris fleizach
Comment 9 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?
Nan Wang
Comment 10 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
Nan Wang
Comment 11 2018-05-23 11:38:43 PDT
Darin Adler
Comment 12 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.
Darin Adler
Comment 13 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.
Nan Wang
Comment 14 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?
Darin Adler
Comment 15 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.
Nan Wang
Comment 16 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.
Nan Wang
Comment 17 2018-05-25 18:03:50 PDT
Created attachment 341369 [details] patch Reopen and try to fix this the right way.
Nan Wang
Comment 18 2018-05-25 18:04:31 PDT
Reopen this bug
EWS Watchlist
Comment 19 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
EWS Watchlist
Comment 20 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
Nan Wang
Comment 21 2018-05-25 19:31:31 PDT
will work on the test
Nan Wang
Comment 22 2018-05-25 19:45:42 PDT
Created attachment 341377 [details] patch Updated test expectation for Mac wk1
chris fleizach
Comment 23 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?
Darin Adler
Comment 24 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.
Nan Wang
Comment 25 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
Nan Wang
Comment 26 2018-05-25 21:21:54 PDT
Created attachment 341385 [details] patch update from review
Darin Adler
Comment 27 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.
Nan Wang
Comment 28 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
Nan Wang
Comment 29 2018-05-29 09:19:13 PDT
Darin Adler
Comment 30 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.
Note You need to log in before you can comment on or make changes to this bug.