Bug 37153

take over the unfinished work.

(In reply to comment #1) > take over the unfinished work. As discussed by email, patch is funcional as it (probably needs rebase against tot now), but we are lacking tests for it. bug 45375 has a bunch of re-usable stuff for this testing. There are also currently 16 spatial nav tests in fast/events/spatial-navigation for reference. What the patch does not do is properly handle cases when the input field has Javascript handles attached to it, handling arrow keys, but it can come later.

Created attachment 69239 [details] fix patch I rebaselined Antonio's original patch and added layout tests for it.

Mn... it seems like this bug can also be reproduced if the entire table was in a content editable area. Copy & paste the following HTML in the HTML source mode at http://www.mozilla.org/editor/midasdemo/ <table style="text-align: left; width: 100%; margin-left: auto; margin-right: auto;" border="1" cellpadding="2" cellspacing="1"> <tbody> <tr> <td style="vertical-align: top; text-align: center;"></td> <td style="vertical-align: top; text-align: center;"><a id="2" href="a">2</a></td> <td style="vertical-align: top; text-align: center;"></td> </tr> <tr> <td style="vertical-align: top; text-align: center;"><a id="4" href="a">4</a></td> <td style="vertical-align: top; text-align: center;"><textarea id="start" rows="5" cols="5">line1 line2 line3</textarea> </td><td style="vertical-align: top; text-align: center;"><a id="6" href="a">6</a></td> </tr> <tr> <td style="vertical-align: top; text-align: center;"></td> <td style="vertical-align: top; text-align: center;"><a id="8" href="a">8</a></td> <td style="vertical-align: top; text-align: center;"></td> </tr> </tbody> </table> You can move the carets around textarea but you can't move it inside the textarea. And once you move the caret inside the textarea by clicking it, you can't move it out.

(In reply to comment #4) > Mn... it seems like this bug can also be reproduced if the entire table was in a content editable area. Copy & paste the following HTML in the HTML source mode at http://www.mozilla.org/editor/midasdemo/ > > <table style="text-align: left; width: 100%; margin-left: auto; margin-right: auto;" border="1" cellpadding="2" cellspacing="1"> <tbody> <tr> <td style="vertical-align: top; text-align: center;"></td> <td style="vertical-align: top; text-align: center;"><a id="2" href="a">2</a></td> <td style="vertical-align: top; text-align: center;"></td> </tr> <tr> <td style="vertical-align: top; text-align: center;"><a id="4" href="a">4</a></td> <td style="vertical-align: top; text-align: center;"><textarea id="start" rows="5" cols="5">line1 line2 line3</textarea> </td><td style="vertical-align: top; text-align: center;"><a id="6" href="a">6</a></td> </tr> <tr> <td style="vertical-align: top; text-align: center;"></td> <td style="vertical-align: top; text-align: center;"><a id="8" href="a">8</a></td> <td style="vertical-align: top; text-align: center;"></td> </tr> </tbody> </table> > > You can move the carets around textarea but you can't move it inside the textarea. And once you move the caret inside the textarea by clicking it, you can't move it out. I will not work as the code is today. Basically we are ignoring contentEditable elements, but it is in the TODO list. Would be great to see how Opera's Spatial Navigation handles this case.

Comment on attachment 69239 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=69239&action=review I feel that this patch has many fixes at wrong layers / levels. In the future, I also want to better isolate some of spatial navigation related code. > WebCore/editing/EditorCommand.cpp:100 > +// For some execute_XXCommandXX routines below, 'true' is returned regardless if the > +// 'command' in case results in visual update for the user or not. For most of the caller I don't really understand this comment. > WebCore/editing/EditorCommand.cpp:101 > +// methods, this returned value is used as the criterio for either marking the originating typo: criteria or criterion > WebCore/editing/EditorCommand.cpp:623 > - frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionBackward, CharacterGranularity, true); > - return true; > + bool retVal = frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionBackward, CharacterGranularity, true); > + return (spatialNavigationEnabled(frame)) ? retVal : true; I don't think it's right to change the return value based on preference here. We should do that inside modify but I do feel that fixes to modify is wrong too because we expose so much low-level details about events there. > WebCore/editing/SelectionController.cpp:656 > + } else > position = modifyExtendingRight(granularity); It seems weird to me that we don't update posBeforeModifying when alter != AlternationMove. It'll be very confusing if we started using this variable for other purposes later. I'd suggest to either always update posBeforeModifying or rename posBeforeModifying to something else like originalCaretPosition. > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:379 > - m_page->triggerAction(action); > + const char* command = QWebPagePrivate::editorCommandForWebActions(action); > + if (command) { > + if (frame->editor()->command(command).execute()) > + event->setDefaultHandled(); > + return; > + } It looks strange to me that we set the default handled here without checking the value of isSpatialNavigationEnabled. Do we assume that spatial navigation is always enabled? Please correct me since I'm new to the whole spatial navigation code but doesn't making such an assumption break DRT on Qt?

Antonio, can you follow up Niwa's review? Thanks!

Created attachment 71034 [details] fix patch 2

Hi, Niwa, Thanks much for your review. I submitted another patch based on your comments. Here are some highlights: 1. I tried to make less impact on the code. So I moved the logic inside SelectionController to EditorCommand. I agree that avoiding making lower level code change is preferred. 2. The return value in EditorCommand functions is still partially based on the spatialnavigation flag. I think this is ok because the editor wants to absorb the key events when spatial navigation is disabled, and not absorb them vice versa. 3. In EditorClientQt.cpp, I fixed the original patch so that it will handle both cases where QT_NO_SHORTCUT is defined or not. It also checks the flag. Btw, the flag is not always enabled and is indeed disabled by default. Thanks, Chang > I feel that this patch has many fixes at wrong layers / levels. In the future, I also want to better isolate some of spatial navigation related code. > > > WebCore/editing/EditorCommand.cpp:100 > > +// For some execute_XXCommandXX routines below, 'true' is returned regardless if the > > +// 'command' in case results in visual update for the user or not. For most of the caller > > I don't really understand this comment. > > > WebCore/editing/EditorCommand.cpp:101 > > +// methods, this returned value is used as the criterio for either marking the originating > > typo: criteria or criterion > > > WebCore/editing/EditorCommand.cpp:623 > > - frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionBackward, CharacterGranularity, true); > > - return true; > > + bool retVal = frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionBackward, CharacterGranularity, true); > > + return (spatialNavigationEnabled(frame)) ? retVal : true; > > I don't think it's right to change the return value based on preference here. We should do that inside modify but I do feel that fixes to modify is wrong too because we expose so much low-level details about events there. > > > WebCore/editing/SelectionController.cpp:656 > > + } else > > position = modifyExtendingRight(granularity); > > It seems weird to me that we don't update posBeforeModifying when alter != AlternationMove. It'll be very confusing if we started using this variable for other purposes later. I'd suggest to either always update posBeforeModifying or rename posBeforeModifying to something else like originalCaretPosition. > > > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:379 > > - m_page->triggerAction(action); > > + const char* command = QWebPagePrivate::editorCommandForWebActions(action); > > + if (command) { > > + if (frame->editor()->command(command).execute()) > > + event->setDefaultHandled(); > > + return; > > + } > > It looks strange to me that we set the default handled here without checking the value of isSpatialNavigationEnabled. Do we assume that spatial navigation is always enabled? Please correct me since I'm new to the whole spatial navigation code but doesn't making such an assumption break DRT on Qt?

Comment on attachment 71034 [details] fix patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=71034&action=review > WebCore/editing/EditorCommand.cpp:627 > + Position endPosBeforeMove = frame->selection()->end(); > frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); > + if (doSpatialNavigation && endPosBeforeMove == frame->selection()->end()) > + return false; You probably need to make a helper function for doSpatialNavigation. But I still don't like the idea of modifying executeMove* functions. I don't think that's a correct place to implement these kind of changes. If I were you, I'd modify executeMoveDown to return the value returned by selection()->modify(), and then go change modify() to return the desired value. What I meant in my previous comment was it's odd to change the return value in executeMove*, not that you should move the code out to executeMove*. You should, in fact, change the return value and obtain the reference position inside modify. IMO, this function should look like: static bool executeMoveDown(Frame* frame, Event*, EditorCommandSource, const String&) { return frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); } Sorry, I should have been more clear about this. > LayoutTests/fast/events/spatial-navigation/snav-textarea.html:1 > +<html> you probably want to add <!DOCTYPE html> > LayoutTests/fast/events/spatial-navigation/snav-textarea.html:34 > + if (window.layoutTestController) { > + layoutTestController.dumpAsText(); > + layoutTestController.setSpatialNavigationEnabled(true); > + layoutTestController.overridePreference("WebKitTabToLinksPreferenceKey", 1); > + layoutTestController.waitUntilDone(); > + } Doesn't this test fail on non-Qt platforms since you didn't make changes to editing delegates in other platforms?

> You probably need to make a helper function for doSpatialNavigation. But I still don't like the idea of modifying executeMove* functions. I don't think that's a correct place to implement these kind of changes. If I were you, I'd modify executeMoveDown to return the value returned by selection()->modify(), and then go change modify() to return the desired value. What I meant in my previous comment was it's odd to change the return value in executeMove*, not that you should move the code out to executeMove*. You should, in fact, change the return value and obtain the reference position inside modify. IMO, this function should look like: > > static bool executeMoveDown(Frame* frame, Event*, EditorCommandSource, const String&) > { > return frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); > } > > Sorry, I should have been more clear about this. Great, I thought about that(just return the modify function), too. > > > LayoutTests/fast/events/spatial-navigation/snav-textarea.html:1 > > +<html> > > you probably want to add <!DOCTYPE html> Sure. > > Doesn't this test fail on non-Qt platforms since you didn't make changes to editing delegates in other platforms? It looks to me the other platforms, aka EditorClientXX.cpp, sets event->defaulthandled() based on the execute function. So it should automatically work. However, I don't have all the setup to verify this. I will make sure it works on Mac.

Created attachment 71203 [details] fix patch 3 Hi, Niwa, A new patch is attached. Can you comment? Thanks! Chang

Comment on attachment 71203 [details] fix patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=71203&action=review This patch is getting better and better. Few comments about the tests. > WebCore/editing/EditorCommand.cpp:623 > - frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); > - return true; > + return frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); Nice changes! > WebCore/editing/SelectionController.cpp:646 > + bool wasRange = m_selection.isRange(); > + Position originalStartPosition = m_selection.start(); I like this change much better than adding a whole bunch of changes inside the switch. > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:405 > + else { > + if (!frame->editor()->command("MoveLeft").execute()) > + return; > + } You can do: else if (!frame->editor()->command("MoveLeft").execute()) return; instead. > LayoutTests/fast/events/spatial-navigation/snav-input.html:10 > + This test ensures the correctness of Spatial Navigation (SNav) algorithm over input box. > + > + * Pre-conditions: > + 1) DRT support for SNav enable/disable. > + > + * Navigation steps: > + 1) Loads this page, focus goes to "start" automatically. > + 2) Focus moves away from input box in 4 different directions to neighbor nodes and back. It's nice to put this in the body so that we can see the description when we open the test on the browser unless doing so sabotages the test. > LayoutTests/fast/events/spatial-navigation/snav-input.html:14 > + <script src="../../js/resources/js-test-pre.js"></script> > + <script src="resources/spatial-navigation-utils.js"></script> It's quite unusual to include js-test-pre.js like this. We typically use that file for script-tests. Why are you including this? > LayoutTests/fast/events/spatial-navigation/snav-input.html:53 > + <script src="../js/resources/js-test-post.js"></script> Ditto. > LayoutTests/fast/events/spatial-navigation/snav-textarea.html:10 > + This test ensures the correctness of Spatial Navigation (SNav) algorithm over input box. > + > + * Pre-conditions: > + 1) DRT support for SNav enable/disable. > + > + * Navigation steps: > + 1) Loads this page, focus goes to "start" automatically. > + 2) Focus moves away from textarea in 4 different directions to neighbor nodes and back. Same comment about including this in body instead. > LayoutTests/fast/events/spatial-navigation/snav-textarea.html:13 > + <script src="../../js/resources/js-test-pre.js"></script> Same comment about including this js. > LayoutTests/fast/events/spatial-navigation/snav-textarea.html:53 > + <script src="../js/resources/js-test-post.js"></script> Ditto.

