Bug 212004 – Bottle up iOS word boundary hack behind interface

• Component: HTML Editing
• Status: ASSIGNED

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 399610 [details]
Patch

[Daniel Bates]

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 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 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 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 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 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"