Bug 133512

Summary: [ATK] Missing 'selection-changed' signal when navigating a combo box with keyboard
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apinheiro, buildbot, cfleizach, jdiggs, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Attachments:
Description Flags
accessible-event listener
none
test case
none
Path proposal
none
Path proposal
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Path proposal cfleizach: review+

Description Mario Sanchez Prada 2014-06-04 08:14:47 PDT
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.
Comment 1 Mario Sanchez Prada 2014-06-04 08:15:54 PDT
Created attachment 232478 [details]
test case
Comment 2 Mario Sanchez Prada 2014-06-04 09:10:32 PDT
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
Comment 3 Mario Sanchez Prada 2014-06-04 09:15:54 PDT
(Forgot to add chris to CC before uploading the patch)
Comment 4 Darin Adler 2014-06-04 10:27:20 PDT
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.
Comment 5 Mario Sanchez Prada 2014-06-05 10:12:12 PDT
(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
Comment 6 Mario Sanchez Prada 2014-06-05 10:14:55 PDT
Created attachment 232560 [details]
Path proposal

Now attaching the new one...
Comment 7 Build Bot 2014-06-05 10:52:07 PDT
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
Comment 8 Build Bot 2014-06-05 10:52:10 PDT
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
Comment 9 Build Bot 2014-06-05 13:22:50 PDT
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
Comment 10 Build Bot 2014-06-05 13:22:54 PDT
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
Comment 11 Mario Sanchez Prada 2014-06-06 05:47:00 PDT
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!
Comment 12 Mario Sanchez Prada 2014-06-13 08:25:16 PDT
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!
Comment 13 Radar WebKit Bug Importer 2014-06-13 08:25:50 PDT
<rdar://problem/17302372>
Comment 14 chris fleizach 2014-06-13 09:41:20 PDT
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()) {

}
Comment 15 Mario Sanchez Prada 2014-06-16 02:33:36 PDT
(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!
Comment 16 Mario Sanchez Prada 2014-06-16 08:01:40 PDT
Committed r170008: <http://trac.webkit.org/changeset/170008>