Bug 53146 – [GTK] Combo boxes should emit object:selection-changed even when collapsed

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

[Joanmarie Diggs]

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

[Mario Sanchez Prada]

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.

[Mario Sanchez Prada]

Created attachment 81978 [details]
Patch proposal + Layout test

Attaching patch proposal + Layout test

[Mario Sanchez Prada]

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.

[Mario Sanchez Prada]

Created attachment 82005 [details]
Patch proposal + Layout test

This is the right one... need to take some fresh air

[Mario Sanchez Prada]

Committed r78979: <http://trac.webkit.org/changeset/78979>

[WebKit Review Bot]

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

[Philippe Normand]

Reverted r78979 for reason:

causes multiple crashes on GTK

Committed r78987: <http://trac.webkit.org/changeset/78987>

[Mario Sanchez Prada]

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.

[Martin Robinson]

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.

[Mario Sanchez Prada]

Committed r79321: <http://trac.webkit.org/changeset/79321>

[Philippe Normand]

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>

[Mario Sanchez Prada]

Comment on attachment 82943 [details]
Patch proposal + Layout test

Looks like I'm doomed with this one.

Clearing flags

[Mario Sanchez Prada]

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.

[Philippe Normand]

2 tests skipped in http://trac.webkit.org/changeset/79989
are fixed by this patch. Please unskip when landing!

[WebKit Review Bot]

http://trac.webkit.org/changeset/79989 might have broken Leopard Intel Release (Build)

[Philippe Normand]

fast/spatial-navigation/snav-single-select-list.html is also affected, skipped in r79990

And sheriff warning above is false positive I think.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79990 might have broken GTK Linux 32-bit Debug

[Martin Robinson]

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.

[WebKit Commit Bot]

Comment on attachment 83786 [details]
Patch proposal + Layout test

Clearing flags on attachment: 83786

Committed r80214: <http://trac.webkit.org/changeset/80214>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

Tests unskipped in http://trac.webkit.org/changeset/80216