Bug 76796 - [Gtk] Combo boxes should be arrow-out-of-able similar to list boxes
: [Gtk] Combo boxes should be arrow-out-of-able similar to list boxes
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: Gtk
:
: 25531
  Show dependency treegraph
 
Reported: 2012-01-22 03:11 PST by
Modified: 2013-04-11 09:37 PST (History)


Attachments
Patch (2.26 KB, patch)
2013-02-26 11:13 PST, Brian Holt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2013-04-09 10:04 PST, Brian Holt
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (1.73 MB, application/zip)
2013-04-09 20:43 PST, Build Bot
no flags Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-22 03:11:54 PST
Steps to reproduce:

1. Launch Epiphany and enable caret navigation (if it is not already enabled) by pressing F7.

2. Load a page with both combo boxes and list boxes (e.g. https://bugs.webkit.org/enter_bug.cgi?product=WebKit).

3. Tab to the list box, arrow Up/Down to change the selection. Then press Left/Right Arrow to exit the listbox.

4. Repeat step 3 but with a combo box.

Expected results: Arrowing Left/Right would move you out of both list boxes and combo boxes.

Actual results: Arrowing Left/Right does move you out of the list box; however, pressing Left and Right in the combo box changes the selection just as Up and Down do.

Impact: Users who are blind and accessing content by caret navigation can get "stuck" in a focused combo box. To get unstuck, they must then move focus out of the form field (e.g. by Tabbing to a nearby link). Finally, to continue reading the content, they must backtrack to the combo box and then arrow around it. This is especially inefficient. And since combo boxes are quite similar to list boxes.... It would be beyond awesome if WebKitGtk's native caret navigation could handle this case as well.

BTW: Placing in component Accessibility now for Mario's consideration/triage and reassignment to the more appropriate component.
------- Comment #1 From 2013-02-26 11:13:12 PST -------
Created an attachment (id=190329) [details]
Patch
------- Comment #2 From 2013-02-27 03:38:17 PST -------
The patch passes the EWS bots - any chance of a review?
------- Comment #3 From 2013-02-27 08:23:45 PST -------
(From update of attachment 190329 [details])
In the listBox handler, presumably the same code looks like this

 if (isSpatialNavigationEnabled(document()->frame()))
            // Check if the selection moves to the boundary.
            if (keyIdentifier == "Left" || keyIdentifier == "Right" || ((keyIdentifier == "Down" || keyIdentifier == "Up") && endIndex == m_activeSelectionEndIndex))
                return;

is SpatialNavigation the same things here? if so it seems that should be used
also is the case where up down is on the last item handled the same way for menuLists?
------- Comment #4 From 2013-02-27 09:21:33 PST -------
Good point - I hadn't thought of down/up navigation at the ends.  I'll make the change and upload a new patch.
------- Comment #5 From 2013-02-28 03:03:21 PST -------
I've been testing the behaviour of listBox at the ends (under Caret browsing) and its not the same as the spatial navigation (i.e. it doesn't go through the spatial navigation codepath).  

So with my patch as is, the behaviour of caret browsing is the same for listBoxes and menuLists: left and right keys will exit the box, and up and down will change the selected item.

Is there is a requirement to exit the listBox/menuList with up/down at the ends under caret browsing? I'm not sure that it would improve navigation, and its likely to be a lot more complicated than the current proposal.
------- Comment #6 From 2013-03-04 07:54:21 PST -------
Any chance of another review?
------- Comment #7 From 2013-03-04 08:47:39 PST -------
(From update of attachment 190329 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=190329&action=review

can you add a layout test for this (r- for the no layout test)

> Source/WebCore/html/HTMLSelectElement.cpp:1126
>          const String& keyIdentifier = static_cast<KeyboardEvent*>(event)->keyIdentifier();

can you move this keyIdentifier call up before your snippet of code, then that line won't have to be duplicated
------- Comment #8 From 2013-04-09 10:04:00 PST -------
Created an attachment (id=197139) [details]
Patch
------- Comment #9 From 2013-04-09 20:43:31 PST -------
(From update of attachment 197139 [details])
Attachment 197139 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17689046

New failing tests:
fast/repaint/japanese-rl-selection-repaint-in-regions.html
------- Comment #10 From 2013-04-09 20:43:33 PST -------
Created an attachment (id=197198) [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
------- Comment #11 From 2013-04-10 01:46:22 PST -------
I've done some investigation and the mac-wk2 layout test failures are completely unrelated to this patch.
------- Comment #12 From 2013-04-10 03:39:48 PST -------
(From update of attachment 197139 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=197139&action=review

This patch looks good to me from a GTK-a11y perspective.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1487
> -    window = gtk_window_new(GTK_WINDOW_POPUP);
> +    window = gtk_window_new(GTK_WINDOW_TOPLEVEL);

The issue behind this change seems to be related only to accessibility stuff (properly detecting focus change for the containing window through a11y related stuff), so it sounds good to me as long as it didn't break any other tests. 

And as far as I know, Brian thoroughly make sure of that, so +1 from me.

> LayoutTests/platform/gtk/accessibility/aria-slider-required-attributes-expected.txt:-2
> -Accessibility object emitted "focus-event = 1" / Name: "(No name)" / Role: 50
> -Accessibility object emitted "state-change:focused = 1" / Name: "(No name)" / Role: 50

It makes sense this events won't ever show up in the results after the POPUP -> TOP_LEVEL change in DRT. Additionally, removing these two lines from expectations won't change at all the purpose of this test, since what it's interesting to test on it is in the lines below.

Thus, I'm fine with this change too.

> LayoutTests/platform/gtk/accessibility/combo-box-collapsed-selection-changed-expected.txt:-2
> -Accessibility object emitted "focus-event = 1" / Name: "foo" / Role: 11
> -Accessibility object emitted "state-change:focused = 1" / Name: "foo" / Role: 11

I'm fine with this too. See my previous comment about aria-slider-required-attributes-expected.txt
------- Comment #13 From 2013-04-11 07:59:30 PST -------
Any chance of a review?
------- Comment #14 From 2013-04-11 09:00:38 PST -------
(From update of attachment 197139 [details])
r+
------- Comment #15 From 2013-04-11 09:01:07 PST -------
do you need commit queue?
------- Comment #16 From 2013-04-11 09:04:19 PST -------
(In reply to comment #15)
> do you need commit queue?

Yes please. Thanks very much
------- Comment #17 From 2013-04-11 09:37:05 PST -------
(From update of attachment 197139 [details])
Clearing flags on attachment: 197139

Committed r148210: <http://trac.webkit.org/changeset/148210>
------- Comment #18 From 2013-04-11 09:37:08 PST -------
All reviewed patches have been landed.  Closing bug.