Bug 50536

(In reply to comment #0) > After bug 47426 got broken in Qt "After bug 47426, caret browsing got broken in Qt..."

Created attachment 75629 [details] patch v1

Attachment 75629 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/Skipped', u'LayoutTests/platform/win/Skipped', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/EditorClientQt.cpp']" exit_code: 1 WebKit/qt/WebCoreSupport/EditorClientQt.cpp:471: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75629 [details] did not build on qt: Build output: http://queues.webkit.org/results/6771071

Comment on attachment 75629 [details] patch v1 r- to fix the build. I can't find any occurance of isCaretBrowsingEnabled() in WebKit trunk.

Created attachment 75677 [details] (committed r74229, r=ariya) patch v2 - it builds and has no style issues.

Comment on attachment 75677 [details] (committed r74229, r=ariya) patch v2 - it builds and has no style issues. Looks good to me, sorry for the regression bug 42476 caused.

Just to make reviewing simpler, patch is simple. Summary: - it makes an already existing gtk-specific test to be a generic test, but skips it for Mac and Win, since these DRTs do not support testing caret browsing. - It reintroduces some of the code removed in bug 47426, since in that bug we missed the caret browsing mode tests. I am actually fixing this because it blocks fixing a conflicts between Spatial Navigation and Caret Browsing features. Easier now anyone? :)

Comment on attachment 75677 [details] (committed r74229, r=ariya) patch v2 - it builds and has no style issues. View in context: https://bugs.webkit.org/attachment.cgi?id=75677&action=review > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:479 > + if (kevent->shiftKey() && kevent->ctrlKey()) > + frame->editor()->command("MoveWordBackwardAndModifySelection").execute(); > + else if (kevent->shiftKey()) > + frame->editor()->command("MoveLeftAndModifySelection").execute(); > + else if (kevent->ctrlKey()) > + frame->editor()->command("MoveWordBackward").execute(); > + else > + frame->editor()->command("MoveLeft").execute(); Isn't better to directly call selection controller's modify?

(In reply to comment #9) > (From update of attachment 75677 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75677&action=review > > > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:479 > > + if (kevent->shiftKey() && kevent->ctrlKey()) > > + frame->editor()->command("MoveWordBackwardAndModifySelection").execute(); > > + else if (kevent->shiftKey()) > > + frame->editor()->command("MoveLeftAndModifySelection").execute(); > > + else if (kevent->ctrlKey()) > > + frame->editor()->command("MoveWordBackward").execute(); > > + else > > + frame->editor()->command("MoveLeft").execute(); > > Isn't better to directly call selection controller's modify? In fact, it could be better. However, since the whole method is already full on frame->editor->command calls, I left these like that too. I actually also plan to fix it to work like Gtk and EFL ports: a map table of actions and commands. I will be a follow up. ps: please comment #8 should make it an easy review :)

@Ryosuke / Yael: any other issues, you guys see here? I would really like to move one here, since it bo

[Hit ENTER by accident] > I would really like to move one here, since it bo ... blocks some other bugs.

Comment on attachment 75677 [details] (committed r74229, r=ariya) patch v2 - it builds and has no style issues. View in context: https://bugs.webkit.org/attachment.cgi?id=75677&action=review LGTM. > LayoutTests/editing/selection/caret-mode-paragraph-keys-navigation-expected.txt:1 > +This tests that arrow keys navigatie through a paragraph as expected when in caret browsing mode, also with shift and ctrl modifiers. "navigate"

Comment on attachment 75677 [details] (committed r74229, r=ariya) patch v2 - it builds and has no style issues. Clearing flags on attachment: 75677 Committed r74229: <http://trac.webkit.org/changeset/74229>

Reverted r74229 for reason: Broken on Snow Leopard and possibly other platforms Committed r74235: <http://trac.webkit.org/changeset/74235>

(In reply to comment #15) > Reverted r74229 for reason: > > Broken on Snow Leopard and possibly other platforms > > Committed r74235: <http://trac.webkit.org/changeset/74235> Eric, thanks for taking care of it. Actually I realized my mistake seconds after the initial checkin, and created a patch to fix it. See bug 51232 ... Since it was > 1:00am I CQ+'ed it, but it seems the CQ got stuck and your rollout came first. I am re land it with the proper fix.

http://trac.webkit.org/changeset/74270 might have broken GTK Linux 64-bit Debug

(In reply to comment #17) > http://trac.webkit.org/changeset/74270 might have broken GTK Linux 64-bit Debug Fake alarm. > I am re land it with the proper fix. http://trac.webkit.org/changeset/74270