RESOLVED FIXED 49261
Spatial Navigation: Add support for <select> element in multiple selection mode 
https://bugs.webkit.org/show_bug.cgi?id=49261
Summary Spatial Navigation: Add support for <select> element in multiple selection mode
Chang Shu
Reported 2010-11-09 10:06:50 PST
Find a way to make multiple selections when spatial navigation is enabled. Right now, up and down keys always changes selection and no way to select multiple items either.
Attachments
fix patch (10.67 KB, patch)
2011-02-07 11:19 PST, Chang Shu 		no flags
DetailsFormatted DiffDiff
fix patch 2 (12.90 KB, patch)
2011-02-07 14:43 PST, Chang Shu 		no flags
DetailsFormatted DiffDiff
fix patch 3 (26.05 KB, patch)
2011-02-08 11:16 PST, Chang Shu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chang Shu
Comment 1 2011-02-07 11:19:32 PST
Created attachment 81498 [details] fix patch
Chang Shu
Comment 2 2011-02-07 11:21:31 PST
Forgot to add cgarcia, who implemented addFocusRingRects, to the ChangeLog. :) I will do it in the 2nd attempt.
Antonio Gomes
Comment 3 2011-02-07 12:57:07 PST
Comment on attachment 81498 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=81498&action=review looks ok to me! some minor nits ... could you please add tests with disabled <selects>? > Source/WebCore/ChangeLog:9 > + Nit: you are missing the bug title. > Source/WebCore/dom/SelectElement.cpp:822 > + updateSelectedState(data, element, listToOptionIndex(data, element, data.activeSelectionEndIndex()), true, false); I would hadadded /**/-like comments for the bools true /*shift*/, false /*foo*/ > LayoutTests/ChangeLog:5 > + Enhanced the test for testing multiple selection. nit: generally such comment is explaining the change. It should go below the bugzilla entry, which should come below the bug title.
Chang Shu
Comment 4 2011-02-07 14:43:35 PST
Created attachment 81524 [details] fix patch 2
Antonio Gomes
Comment 5 2011-02-08 07:00:49 PST
Comment on attachment 81524 [details] fix patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=81524&action=review LGTM. As discussed on IRC, cshu will add a few more requested tests. > Source/WebCore/dom/SelectElement.cpp:822 > + updateSelectedState(data, element, listToOptionIndex(data, element, data.activeSelectionEndIndex()), true /*multi*/, false /*shift*/); do we want to force multi here? even if it is disabled in the html?
Chang Shu
Comment 6 2011-02-08 08:53:27 PST
> > Source/WebCore/dom/SelectElement.cpp:822 > > + updateSelectedState(data, element, listToOptionIndex(data, element, data.activeSelectionEndIndex()), true /*multi*/, false /*shift*/); > > do we want to force multi here? even if it is disabled in the html? As discussed on IRC, we found a couple of remaining issues to be addressed: 1. support select with shiftkey with sp enabled. 2. fix the multiple=false case 3. add tests for disable <select> I will work on them in the follow-up patches.
Chang Shu
Comment 7 2011-02-08 11:16:47 PST
Created attachment 81668 [details] fix patch 3
WebKit Commit Bot
Comment 8 2011-02-26 00:12:40 PST
Comment on attachment 81668 [details] fix patch 3 Clearing flags on attachment: 81668 Committed r79762: <http://trac.webkit.org/changeset/79762>
Note You need to log in before you can comment on or make changes to this bug.