Bug 153062 – [GTK] accessibility/gtk/entry-and-password.html is failing since r194847

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

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

[Mario Sanchez Prada]

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.

[Carlos Garcia Campos]

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.

[Mario Sanchez Prada]

I see, let's add Joanie on CC then, she might know better what to expect. Thanks

[Joanmarie Diggs]

(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!

[Joanmarie Diggs]

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}"

[Joanmarie Diggs]

Created attachment 268914 [details]
Patch

[Joanmarie Diggs]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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.

[Joanmarie Diggs]

(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!

[Joanmarie Diggs]

As best as I can tell (see 153212), the indicator is not being exposed via platform accessibility APIs.

[Joanmarie Diggs]

Created attachment 276677 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 276677 [details]
Patch

Carlos: Please review. Thanks!

[Carlos Garcia Campos]

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.

[Joanmarie Diggs]

Created attachment 276706 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 276706 [details]
patch for landing

Clearing flags on attachment: 276706

Committed r199715: <http://trac.webkit.org/changeset/199715>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.