[webkit-reviews] review granted: [Bug 99525] Spatial Navigation handling of space key in <select> appears to confuse listIndex and optionIndex : [Attachment 206469] Updated patch-1

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Abhijeet Kandalkar
<kandalkar.abhijeet58 at gmail.com>'s request for review:
Bug 99525: Spatial Navigation handling of space key in <select> appears to
confuse listIndex and optionIndex
https://bugs.webkit.org/show_bug.cgi?id=99525

Attachment 206469: Updated patch-1
https://bugs.webkit.org/attachment.cgi?id=206469&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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.