Bug 133512 – [ATK] Missing 'selection-changed' signal when navigating a combo box with keyboard

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Mario Sanchez Prada]

Created attachment 232477 [details]
accessible-event listener

Steps to reproduce:
1. Launch the attached accessible-event listener in a terminal
2. Load the attached test case is MiniBrowser
3. Tab into the combo box with the keyboard (focus on it)
4. Navigate the different options with the keyboard (using arrow keys)

Expected results:

In the terminal, you should see that the selection-changed signal is being emitted from WebCore, by printing lines like the following one:

  Selection changed for 'combo box' object. RAW DATA:
   object:selection-changed(0, 0, 0)
    source: [combo box | bar]
    host_application: [application | WebKitWebProcess]

Actual results: Nothing is printed by the listener app, meaning that no signal is being emmitted.

Note: This works fine in epiphany 3.10.3, so it's a bug in master.

[Mario Sanchez Prada]

Created attachment 232478 [details]
test case

[Mario Sanchez Prada]

Created attachment 232484 [details]
Path proposal

This patch fixes the issue and gets a couple of tests more running properly as a plus :)

Please review, thanks

[Mario Sanchez Prada]

(Forgot to add chris to CC before uploading the patch)

[Darin Adler]

Comment on attachment 232484 [details]
Path proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=232484&action=review

Looks like the fix is not entirely platform-specific, but the tests are. Any way we can do tests that can run on the other platforms too?

> Source/WebCore/accessibility/AccessibilityMenuList.cpp:117
> +        // We need to ensure the popup has children before updating its active option to protect against
> +        // situations like the one in accessibility/insert-selected-option-into-select-causes-crash.html.

This comment is too oblique. It doesn’t say what the problem is. But it could. Please be more specific about the problem in the comment.

> Source/WebCore/rendering/RenderMenuList.cpp:429
> +    // Ensure that the passed index is in a valid range before updating accessibility.

This comment doesn’t seem helpful. The code says the same thing as the comment. The comment doesn’t say why or anything like that. Please add comments when they say something that the code does not say.

[Mario Sanchez Prada]

(In reply to comment #4)
> (From update of attachment 232484 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232484&action=review
> 
> Looks like the fix is not entirely platform-specific, but the tests are.
> Any way we can do tests that can run on the other platforms too?
> 
You are perfectly right. Sorry I did not realize of this before and focus just in reimplementing the combo-box text for GTK only. Next patch will make that test cross platform (or at least I will try) and move it out to LayoutTests/accessibility.

> > Source/WebCore/accessibility/AccessibilityMenuList.cpp:117
> > +        // We need to ensure the popup has children before updating its active option to protect against
> > +        // situations like the one in accessibility/insert-selected-option-into-select-causes-crash.html.
> 
> This comment is too oblique. It doesn’t say what the problem is. But it could. Please be more specific about the problem in the comment.

Again, sorry. Next patch will hopefully explain it better :)

> > Source/WebCore/rendering/RenderMenuList.cpp:429
> > +    // Ensure that the passed index is in a valid range before updating accessibility.
> 
> This comment doesn’t seem helpful. The code says the same thing as the comment. The comment doesn’t say why or anything like that. Please add comments when they say something that the code does not say.

Ok, will remove it

[Mario Sanchez Prada]

Created attachment 232560 [details]
Path proposal

Now attaching the new one...

[Build Bot]

Comment on attachment 232560 [details]
Path proposal

Attachment 232560 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6126403745480704

New failing tests:
accessibility/combo-box-collapsed-selection-changed.html

[Build Bot]

Created attachment 232564 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232560 [details]
Path proposal

Attachment 232560 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6512207772778496

New failing tests:
accessibility/combo-box-collapsed-selection-changed.html

[Build Bot]

Created attachment 232577 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

I have been taking a look to this and I can't see why the combo-box test is timing out in the Mac, as I haven't found anything in the HTML/JavaScript code, nor in the two changes in RenderMenuList and AccessibilityMenuList, that tell me why it's not finishing.

I'm currently trying to get a OS X box to try this out, but not sure how long that  will take, so I would appreciate any help with this in the meanwhile, if possible.

Thanks!

[Mario Sanchez Prada]

Created attachment 233052 [details]
Path proposal

(In reply to comment #11)
> I have been taking a look to this and I can't see why the combo-box test is timing
> out in the Mac, as I haven't found anything in the HTML/JavaScript code, nor in the
> two changes in RenderMenuList and AccessibilityMenuList, that tell me why it's not
> finishing.
>
> I'm currently trying to get a OS X box to try this out, but not sure how long that
> will take, so I would appreciate any help with this in the meanwhile, if possible.
> 

I finally got a Mac to try this out and indeed I managed to reproduce the timeout, which is good. However, I could also check that such a timeout has nothing to do with the changes in RenderMenuList.cpp and AccessibilityMenuList.cpp, but with the test itself, which is blocked after each call to eventSender.keyDown() for some reason.

Actually, executing the test manually I can see the popup showing up in the screen, and the test only finishes if I manually close it with Esc key. However, even if I do that I don't see any notification being fired, and a more in deep investigation reveled that the code inside the "if (!chidObjects.isEmpty() { ... }" is never been executed, and I'm not sure what ithat means. Perhaps that notification over the popup is not meant to be emitted in the Mac, but I'm not sure.

I'm the meantime, and in order to try to fix this issue at least in ATK based platforms, I'm attaching a new patch that adds this test to TestExpectations, together with other tests that might be in a similar situation.

Please review, thanks!

[Radar WebKit Bug Importer]

<rdar://problem/17302372>

[chris fleizach]

Comment on attachment 233052 [details]
Path proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=233052&action=review

> Source/WebCore/ChangeLog:9
> +        option when it changes, which will send a platform-dependant

platform-dependEnt

> Source/WebCore/rendering/RenderMenuList.cpp:439
> +    if (AccessibilityMenuList* menuList = toAccessibilityMenuList(document().axObjectCache()->get(this)))

we should do a check here
if (AXObjectCache* cache = document()->existingAXObjectCache()) {

}

[Mario Sanchez Prada]

(In reply to comment #14)
> (From update of attachment 233052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233052&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        option when it changes, which will send a platform-dependant
> 
> platform-dependEnt
> 
Oops!

> > Source/WebCore/rendering/RenderMenuList.cpp:439
> > +    if (AccessibilityMenuList* menuList = toAccessibilityMenuList(document().axObjectCache()->get(this)))
> 
> we should do a check here
> if (AXObjectCache* cache = document()->existingAXObjectCache()) {
> 
> }

Will do before landing.

Thanks!

[Mario Sanchez Prada]

Committed r170008: <http://trac.webkit.org/changeset/170008>