The default key event handler for HTMLSelectElement (HTMLSelectElement::listBoxDefaultEventHandler) attempts to handle the space key with spatial navigation like so: } else if (m_multiple && keyCode == ' ' && isSpatialNavigationEnabled(document()->frame())) { // Use space to toggle selection change. m_activeSelectionState = !m_activeSelectionState; updateSelectedState(listToOptionIndex(m_activeSelectionEndIndex), true /*multi*/, false /*shift*/); listBoxOnChange(); event->setDefaultHandled(); } This would likely fail in a <select multiple> with <optgroup>s because • m_activeSelectionEndIndex is a listIndex • updateSelectedState expects a listIndex to be passed in • the handler above is converting the listIndex to an optionIndex! This would cause issues if there are optgroups Proposed fix would be to just remove the listToOptionIndex conversion. Test page would be something like this: <select multiple> <optgroup><option>1</option><option>2</option></optgroup> <optgroup><option>3</option><option>4</option></optgroup> </select>
There is an existing spatial navigation and <select> test: LayoutTests/fast/events/spatial-navigation/snav-multiple-select.html Let me see if I can write a test for this (first time ever seeing spatial navigation code).
Apparently the spatial navigation (up / down) is affected by <optgroup>s? In my basic test (using platform/mac DumpRenderTree) I wasn't able to cause an issue with space key handling. In any case I think this sounds like it might be worth investigation by someone more familiar with spatial navigation who can test it more readily. I'll attached my untested patch if someone wants to continue it.
Created attachment 169063 [details] [PATCH] Untested Patch - Proposed Change Attaching the untested patch.
Hi Joseph, Can you please attach the demo html page with use case you were explaining ?
(In reply to comment #4) > Hi Joseph, > Can you please attach the demo html page with use case you were explaining ? Everything is explained in the comments above. I could not create / test a page, but comment #1 contains what I thought would be a test.
Created attachment 202658 [details] Patch attached contain fix for bug and layout test to verify changes.(r150435) Patch attached ensures that- - User should be able to navigate <select> element with <optgroup> inside it. - Multiple selection inside <select> element. - Avoided crashes which used to happen when user click on first <option> element to select it after some random navigation. Revision used : r150435
Created attachment 206469 [details] Updated patch-1 Please find updated patch.
Comment on attachment 206469 [details] Updated patch-1 View in context: https://bugs.webkit.org/attachment.cgi?id=206469&action=review Thanks for taking a look. This looks good to me! Let me know if you need me to cq+ > LayoutTests/fast/spatial-navigation/snav-multiple-select-optgroup.html:55 > + shouldBe("gFocusedDocument.getElementById(\"start\").options[0].selected", "false"); > + shouldBe("gFocusedDocument.getElementById(\"start\").options[1].selected", "false"); > + shouldBe("gFocusedDocument.getElementById(\"start\").options[2].selected", "false"); > + shouldBe("gFocusedDocument.getElementById(\"start\").options[3].selected", "false"); Seems like you could make a helper function that does this. You could then simplify the test and reduce the noise / boilerplate. For example you could have something like: sendKeyAndCheckOptions("downArrow", false, false, false, false); // Move to 2nd item. sendKeyAndCheckOptions(" ", false, true, false, false); // Select 2nd item sendKeyAndCheckOptions("downArrow", false, true, false, false); // Move to 4th item (3rd disabled) Or, something like resultMap, where you have the event => results formatted nicely together. But either way, what you have looks like it tests the right things.
Comment on attachment 206469 [details] Updated patch-1 View in context: https://bugs.webkit.org/attachment.cgi?id=206469&action=review > Source/WebCore/ChangeLog:7 > + Moreover,code update current active index of Select Element so that confusion should not happen between listIndex and optionIndex. Typo: Space after the comma. Grammar: "code update current active". Its unclear what you mean by this, and the entire sentence is unclear. You should update the sentence to explain what the code is doing.
Created attachment 207132 [details] Updated patch-2 Patch is updated as per your comments. If you find changes appropriate please go for cq+. Thanks :)
(In reply to comment #10) > Created an attachment (id=207132) [details] > Updated patch-2 > > Patch is updated as per your comments. > If you find changes appropriate please go for cq+. > Thanks :) Note, you shouldn't set the reviewer as yourself on your own patches. The Commit Queue would likely reject that, especially if you aren't a reviewer. I'll take a look at the updated patch!
Comment on attachment 207132 [details] Updated patch-2 View in context: https://bugs.webkit.org/attachment.cgi?id=207132&action=review Looks good. I can cq+ this one. Thanks. > LayoutTests/fast/spatial-navigation/snav-multiple-select-optgroup.html:49 > + function sendKeyAndCheckOptions(option0, option1, option2, option3) Heh, this function doesn't actually "sendKey", it just checks options. > Source/WebCore/ChangeLog:6 > + HTMLSelect Element inherently contains the logic to focus option node and thus, implementing an explicit logic to find the focus index is not required. Grammar: "implementing an explicit" => "implementing explicit"
Comment on attachment 207132 [details] Updated patch-2 Clearing flags on attachment: 207132 Committed r152919: <http://trac.webkit.org/changeset/152919>
Comment on attachment 206469 [details] Updated patch-1 Cleared Joseph Pecoraro's review+ from obsolete attachment 206469 [details] so that this bug does not appear in http://webkit.org/pending-commit.
(In reply to comment #14) > (From update of attachment 206469 [details]) > Cleared Joseph Pecoraro's review+ from obsolete attachment 206469 [details] so that this bug does not appear in http://webkit.org/pending-commit. I think you may have been able to keep the review+ marker, and instead move the bug to resolved / fixed. I personally find the r/cq flag useful documentation and easy to see at a glance what happened. I dislike when they are removed, that means I might have to read through all the comments to see what happened.
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 206469 [details] [details]) > > Cleared Joseph Pecoraro's review+ from obsolete attachment 206469 [details] [details] so that this bug does not appear in http://webkit.org/pending-commit. > > I think you may have been able to keep the review+ marker, and instead move the bug to resolved / fixed. I personally find the r/cq flag useful documentation and easy to see at a glance what happened. I dislike when they are removed, that means I might have to read through all the comments to see what happened. You can always check the history of the flags, for example: https://bugs.webkit.org/show_activity.cgi?id=99525 Tools/Scripts/webit-patch tool always remove flags when you (or commit queue) land a patch. And it removes flags and obsolete old attachments if you upload a new patch to a bug. The reason is not to have obsolete and/or already landed patches in http://webkit.org/pending-commit and http://webkit.org/pending-review webkit-patch has two commands to maintain these flags, I just ran them: - clean-pending-commit Clear r+ on obsolete patches so they do not appear in the pending-commit list. - clean-review-queue Clear r? on obsolete patches so they do not appear in the pending-review list.