Summary: | [Gtk] Combo boxes should be arrow-out-of-able similar to list boxes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joanmarie Diggs <jdiggs> | ||||||||
Component: | Accessibility | Assignee: | Brian Holt <brian.holt> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, commit-queue, dmazzoni, esprehn+autocc, haraken, mario, mifenton, ojan.autocc, rniwa, tkent, webkit.review.bot, zan | ||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 25531 | ||||||||||
Attachments: |
|
Description
Joanmarie Diggs
2012-01-22 03:11:54 PST
Created attachment 190329 [details]
Patch
The patch passes the EWS bots - any chance of a review? Comment on attachment 190329 [details]
Patch
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?
Good point - I hadn't thought of down/up navigation at the ends. I'll make the change and upload a new patch. 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. Any chance of another review? Comment on attachment 190329 [details] Patch 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 Created attachment 197139 [details]
Patch
Comment on attachment 197139 [details] Patch 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 Created attachment 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
I've done some investigation and the mac-wk2 layout test failures are completely unrelated to this patch. Comment on attachment 197139 [details] Patch 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 Any chance of a review? Comment on attachment 197139 [details]
Patch
r+
do you need commit queue? (In reply to comment #15) > do you need commit queue? Yes please. Thanks very much Comment on attachment 197139 [details] Patch Clearing flags on attachment: 197139 Committed r148210: <http://trac.webkit.org/changeset/148210> All reviewed patches have been landed. Closing bug. |