The following layout test is failing on GTK, probably since r194847 accessibility/gtk/entry-and-password.html Blame list is r194846 - r194849 [1], where r194846 and r194847 look like the suspicious ones. However, I still get the failure if I revert r194846, so that seems to point to r194847 instead, will comment here once I finish building to confirm it. [1] https://trac.webkit.org/log/?verbose=on&rev=194849&stop_rev=194846
Confirmed: test passes until r194846 (included) and starts failing in r194847, as I could see by doing git reset --hard and building+running from there.
I noticed it, but didn't rebaseline it because I had no idea if the current results are wrong or not and forgot to file a bug report.
I see, let's add Joanie on CC then, she might know better what to expect. Thanks
(In reply to comment #3) > I see, let's add Joanie on CC then, she might know better what to expect. /me looks in horror at the new child of the AXPasswordField :P This needs to be fixed; not rebaselined. I'll see what's up. Thanks!
Before we fix this in the platform accessibility code.... The reason for the additional child is due to the insertion of two new RenderObjects under the RenderTextControl associated with the password field. The addition was only for the password field; not the text field. (See below.) So my questions are: 1. Is this modification of the render tree deliberate and/or desired? 2. If the answer to 1 is "yes", is the lack of additional render objects for the non-password field deliberate and/or desired? BEFORE: layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x600 RenderBlock {HTML} at (0,0) size 800x600 RenderBody {BODY} at (8,8) size 784x576 RenderBlock {FORM} at (0,0) size 784x27 RenderTextControl {INPUT} at (2,2) size 191x23 [bgcolor=#FFFFFF] [border: (2px inset #000000)] RenderText {#text} at (195,5) size 4x17 text run at (195,5) width 4: " " RenderTextControl {INPUT} at (201,2) size 191x23 [bgcolor=#FFFFFF] [border: (2px inset #000000)] RenderText {#text} at (0,0) size 0x0 RenderBlock {DIV} at (0,43) size 784x17 RenderText {#text} at (0,0) size 68x17 text run at (0,0) width 68: "End of test" RenderBlock {PRE} at (0,73) size 784x0 RenderBlock {P} at (0,76) size 784x0 RenderBlock {DIV} at (0,76) size 784x0 layer at (13,13) size 185x17 RenderBlock {DIV} at (3,3) size 185x17 RenderText {#text} at (0,0) size 27x17 text run at (0,0) width 27: "123" layer at (212,13) size 185x17 RenderBlock {DIV} at (3,3) size 185x17 RenderText {#text} at (0,0) size 18x17 text run at (0,0) width 18: "\x{2022}\x{2022}\x{2022}" AFTER: layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x600 RenderBlock {HTML} at (0,0) size 800x600 RenderBody {BODY} at (8,8) size 784x576 RenderBlock {FORM} at (0,0) size 784x27 RenderTextControl {INPUT} at (2,2) size 191x23 [bgcolor=#FFFFFF] [border: (2px inset #000000)] RenderText {#text} at (195,5) size 4x17 text run at (195,5) width 4: " " RenderTextControl {INPUT} at (201,2) size 191x23 [bgcolor=#FFFFFF] [border: (2px inset #000000)] RenderFlexibleBox {DIV} at (3,3) size 185x17 RenderBlock {DIV} at (0,0) size 185x17 RenderText {#text} at (0,0) size 0x0 RenderBlock {DIV} at (0,43) size 784x17 RenderText {#text} at (0,0) size 68x17 text run at (0,0) width 68: "End of test" RenderBlock {PRE} at (0,73) size 784x0 RenderBlock {P} at (0,76) size 784x0 RenderBlock {DIV} at (0,76) size 784x0 layer at (13,13) size 185x17 RenderBlock {DIV} at (3,3) size 185x17 RenderText {#text} at (0,0) size 27x17 text run at (0,0) width 27: "123" layer at (212,13) size 185x17 RenderBlock {DIV} at (0,0) size 185x17 RenderText {#text} at (0,0) size 18x17 text run at (0,0) width 18: "\x{2022}\x{2022}\x{2022}"
Created attachment 268914 [details] Patch
(In reply to comment #6) > Created attachment 268914 [details] > Patch Given the timezone difference and some stuff I need to work on tomorrow, I'm going to gamble that the answers to the questions I ask in comment 5 are "yes" and "yes". :) In which case, please review.
(In reply to comment #5) > Before we fix this in the platform accessibility code.... > > The reason for the additional child is due to the insertion of two new > RenderObjects under the RenderTextControl associated with the password > field. The addition was only for the password field; not the text field. > (See below.) So my questions are: > > 1. Is this modification of the render tree deliberate and/or desired? Yes. I think the new node is the caps lock indicator that is now implemented using shadow DOM. See bug #141868. > 2. If the answer to 1 is "yes", is the lack of additional render objects for > the non-password field deliberate and/or desired? Yes, because the caps lock indicator is only used in password fields.
Comment on attachment 268914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268914&action=review > Source/WebCore/ChangeLog:10 > + The changes in r194847 include the addition of a RenderObject subtree to > + lay out the contents of password fields. None of this subtree should be > + exposed to assistive technologies. We now check for this condition. I would explain here why there's a new RenderObject for password fields, and why we don't want to expose the caps lock indicator to a11y. > Source/WebCore/accessibility/atk/AccessibilityObjectAtk.cpp:89 > + // Don't expose the RenderObject subtree used to lay out password text. And here I would also mention that this is the shadow DOM used to implement the caps lock indicator in password fields.
(In reply to comment #9) > Comment on attachment 268914 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268914&action=review > > > Source/WebCore/ChangeLog:10 > > + The changes in r194847 include the addition of a RenderObject subtree to > > + lay out the contents of password fields. None of this subtree should be > > + exposed to assistive technologies. We now check for this condition. > > I would explain here why there's a new RenderObject for password fields, and > why we don't want to expose the caps lock indicator to a11y. When you put it that way, and given that I've had some sleep.... :) The caps lock indicator should be accessible -- when it's indicating something. I want to take a look at how we can do that. If it's not a trivial patch, I'll open a new issue for it and just clean up the current patch to address your review. Otherwise I'll do a new patch for both. Thanks for the review!
As best as I can tell (see 153212), the indicator is not being exposed via platform accessibility APIs.
Created attachment 276677 [details] Patch
Comment on attachment 276677 [details] Patch Carlos: Please review. Thanks!
Comment on attachment 276677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276677&action=review > Source/WebCore/accessibility/atk/AccessibilityObjectAtk.cpp:104 > + if (node && is<TextControlInnerTextElement>(*node)) There's an is<> that receives a pointer and already checks nullptr, so you could just pass the pointer instead and remove the null check.
Created attachment 276706 [details] patch for landing
Comment on attachment 276706 [details] patch for landing Clearing flags on attachment: 276706 Committed r199715: <http://trac.webkit.org/changeset/199715>
All reviewed patches have been landed. Closing bug.