Bug 48937

Created attachment 72847 [details] fix patch

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.

Created attachment 72941 [details] fix patch 2

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?

Created attachment 73092 [details] fix patch 3

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.

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

> 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"

Comment on attachment 73092 [details] fix patch 3 Clearing flags on attachment: 73092 Committed r71442: <http://trac.webkit.org/changeset/71442>

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

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

This test has been failing on gtk. Please fix.