Bug 99525 – Spatial Navigation handling of space key in <select> appears to confuse listIndex and optionIndex

• Component: Forms
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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>

[Joseph Pecoraro]

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).

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 169063 [details]
[PATCH] Untested Patch - Proposed Change

Attaching the untested patch.

[Abhijeet Kandalkar]

Hi Joseph,
Can you please attach the demo html page with use case you were explaining ?

[Joseph Pecoraro]

(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.

[Abhijeet Kandalkar]

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

[Abhijeet Kandalkar]

Created attachment 206469 [details]
Updated patch-1

Please find updated patch.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Abhijeet Kandalkar]

Created attachment 207132 [details]
Updated patch-2

Patch is updated as per your comments.
If you find changes appropriate please go for cq+.
Thanks :)

[Joseph Pecoraro]

(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!

[Joseph Pecoraro]

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"

[WebKit Commit Bot]

Comment on attachment 207132 [details]
Updated patch-2

Clearing flags on attachment: 207132

Committed r152919: <http://trac.webkit.org/changeset/152919>

[Csaba Osztrogonác]

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.

[Joseph Pecoraro]

(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.

[Csaba Osztrogonác]

(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.