Currently accessibility setValue on contenteditable elements would collapse whitespaces. <rdar://problem/36563892>
Created attachment 341053 [details] patch
Created attachment 341054 [details] patch remove redundant header
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?
(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
Created attachment 341062 [details] patch update
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
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
The test failure shouldn't be related
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?
(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
Committed r232120: <https://trac.webkit.org/changeset/232120>
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.
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.
(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?
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.
(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.
Created attachment 341369 [details] patch Reopen and try to fix this the right way.
Reopen this bug
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
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
will work on the test
Created attachment 341377 [details] patch Updated test expectation for Mac wk1
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 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 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
Created attachment 341385 [details] patch update from review
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.
(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
Committed r232259: <https://trac.webkit.org/changeset/232259>
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.