Bug 53146

Summary: [GTK] Combo boxes should emit object:selection-changed even when collapsed
Product: WebKit Reporter: Joanmarie Diggs (irc: joanie) <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apinheiro, commit-queue, eric, mario, pnormand, webkit.review.bot
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 54116, 54185, 54198    
Bug Blocks: 25531    
Attachments:
Description Flags
Patch proposal + Layout test
none
Patch proposal + Layout test
none
Patch proposal + Layout test
none
Patch proposal + Layout test none

Description Joanmarie Diggs (irc: joanie) 2011-01-25 16:50:30 PST
Currently when you arrow around in a combo box without first expanding it, the combo box emits object:state-changed:focused; but it does not emit object:selection-changed.

This suggests to an AT that focus used to be elsewhere (i.e. not on the combo box) and that the currently-selected item has not changed.

Yes, an AT *could* figure out this quirk and present the newly-selected item, because WebKitGtk's AtkSelection implementation seems to be doing the right thing regardless of whether or not the combo box is expanded. (Thanks!) BUT it would be much nicer if the combo box also announced the selection change. :-)

(Mario: If this is a hassle for some reason, lemme know so that I can handle it in Orca.)
Comment 1 Mario Sanchez Prada 2011-02-10 07:08:11 PST
Joanie, just to keep you posted on this: I'm currently working on this stuff and I even have a patch for it, but couldn't propose it yet since I found some things missing in DRT that would be required to implement a proper test (see bug 54116 and bug 54185).

I'll upload the patch + layout test here as soon as those get fixed.
Comment 2 Mario Sanchez Prada 2011-02-10 07:30:34 PST
Created attachment 81978 [details]
Patch proposal + Layout test

Attaching patch proposal + Layout test
Comment 3 Mario Sanchez Prada 2011-02-10 07:31:45 PST
In the attached patch, I assumed that bug 54198 was fixed, as that would impact the expected file.

Hence this patch shouldn't be applied prior to patch for bug 54198, so that's why I'm setting a dependency here.
Comment 4 Mario Sanchez Prada 2011-02-10 10:55:50 PST
Created attachment 82005 [details]
Patch proposal + Layout test

This is the right one... need to take some fresh air
Comment 5 Mario Sanchez Prada 2011-02-18 01:46:33 PST
Committed r78979: <http://trac.webkit.org/changeset/78979>
Comment 6 WebKit Review Bot 2011-02-18 02:38:32 PST
http://trac.webkit.org/changeset/78979 might have broken GTK Linux 32-bit Release
The following tests are not passing:
fast/events/popup-when-select-change.html
fast/forms/listbox-hit-test-zoomed.html
fast/forms/listbox-typeahead-greek.html
fast/forms/select-type-ahead-non-latin.html
fast/spatial-navigation/snav-single-select.html
inspector/debugger/debug-inlined-scripts.html
inspector/debugger/debugger-no-nested-pause.html
inspector/debugger/debugger-step-in.html
platform/gtk/accessibility/combo-box-collapsed-selection-changed.html
platform/gtk/fast/forms/menulist-typeahead-find.html
Comment 7 Philippe Normand 2011-02-18 02:53:32 PST
Reverted r78979 for reason:

causes multiple crashes on GTK

Committed r78987: <http://trac.webkit.org/changeset/78987>
Comment 8 Mario Sanchez Prada 2011-02-18 04:53:27 PST
Created attachment 82943 [details]
Patch proposal + Layout test

