Bug 133512

Created attachment 232478 [details] test case

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

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

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.

(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

Created attachment 232560 [details] Path proposal Now attaching the new one...

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

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

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

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!

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!

<rdar://problem/17302372>

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()) { }

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

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