Bug 201575 - Option + arrow moves caret past whitespace on iOS
Summary: Option + arrow moves caret past whitespace on iOS
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 201531
  Show dependency treegraph
 
Reported: 2019-09-06 22:10 PDT by Ryosuke Niwa
Modified: 2019-09-13 09:53 PDT (History)
10 users (show)

See Also:


Attachments
Fixes the bug (43.39 KB, patch)
2019-09-06 22:22 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed non-iOS builds (42.28 KB, patch)
2019-09-06 22:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
GTK build fix attempt (45.79 KB, patch)
2019-09-06 23:12 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Another GTK build fix attempt (47.17 KB, patch)
2019-09-06 23:21 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Yet another GTK build fix attempt (47.44 KB, patch)
2019-09-06 23:28 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed build more (40.83 KB, patch)
2019-09-09 13:01 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (40.92 KB, patch)
2019-09-09 23:01 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-09-06 22:10:53 PDT
See https://bugs.webkit.org/show_bug.cgi?id=201531.

This bug tracks the fix for the hardware keyboard.

<rdar://problem/51404664>
Comment 1 Ryosuke Niwa 2019-09-06 22:22:00 PDT
Created attachment 378278 [details]
Fixes the bug
Comment 2 Ryosuke Niwa 2019-09-06 22:48:31 PDT
Created attachment 378279 [details]
Fixed non-iOS builds
Comment 3 Ryosuke Niwa 2019-09-06 23:12:38 PDT
Created attachment 378282 [details]
GTK build fix attempt
Comment 4 Ryosuke Niwa 2019-09-06 23:21:58 PDT
Created attachment 378283 [details]
Another GTK build fix attempt
Comment 5 Ryosuke Niwa 2019-09-06 23:28:58 PDT
Created attachment 378284 [details]
Yet another GTK build fix attempt
Comment 6 Ryosuke Niwa 2019-09-09 13:01:04 PDT
Created attachment 378399 [details]
Fixed build more
Comment 7 Wenson Hsieh 2019-09-09 22:15:42 PDT
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)
Comment 8 Ryosuke Niwa 2019-09-09 23:00:23 PDT
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.
Comment 9 Ryosuke Niwa 2019-09-09 23:01:23 PDT
Created attachment 378444 [details]
Patch for landing
Comment 10 Ryosuke Niwa 2019-09-09 23:27:06 PDT
Comment on attachment 378444 [details]
Patch for landing

Wait for EWS again.
Comment 11 Ryosuke Niwa 2019-09-10 02:36:51 PDT
Finally!
Comment 12 Ryosuke Niwa 2019-09-10 03:09:35 PDT
Comment on attachment 378444 [details]
Patch for landing

Ugh... wrong + :(
Comment 13 WebKit Commit Bot 2019-09-10 03:54:06 PDT
Comment on attachment 378444 [details]
Patch for landing

Clearing flags on attachment: 378444

Committed r249709: <https://trac.webkit.org/changeset/249709>
Comment 14 WebKit Commit Bot 2019-09-10 03:54:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Russell Epstein 2019-09-13 09:53:36 PDT
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>