(In reply to comment #7)
> Reverted r78979 for reason:
> 
> causes multiple crashes on GTK
> 
> Committed r78987: <http://trac.webkit.org/changeset/78987>

Attaching a new version of the patch fixing that issue. The diff between the new patch and the previous one is as follows:

--- a/Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp
+++ b/Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp
@@ -95,10 +95,10 @@ static void notifyChildrenSelectionChange(AccessibilityObject* object)
         return;
     AccessibilityObject* item = items.at(changedItemIndex).get();
 
-    // Ensure the oldFocusedObject belongs to the same document that
+    // Ensure oldFocusedObject belongs to the same list object that
     // the current item so further comparisons make sense. Otherwise,
     // just reset oldFocusedObject so it won't be taken into account.
-    if (item && oldFocusedObject && item->document() != oldFocusedObject->document())
+    if (item && oldFocusedObject && item->parentObject() != oldFocusedObject->parentObject())
         oldFocusedObject = 0;


The problem was that at some point AccessibleObject:documentFrameView() (called internally from AccessibilityObject::document()) was crashing because of reaching and invalid object when called oldFocusedObject->document(). And this was easily avoidable by just checking that the parent object (instead of the document) for the currently selected item ('item') and the old focused object are the same, which is more than enough.

Tested locally and this doesn't make other tests crash anymore.
Comment 9 Martin Robinson 2011-02-21 15:54:17 PST
Comment on attachment 82943 [details]
Patch proposal + Layout test

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

> LayoutTests/platform/gtk/accessibility/combo-box-collapsed-selection-changed.html:12
> +  description("This tests that the 'state-changed:selected' signal is emitted when arrowing through the options of a combobox while collapsed.");
> +
> +  if (window.layoutTestController) {

Please use 4 space indents.
Comment 10 Mario Sanchez Prada 2011-02-22 07:26:43 PST
Committed r79321: <http://trac.webkit.org/changeset/79321>
Comment 11 Philippe Normand 2011-02-22 10:25:13 PST
Reverted r79321 for reason:

Regresses  fast/forms/listbox-typeahead-cyrillic.html and fast/spatial-navigation/snav-single-select.html on GTK

Committed r79332: <http://trac.webkit.org/changeset/79332>
Comment 12 Mario Sanchez Prada 2011-02-22 10:38:41 PST
Comment on attachment 82943 [details]
Patch proposal + Layout test

Looks like I'm doomed with this one.

Clearing flags
Comment 13 Mario Sanchez Prada 2011-02-25 02:01:20 PST
Created attachment 83786 [details]
Patch proposal + Layout test

Ok, here we go again... strike 3!

I've changed the code, once again, this time to directly compare the list object currently used with the list object used before, so if they're not the same we assume further comparisons make no sense. Semantically it's the same than asking for the parentObject() of a listBox option object, but without having to call oldFocusedObject->parentObject(), which was what was causing the failure before.

I've tested locally and couldn't reproduce crashes anymore. Also Philippe (who was able to reproduce the crashes always in a more reliable way) tested this new patch and confirms he doesn't get crashes anymore, so I hope this time will be "the good one (tm)"

Sorry for the hassle.
Comment 14 Philippe Normand 2011-03-01 04:10:05 PST
2 tests skipped in http://trac.webkit.org/changeset/79989
are fixed by this patch. Please unskip when landing!
Comment 15 WebKit Review Bot 2011-03-01 05:15:55 PST
http://trac.webkit.org/changeset/79989 might have broken Leopard Intel Release (Build)
Comment 16 Philippe Normand 2011-03-01 05:18:20 PST
fast/spatial-navigation/snav-single-select-list.html is also affected, skipped in r79990

And sheriff warning above is false positive I think.
Comment 17 WebKit Review Bot 2011-03-01 08:39:02 PST
http://trac.webkit.org/changeset/79990 might have broken GTK Linux 32-bit Debug
Comment 18 Martin Robinson 2011-03-01 09:12:20 PST
Comment on attachment 83786 [details]
Patch proposal + Layout test

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

Please unskip tests that were previously crashing with this commit.

> LayoutTests/platform/gtk/accessibility/combo-box-collapsed-selection-changed.html:26
> +  description("This tests that the 'state-changed:selected' signal is emitted when arrowing through the options of a combobox while collapsed.");
> +
> +  if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +  }
> +
> +  if (window.accessibilityController) {
> +    accessibilityController.logAccessibilityEvents();
> +  }
> +
> +  // Focus in the combobox and move around the options. The signal
> +  // 'state-change:selected' should be emitted with every change.
> +  document.getElementById("combo").focus();
> +  eventSender.keyDown("downArrow");
> +  eventSender.keyDown("downArrow");
> +  eventSender.keyDown("upArrow");
> +  eventSender.keyDown("upArrow");

Please switch this to four space indent.

> Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:106
> +    // Ensure the current list object is the same than the old one so
> +    // further comparisons make sense. Otherwise, just reset
> +    // oldFocusedObject so it won't be taken into account.
> +    if (oldListObject != listObject)
>          oldFocusedObject = 0;

This approach is incorrect for dealing with multiple views, I think, but that's a bigger problem that should be addressed in some other patch. There's probably a better way to handle focus changes than caching only one oldFocusedObject statically.
Comment 19 WebKit Commit Bot 2011-03-03 01:00:33 PST
Comment on attachment 83786 [details]
Patch proposal + Layout test

Clearing flags on attachment: 83786

Committed r80214: <http://trac.webkit.org/changeset/80214>
Comment 20 WebKit Commit Bot 2011-03-03 01:00:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Philippe Normand 2011-03-03 02:10:32 PST
Tests unskipped in http://trac.webkit.org/changeset/80216