Bug 48937 – Spatial Navigation: Add support for <select> element in single selection mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Chang Shu]

This case handles the single selection mode only.

[Chang Shu]

Created attachment 72847 [details]
fix patch

[Antonio Gomes]

Comment on attachment 72847 [details]
fix patch

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.js:58
> -    return;
> +    direction = gExpectedResults[gIndex][0];
>    }

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

[Chang Shu]

Created attachment 72941 [details]
fix patch 2

[Antonio Gomes]

Comment on attachment 72941 [details]
fix patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=72941&action=review

Looks good. r=me with my comments addressed.

> WebCore/dom/SelectElement.cpp:572
> +        if (isSpatialNavigationEnabled(element))

lets do it as

if (isSpatialNavigationEnabled(frame->settings()))

then in the function you do:

bool isSpatialNavigationenabled(Settings* settings)
{
  if (!setting)
     return false;

  return settings->isSpatialNavigationEnabled();
}

> WebCore/dom/SelectElement.cpp:777
> -        if (Frame* frame = element->document()->frame()) {
> -            if (frame->settings() && frame->settings()->isSpatialNavigationEnabled()) {
> -                // Check if the selection moves to the boundary.
> -                if (keyIdentifier == "Left" || keyIdentifier == "Right" || ((keyIdentifier == "Down" || keyIdentifier == "Up") && endIndex == data.activeSelectionEndIndex()))
> -                    return;
> -            }
> -        }
> +        if (isSpatialNavigationEnabled(element))

same here:

id (Frame* frame = element->document()->frame())
   if (isSpatialNavigationEnabled(frame->settings()))

so we would change only only of code here.

> LayoutTests/fast/events/spatial-navigation/resources/spatial-navigation-utils.js:63
> +  if (window.layoutTestController && direction != null)

only "&& direction" does not work?

[Chang Shu]

Created attachment 73092 [details]
fix patch 3

[Antonio Gomes]

Comment on attachment 73092 [details]
fix patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=73092&action=review

> WebCore/dom/SelectElement.cpp:75
> +static bool isSpatialNavigationEnabled(const Frame* frame)
> +{

Could you please file a bug to add a "static bool SpatialNavigation::isEnabled(Settings*)" method in spatialNavigation.h

they each file that touches it will have to have its own copy of this method. It is in /editing already.

If we do that, it should probably take a Settings* and that we should do the early return logic even for such small function, as I described in the previous comment.

[Chang Shu]

(In reply to comment #6)
> (From update of attachment 73092 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73092&action=review
> 
> > WebCore/dom/SelectElement.cpp:75
> > +static bool isSpatialNavigationEnabled(const Frame* frame)
> > +{
> 
> Could you please file a bug to add a "static bool SpatialNavigation::isEnabled(Settings*)" method in spatialNavigation.h
> 
> they each file that touches it will have to have its own copy of this method. It is in /editing already.
> 
> If we do that, it should probably take a Settings* and that we should do the early return logic even for such small function, as I described in the previous comment.

Makes sense, will do.

[Antonio Gomes]

> they each file that touches it will have to have its own copy of this method. It is in /editing already.

in english now: "then files that want to know about spatial navigation status do not need to have their own copy of this method. 

e.g. this method is also in /editing"

[WebKit Commit Bot]

Comment on attachment 73092 [details]
fix patch 3

Clearing flags on attachment: 73092

Committed r71442: <http://trac.webkit.org/changeset/71442>

[WebKit Commit Bot]

Comment on attachment 73092 [details]
fix patch 3

Rejecting patch 73092 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 73092]" exit_code: 1
Last 500 characters of output:
ts/CommitQueue/WebKitTools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_html.py", line 546, in __getattr__
    self.forms()
  File "/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_html.py", line 559, in forms
    self._forms_factory.forms())
  File "/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_html.py", line 228, in forms
    raise ParseError(exc)
webkitpy.thirdparty.autoinstalled.mechanize._html.ParseError

Full output: http://queues.webkit.org/results/5252020

[Chang Shu]

(In reply to comment #10)
> (From update of attachment 73092 [details])
> Rejecting patch 73092 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 73092]" exit_code: 1
> Last 500 characters of output:
> ts/CommitQueue/WebKitTools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_html.py", line 546, in __getattr__
>     self.forms()
>   File "/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_html.py", line 559, in forms
>     self._forms_factory.forms())
>   File "/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_html.py", line 228, in forms
>     raise ParseError(exc)
> webkitpy.thirdparty.autoinstalled.mechanize._html.ParseError
> 
> Full output: http://queues.webkit.org/results/5252020

I guess the script is broken. can anyone take a look?

[Ryosuke Niwa]

This test has been failing on gtk.  Please fix.