Thanks for the review! Just one comment about js-test-pre.js. Some functions defined there are used in spatial-navigation-utils.js. Removing it will hang the test. Any other way to write the html? > > LayoutTests/fast/events/spatial-navigation/snav-input.html:14 > > + <script src="../../js/resources/js-test-pre.js"></script> > > + <script src="resources/spatial-navigation-utils.js"></script> > > It's quite unusual to include js-test-pre.js like this. We typically use that file for script-tests. Why are you including this?

I do not see any problem in including them. they are included in all other spatial nav tests and many others.

Created attachment 71292 [details] (committed r71388, r=smfr) fix patch 4 Niwa, I updated patch to display descriptions in the tests. It seems we have to include the js-pre.js in order to use some functions there. Thanks!

Comment on attachment 71292 [details] (committed r71388, r=smfr) fix patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=71292&action=review > LayoutTests/fast/events/spatial-navigation/snav-input.html:35 > + description('This test ensures the correctness of Spatial Navigation (SNav) algorithm over input box.<br>\ > + * Pre-conditions:<br>\ > + 1) DRT support for SNav enable/disable.<br>\ > + * Navigation steps:<br>\ > + 1) Loads this page, focus goes to "start" automatically.<br>\ > + 2) Focus moves away from input box in 4 different directions to neighbor nodes and back.<br>'); You could just put this in <p>. > LayoutTests/fast/events/spatial-navigation/snav-input.html:70 > + window.onload = runTest; I don't understand why we need to wait for onload event. Maybe this is a quirk with js-test / spatial-navigation ?

