[webkit-reviews] review denied: [Bug 48937] Spatial Navigation: Add support for <select> element in single selection mode : [Attachment 72847] fix patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Chang Shu
<Chang.Shu at nokia.com>'s request for review:
Bug 48937: Spatial Navigation: Add support for <select> element in single
selection mode
https://bugs.webkit.org/show_bug.cgi?id=48937

Attachment 72847: fix patch
https://bugs.webkit.org/attachment.cgi?id=72847&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72847&action=review

Looks generally good. Comments below.

> WebCore/dom/SelectElement.cpp:236
> +bool SelectElement::isSpatialNavigation(const Element* element)

Should not be a class method, but a static helper.

Name is also not so good.

> WebCore/dom/SelectElement.cpp:655
> +	   } else if (keyCode == ' ' && isSpatialNavigation(element)) {

What about ENTER?

>
LayoutTests/fast/events/spatial-navigation/resources/spatial-navigation-utils.j
s:58
> -    return;
> +    direction = gExpectedResults[gIndex][0];
>    }

Lets also do "case " ":" and keep the default no-op. otherwise anything could
get here.