Bug 212004

Summary: Bottle up iOS word boundary hack behind interface
Product: WebKit Reporter: Daniel Bates <dbates>
Component: HTML EditingAssignee: 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 Flags
Patch dbates: review?

Daniel Bates
Reported 2020-05-17 15:35:07 PDT
On iOS finding of word boundaries uses a different algorithm than Mac. Fixing this is tracked in <rdar://problem/7259611>, but doing so carries substantial regression risk because editing code may have been written knowingly or unknowingly against iOS current algorithm. Over the years this discrepancy has led to bugs and there are least 3 call sites that hack around it. Instead of duplicating this hack, centralize it, and make it very clear that it is a hack and that the goal is to remove it and fix the algorithm.
Attachments
Patch (10.89 KB, patch)
2020-05-17 16:04 PDT, Daniel Bates 		dbates: review?
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 2020-05-17 16:04:15 PDT
Created attachment 399610 [details] Patch
Daniel Bates
Comment 2 2020-05-17 16:05:08 PDT
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
Daniel Bates
Comment 3 2020-05-17 16:08:18 PDT
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...
Daniel Bates
Comment 4 2020-05-19 14:46:45 PDT
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.
Daniel Bates
Comment 5 2020-05-19 14:47:15 PDT
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 ^^^.
Tim Horton
Comment 6 2020-05-19 14:58:04 PDT
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? 🤷‍♂️
Darin Adler
Comment 7 2020-05-22 16:20:22 PDT
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"
Note You need to log in before you can comment on or make changes to this bug.