Bug 153062 - [GTK] accessibility/gtk/entry-and-password.html is failing since r194847
Summary: [GTK] accessibility/gtk/entry-and-password.html is failing since r194847
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: All Linux
: P2 Normal
Assignee: Joanmarie Diggs (irc: joanie)
URL:
Keywords:
Depends on: 153212
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-13 04:21 PST by Mario Sanchez Prada
Modified: 2016-04-19 02:50 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.85 KB, patch)
2016-01-13 16:48 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (6.87 KB, patch)
2016-04-18 15:57 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
patch for landing (6.91 KB, patch)
2016-04-19 01:55 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2016-01-13 04:21:06 PST
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
Comment 1 Mario Sanchez Prada 2016-01-13 05:50:44 PST
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.
Comment 2 Carlos Garcia Campos 2016-01-13 06:01:18 PST
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.
Comment 3 Mario Sanchez Prada 2016-01-13 06:15:37 PST
I see, let's add Joanie on CC then, she might know better what to expect. Thanks
Comment 4 Joanmarie Diggs (irc: joanie) 2016-01-13 11:02:04 PST
(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!
Comment 5 Joanmarie Diggs (irc: joanie) 2016-01-13 14:09:54 PST
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}"
Comment 6 Joanmarie Diggs (irc: joanie) 2016-01-13 16:48:01 PST
Created attachment 268914 [details]
Patch
Comment 7 Joanmarie Diggs (irc: joanie) 2016-01-13 16:54:17 PST
(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.
Comment 8 Carlos Garcia Campos 2016-01-13 22:50:22 PST
(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 9 Carlos Garcia Campos 2016-01-13 22:53:33 PST
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.
Comment 10 Joanmarie Diggs (irc: joanie) 2016-01-14 07:06:55 PST
(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!
Comment 11 Joanmarie Diggs (irc: joanie) 2016-01-18 15:49:04 PST
As best as I can tell (see 153212), the indicator is not being exposed via platform accessibility APIs.
Comment 12 Joanmarie Diggs (irc: joanie) 2016-04-18 15:57:07 PDT
Created attachment 276677 [details]
Patch
Comment 13 Joanmarie Diggs (irc: joanie) 2016-04-18 16:13:28 PDT
Comment on attachment 276677 [details]
Patch

Carlos: Please review. Thanks!
Comment 14 Carlos Garcia Campos 2016-04-19 00:05:24 PDT
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.
Comment 15 Joanmarie Diggs (irc: joanie) 2016-04-19 01:55:55 PDT
Created attachment 276706 [details]
patch for landing
Comment 16 WebKit Commit Bot 2016-04-19 02:50:11 PDT
Comment on attachment 276706 [details]
patch for landing

Clearing flags on attachment: 276706

Committed r199715: <http://trac.webkit.org/changeset/199715>
Comment 17 WebKit Commit Bot 2016-04-19 02:50:17 PDT
All reviewed patches have been landed.  Closing bug.