| Summary: | Bottle up iOS word boundary hack behind interface | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||
| Component: | HTML Editing | Assignee: | Daniel Bates <dbates> | ||||
| Status: | ASSIGNED --- | ||||||
| Severity: | Normal | CC: | darin, ews-watchlist, mifenton, thorton, wenson_hsieh | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Local Build | ||||||
| Hardware: | iPhone / iPad | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Daniel Bates
2020-05-17 15:35:07 PDT
Created attachment 399610 [details]
Patch
Comment on attachment 399610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399610&action=review > Source/WebCore/ChangeLog:23 > + (WebCore::startOfWordWithMacCompatibility): Added. Bottles up the hacks, but I discovered its its => it's Comment on attachment 399610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399610&action=review > Source/WebCore/editing/TypingCommand.cpp:449 > + VisiblePosition p1 = startOfWordWithMacCompatibility(previous, LeftWordIfOnBoundary); > + VisiblePosition p2 = startOfWordWithMacCompatibility(start, LeftWordIfOnBoundary); Could rename these to something more descriptive....I think I might have to rename start and previous as well... Comment on attachment 399610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399610&action=review > Source/WebCore/editing/VisibleUnits.cpp:730 > + if (isSpaceOrNewline(characterBefore) || characterBefore == noBreakSpace || isStartOfParagraph(position)) @Reviewer: You might be asking yourself: why!? why is OK to always call isStartOfParagraph() one call site in the current code calls isEndOfParargraph() instead?! The answer: That call site was passing the **previous** position to isEndOfParargraph(). That's why. It could have been written using isStartOfParagraph(start) and that's what I'm converting to. Comment on attachment 399610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399610&action=review >> Source/WebCore/editing/VisibleUnits.cpp:730 >> + if (isSpaceOrNewline(characterBefore) || characterBefore == noBreakSpace || isStartOfParagraph(position)) > > @Reviewer: You might be asking yourself: why!? why is OK to always call isStartOfParagraph() one call site in the current code calls isEndOfParargraph() instead?! > The answer: That call site was passing the **previous** position to isEndOfParargraph(). That's why. It could have been written using isStartOfParagraph(start) and that's what I'm converting to. Just for me: I should probably update the ChangeLog to explain ^^^. Comment on attachment 399610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399610&action=review > Source/WebCore/ChangeLog:13 > + without any kind of commenting to indicate if it depended on it. Over the years there are > + have been spot hacks added to workaround the differences. Instead of duplicating the hacks "there are have been" > Source/WebCore/editing/VisibleUnits.cpp:718 > +// FIXME: This is a workaround for <rdar://problem/7259611> Word boundary code on iPhone gives different results than desktop. > +VisiblePosition startOfWordWithMacCompatibility(const VisiblePosition& position, EWordSide side) Maybe just a Behavior argument to startOfWord that defaults to the platform's local behavior but can be specified either way? 🤷♂️ Comment on attachment 399610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399610&action=review > Source/WebCore/editing/VisibleUnits.h:53 > +// knowingly- and unknowningly- been written against the iOS algorithm. Those call sites needs to be "unknowningly" |