Summary: | [Qt] Fix caret browsing navigation mode | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antonio Gomes <tonikitoo> | ||||||
Component: | Accessibility | Assignee: | Antonio Gomes <tonikitoo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, eric, hausmann, kenneth, kling, mario, rniwa, webkit-ews, webkit.review.bot, yael | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | 51235 | ||||||||
Bug Blocks: | 50942, 47271 | ||||||||
Attachments: |
|
Description
Antonio Gomes
2010-12-05 07:12:25 PST
(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 |