Bug 49261

Summary: Spatial Navigation: Add support for <select> element in multiple selection mode
Product: WebKit Reporter: Chang Shu <cshu>
Component: FormsAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, tonikitoo, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 53928    
Bug Blocks: 47094    
Attachments:
Description Flags
fix patch
none
fix patch 2
none
fix patch 3 none

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.