Bug 122148 – [ATK] Expose ARIA Busy state

• Component: Accessibility
• Status: UNCONFIRMED

[Krzysztof Czech]

Expose aria-busy state.

[Radar WebKit Bug Importer]

<rdar://problem/15119077>

[Krzysztof Czech]

Created attachment 213067 [details]
Patch

[Krzysztof Czech]

I found that, changes I proposed make accessibility/loading-iframe-sends-notification.html test failing both for EFL and GTK.

[Mario Sanchez Prada]

Comment on attachment 213067 [details]
Patch

Looks good to me. Also, I realized that we are not exposing ATK_STATE_BUSY in the ATK wrapper.

Perhaps it would be a good thing to do now for consistency with the fact that we are exposing the state change (but in a different bug anyway). What do you think?

[Mario Sanchez Prada]

Comment on attachment 213067 [details]
Patch

(In reply to comment #3)
> I found that, changes I proposed make accessibility/loading-iframe-sends-notification.html test failing both for EFL and GTK.

(Sorry, I didn't see this message until now. Removing the cq flag for now, just in case)

How to they fail exactly? Could you paste here the diff output?

Perhaps they just need some rebaselining after all...

[Krzysztof Czech]

  PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is false
  Got notification on iframe.
 +FAIL findByAccessibleTitleSubstring(root, 'InnerButton') != null should be true. Was false.
 +Got notification on iframe.
 +PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
 +Got notification on iframe.
  PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
  PASS gotIframeNotification is true
  PASS successfullyParsed is true



This is a diff output while I'm running accessibility/loading-iframe-sends-notification.html with the changes related to exposing aria-busy notification

[Mario Sanchez Prada]

(In reply to comment #6)
>   PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is false
>   Got notification on iframe.
>  +FAIL findByAccessibleTitleSubstring(root, 'InnerButton') != null should be true. Was false.
>  +Got notification on iframe.
>  +PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
>  +Got notification on iframe.
>   PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
>   PASS gotIframeNotification is true
>   PASS successfullyParsed is true
> 
> 
> 
> This is a diff output while I'm running accessibility/loading-iframe-sends-notification.html
> with the changes related to exposing aria-busy notification

Hrm... I think what it's happening here is that in the following piece of code in that test...

    [...]
    // Initially, the iframe should not be loaded, so we shouldn't be able to find this button.
    shouldBeFalse("findByAccessibleTitleSubstring(root, 'InnerButton') != null");

    window.accessibilityController.addNotificationListener(function (target, notification) {
        // Ignore this notification if it's not on the iframe.
        if (target.description.indexOf("InnerFrame") == -1)
            return;

        debug("Got notification on iframe.");
        gotIframeNotification = true;

        // Check that the button within the iframe is now reachable from the root.
        shouldBeTrue("findByAccessibleTitleSubstring(root, 'InnerButton') != null");
    });
    [...]


... the notification listener is now being called more often than before, as now you are also emitting a signal/notification when the busy state changes (AXElementBusyChanged), not simply when the layout is completed (AXLayoutComplete), which I think is the only case that was being captured before in that listener. Adding a "debug('Notification received: ' + notification);" line in that test could be useful to check whether that theory is true.

If that's the case, maybe we could update this test so that listener early returns when the notification received is not the expected one (AXLayoutComplete, I suppose), but still it's a mystery to me why this problem is not happening in the Mac in the first place, since I guess they should be getting the additional notification there as well.

Chris, Samuel, any comment on this?

[Samuel White]

(In reply to comment #7)
> (In reply to comment #6)
> >   PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is false
> >   Got notification on iframe.
> >  +FAIL findByAccessibleTitleSubstring(root, 'InnerButton') != null should be true. Was false.
> >  +Got notification on iframe.
> >  +PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
> >  +Got notification on iframe.
> >   PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
> >   PASS gotIframeNotification is true
> >   PASS successfullyParsed is true
> > 
> > 
> > 
> > This is a diff output while I'm running accessibility/loading-iframe-sends-notification.html
> > with the changes related to exposing aria-busy notification
> 
> Hrm... I think what it's happening here is that in the following piece of code in that test...
> 
>     [...]
>     // Initially, the iframe should not be loaded, so we shouldn't be able to find this button.
>     shouldBeFalse("findByAccessibleTitleSubstring(root, 'InnerButton') != null");
> 
>     window.accessibilityController.addNotificationListener(function (target, notification) {
>         // Ignore this notification if it's not on the iframe.
>         if (target.description.indexOf("InnerFrame") == -1)
>             return;
> 
>         debug("Got notification on iframe.");
>         gotIframeNotification = true;
> 
>         // Check that the button within the iframe is now reachable from the root.
>         shouldBeTrue("findByAccessibleTitleSubstring(root, 'InnerButton') != null");
>     });
>     [...]
> 
> 
> ... the notification listener is now being called more often than before, as now you are also emitting a signal/notification when the busy state changes (AXElementBusyChanged), not simply when the layout is completed (AXLayoutComplete), which I think is the only case that was being captured before in that listener. Adding a "debug('Notification received: ' + notification);" line in that test could be useful to check whether that theory is true.

Agreed. We should be seeing one AXLoadComplete and one AXLayoutComplete notification, not sure about the third one. Adding a debug as suggested to dump the notification type should shed some light on what is going wrong here.

> 
> If that's the case, maybe we could update this test so that listener early returns when the notification received is not the expected one (AXLayoutComplete, I suppose), but still it's a mystery to me why this problem is not happening in the Mac in the first place, since I guess they should be getting the additional notification there as well.
> 
> Chris, Samuel, any comment on this?

[Krzysztof Czech]

(In reply to comment #4)
> (From update of attachment 213067 [details])
> Looks good to me. Also, I realized that we are not exposing ATK_STATE_BUSY in the ATK wrapper.
> 
> Perhaps it would be a good thing to do now for consistency with the fact that we are exposing the state change (but in a different bug anyway). What do you think?

I agree with you ATK_STATE_BUSY should be exposed. Other thing is that DRT and WTR do not provide isBusy() property for testing it. I guess one could be added in one patch along with exposing ATK_STATE_BUSY. I will propose it. What do you think ?.

[chris fleizach]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > >   PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is false
> > >   Got notification on iframe.
> > >  +FAIL findByAccessibleTitleSubstring(root, 'InnerButton') != null should be true. Was false.
> > >  +Got notification on iframe.
> > >  +PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
> > >  +Got notification on iframe.
> > >   PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
> > >   PASS gotIframeNotification is true
> > >   PASS successfullyParsed is true
> > > 
> > > 
> > > 
> > > This is a diff output while I'm running accessibility/loading-iframe-sends-notification.html
> > > with the changes related to exposing aria-busy notification
> > 
> > Hrm... I think what it's happening here is that in the following piece of code in that test...
> > 
> >     [...]
> >     // Initially, the iframe should not be loaded, so we shouldn't be able to find this button.
> >     shouldBeFalse("findByAccessibleTitleSubstring(root, 'InnerButton') != null");
> > 
> >     window.accessibilityController.addNotificationListener(function (target, notification) {
> >         // Ignore this notification if it's not on the iframe.
> >         if (target.description.indexOf("InnerFrame") == -1)
> >             return;
> > 
> >         debug("Got notification on iframe.");
> >         gotIframeNotification = true;
> > 
> >         // Check that the button within the iframe is now reachable from the root.
> >         shouldBeTrue("findByAccessibleTitleSubstring(root, 'InnerButton') != null");
> >     });
> >     [...]

Generally, you can't be that sure of ordering of notifications I've noticed, especially once tests start being run in parallel. I might rewrite this test so that you only check the new conditions after you've received the specific notification you're looking for. There can be all other kinds of notifications flying around


> > 
> > 
> > ... the notification listener is now being called more often than before, as now you are also emitting a signal/notification when the busy state changes (AXElementBusyChanged), not simply when the layout is completed (AXLayoutComplete), which I think is the only case that was being captured before in that listener. Adding a "debug('Notification received: ' + notification);" line in that test could be useful to check whether that theory is true.
> 
> Agreed. We should be seeing one AXLoadComplete and one AXLayoutComplete notification, not sure about the third one. Adding a debug as suggested to dump the notification type should shed some light on what is going wrong here.
> 
> > 
> > If that's the case, maybe we could update this test so that listener early returns when the notification received is not the expected one (AXLayoutComplete, I suppose), but still it's a mystery to me why this problem is not happening in the Mac in the first place, since I guess they should be getting the additional notification there as well.
> > 
> > Chris, Samuel, any comment on this?

[Mario Sanchez Prada]

(In reply to comment #8)
> [...]
> 
> Agreed. We should be seeing one AXLoadComplete and one AXLayoutComplete 
> notification, not sure about the third one. Adding a debug as suggested 
> to dump the notification type should shed some light on what is going 
> wrong here.

I added a debug line like that right before the shouldBeTrue() line, and this is what I got:


  On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


  PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is false
  Got notification on iframe.

  ### Notification received: AXElementBusyChanged ### 

  FAIL findByAccessibleTitleSubstring(root, 'InnerButton') != null should be true. Was false.
  Got notification on iframe.

  ### Notification received: AXElementBusyChanged ### 

  PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
  Got notification on iframe.

  ### Notification received: AXLayoutComplete ### 

  PASS findByAccessibleTitleSubstring(root, 'InnerButton') != null is true
  PASS gotIframeNotification is true
  PASS successfullyParsed is true

  TEST COMPLETE


If I instead leave Krzysztof's patch out of the game, then I only get the one AXLayoutComplete notification, which seems to be correct according to the test expectations.

I checked the patch again and again, and I don't see it's doing anything different than the mac counterparts (just emitting the event and catching it in DRT/WKTR) so I wonder why this is not happening in the Mac. 

Of course, adding a filter to early return in case the notification is not AXLayoutComplete would "fix" the failing test, but I do want to make sure we are not hiding any other problem

[Mario Sanchez Prada]

(In reply to comment #10)
> Generally, you can't be that sure of ordering of notifications I've noticed, 
> especially once tests start being run in parallel. I might rewrite this test 
> so that you only check the new conditions after you've received the specific 
> notification you're looking for. There can be all other kinds of 
> notifications flying around

See my previous comment about my findings after applying the patch and logging the notifications. That might be helpful.