Bug 49261 - Spatial Navigation: Add support for <select> element in multiple selection mode
Summary: Spatial Navigation: Add support for <select> element in multiple selection mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on: 53928
Blocks: 47094
  Show dependency tree / graph
 
Reported: 2010-11-09 10:06 PST by Chang Shu
Modified: 2011-03-01 10:23 PST (History)
3 users (show)

See Also:

Attachments
fix patch (10.67 KB, patch)
2011-02-07 11:19 PST, Chang Shu 		no flags Details | Formatted Diff | Diff
fix patch 2 (12.90 KB, patch)
2011-02-07 14:43 PST, Chang Shu 		no flags Details | Formatted Diff | Diff
fix patch 3 (26.05 KB, patch)
2011-02-08 11:16 PST, Chang Shu 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 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.
Comment 1 Chang Shu 2011-02-07 11:19:32 PST 
Created attachment 81498 [details]
fix patch
Comment 2 Chang Shu 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.
Comment 3 Antonio Gomes 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.
Comment 4 Chang Shu 2011-02-07 14:43:35 PST 
Created attachment 81524 [details]
fix patch 2
Comment 5 Antonio Gomes 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?
Comment 6 Chang Shu 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.
Comment 7 Chang Shu 2011-02-08 11:16:47 PST 
Created attachment 81668 [details]
fix patch 3
Comment 8 WebKit Commit Bot 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>