Summary: | Option + arrow moves caret past whitespace on iOS | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, darin, ews-watchlist, koivisto, megan_gardner, mifenton, repstein, webkit-bug-importer, wenson_hsieh, zalan | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 201531 | ||||||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2019-09-06 22:10:53 PDT
Created attachment 378278 [details]
Fixes the bug
Created attachment 378279 [details]
Fixed non-iOS builds
Created attachment 378282 [details]
GTK build fix attempt
Created attachment 378283 [details]
Another GTK build fix attempt
Created attachment 378284 [details]
Yet another GTK build fix attempt
Created attachment 378399 [details]
Fixed build more
Comment on attachment 378399 [details] Fixed build more View in context: https://bugs.webkit.org/attachment.cgi?id=378399&action=review > Source/WebCore/ChangeLog:16 > + are involed from author scripts. Nit - involed => involved > Source/WebCore/editing/VisibleUnits.cpp:772 > + return findNextWordFromIndex(text, offset, NextWordDirection::Backward, iosMode); Nit - maybe name the variable nextWordMode or whitespaceMode instead of iosMode? (Or perhaps more verbose — nextWordModeInIOS). > Source/WebCore/editing/VisibleUnits.h:45 > +WEBCORE_EXPORT VisiblePosition nextWordPosition(const VisiblePosition &, NextWordModeInIOS = NextWordModeInIOS::LegacyStopBeforeWord); Nit - might consider removing the extra space before the ampersand in `const VisiblePosition &`, while we’re touching this line. > Source/WebCore/platform/text/TextBoundaries.h:51 > + enum class NextWordDirection { Forward, Backward }; I usually use `enum class NextWordDirection : bool { ~ }` for these, but it doesn’t really matter since NextWordDirection isn’t stored anywhere. > Source/WebCore/platform/text/TextBoundaries.h:52 > + enum class NextWordModeInIOS { LegacyStopBeforeWord, StopAfterWord }; (Ditto with the bool enum class) Thanks for the review. (In reply to Wenson Hsieh from comment #7) > Comment on attachment 378399 [details] > Fixed build more > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378399&action=review > > > Source/WebCore/ChangeLog:16 > > + are involed from author scripts. > > Nit - involed => involved Fixed. > > Source/WebCore/editing/VisibleUnits.cpp:772 > > + return findNextWordFromIndex(text, offset, NextWordDirection::Backward, iosMode); > > Nit - maybe name the variable nextWordMode or whitespaceMode instead of > iosMode? (Or perhaps more verbose — nextWordModeInIOS). Will use nextWordModeInIOS. whitespaceMode isn't right because we consider delimiters such as "," as well. > > Source/WebCore/editing/VisibleUnits.h:45 > > +WEBCORE_EXPORT VisiblePosition nextWordPosition(const VisiblePosition &, NextWordModeInIOS = NextWordModeInIOS::LegacyStopBeforeWord); > > Nit - might consider removing the extra space before the ampersand in `const > VisiblePosition &`, while we’re touching this line. Sure, done. > > Source/WebCore/platform/text/TextBoundaries.h:51 > > + enum class NextWordDirection { Forward, Backward }; > > I usually use `enum class NextWordDirection : bool { ~ }` for these, but it > doesn’t really matter since NextWordDirection isn’t stored anywhere. Thanks for catching that. I always forget to specify the enum class' width. I've been coding too long in old C/C++ :( > > Source/WebCore/platform/text/TextBoundaries.h:52 > > + enum class NextWordModeInIOS { LegacyStopBeforeWord, StopAfterWord }; > > (Ditto with the bool enum class) Fixed this one too. Created attachment 378444 [details]
Patch for landing
Comment on attachment 378444 [details]
Patch for landing
Wait for EWS again.
Finally! Comment on attachment 378444 [details]
Patch for landing
Ugh... wrong + :(
Comment on attachment 378444 [details] Patch for landing Clearing flags on attachment: 378444 Committed r249709: <https://trac.webkit.org/changeset/249709> All reviewed patches have been landed. Closing bug. Reverted r249709 for reason: Layout test added in this patch has been consistently failing since it landed. Committed r249838: <https://trac.webkit.org/changeset/249838> |