WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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-watchlist
: 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
,
EWS Watchlist
no flags
Details
patch
(6.17 KB, patch)
2018-05-25 18:03 PDT
,
Nan Wang
ews-watchlist
: 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
,
EWS Watchlist
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Nan Wang
Comment 1
2018-05-22 18:25:21 PDT
Created
attachment 341053
[details]
patch
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
Committed
r232120
: <
https://trac.webkit.org/changeset/232120
>
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
Committed
r232259
: <
https://trac.webkit.org/changeset/232259
>
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.
Top of Page
Format For Printing
XML
Clone This Bug