Otherwise the patch LGTM. Antonio & Csaba: could you do the formal review?

(In reply to comment #18) > Otherwise the patch LGTM. Antonio & Csaba: could you do the formal review? Thanks rniwa. I am the original author, so I can not review it. I will test them o Mac and gtk tonight. Maybe Darin could you have a look (please :)?

I haven’t been reviewing spatial navigation patches before this one, and I don’t have as much time for patch review lately as I’d like. But I may be able to get to this later.

(In reply to comment #20) > I haven’t been reviewing spatial navigation patches before this one, and I don’t have as much time for patch review lately as I’d like. > > But I may be able to get to this later. Ping review?

Comment on attachment 71292 [details] (committed r71388, r=smfr) fix patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=71292&action=review > WebCore/ChangeLog:8 > + [Qt] Makes <input type=text> and <textarea> functional with These WebCore changes could potentially affect behavior on all platforms, so the Qt prefix here is not applicable. > WebCore/editing/EditorCommand.cpp:622 > - frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); > - return true; > + return frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); > } Did you run LayoutTests on Mac or Window with these changes?

> > WebCore/ChangeLog:8 > > + [Qt] Makes <input type=text> and <textarea> functional with > > These WebCore changes could potentially affect behavior on all platforms, so the Qt prefix here is not applicable. I will remove [Qt]. > > WebCore/editing/EditorCommand.cpp:622 > > - frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); > > - return true; > > + return frame->selection()->modify(SelectionController::AlterationMove, SelectionController::DirectionForward, LineGranularity, true); > > } > > Did you run LayoutTests on Mac or Window with these changes? The code change should have changed some behavior but yes, the laytout tests didn't seem to break on Mac.

Thanks so much for the review, Simon. This patch needs a rebaseline and I will submit a new patch soon.

> The code change should have changed some behavior but yes, the laytout tests didn't seem to break on Mac. No port makes use of the return value of this method, but Qt now. Also, I will land it manually to watch the bots.

Comment on attachment 71292 [details] (committed r71388, r=smfr) fix patch 4 Clearing flags on attachment: 71292 Committed r71388: <http://trac.webkit.org/changeset/71388>

Tests skipped in Gtk, and bug 49056 filed about it.

+ This patch does not support yet cases where the focused <input> + has a JS handler for the arrow keys. File bug 49068 about it.

Revision r71388 cherry-picked into qtwebkit-2.1 with commit 6ddf322 <http://gitorious.org/webkit/qtwebkit/commit/6ddf322>