Bug 216327 – Text replacements at the beginning of a second line are replaced too early

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Overlapping text replacments at the beginning of a line are replaced too early.

[Megan Gardner]

Created attachment 408371 [details]
Patch

[Megan Gardner]

Created attachment 408395 [details]
Patch

[Megan Gardner]

Created attachment 408396 [details]
Patch

[Darin Adler]

Comment on attachment 408396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408396&action=review

> Source/WebCore/ChangeLog:12
> +        In the changes in r258871, using SimpleRanges instead of Range causing some side effects
> +        when the replacements at the beginning of lines. The ranges that we are counting are backwards
> +        and the return characters are being counted instead of being ignored. There is almost 
> +        certainly a better fix than this, but this patch restores the original logic that
> +        was present when Range was being used, until a better fix can be worked out. 

I appreciate you fixing this regression I caused, and I agree that we can do an expedient change to help buy us time to do a better one.

I’m guessing that the "real" fix might be to clamp the replacement range inside the paragraph when we create the TextCheckingHelper object.

> Source/WebCore/editing/TextCheckingHelper.cpp:223
> -uint64_t TextCheckingParagraph::automaticReplacementStart() const
> +uint64_t TextCheckingParagraph::automaticReplacementStart(bool oldBehaviour) const
>  {
> -    if (!m_automaticReplacementStart)
> -        m_automaticReplacementStart = characterCount({ paragraphRange().start, m_automaticReplacementRange.start });
> +    if (!m_automaticReplacementStart) {
> +
> +        auto ordering = documentOrder(paragraphRange().start, m_automaticReplacementRange.start);
> +        if (oldBehaviour && is_gt(ordering))
> +            m_automaticReplacementStart = characterCount({ m_automaticReplacementRange.start, m_automaticReplacementRange.start });
> +        else
> +            m_automaticReplacementStart = characterCount({ paragraphRange().start, m_automaticReplacementRange.start });
> +        
> +    }
>      return *m_automaticReplacementStart;
>  }

This isn’t OK, because the result is cached, so calling this once with oldBehavior true has a side effect on future calls. So if you want to make this two separate functions for now, that’s fine, but we need to either not cache the results from the "old behavior" versions (probably the right way to go) or cache the results separately.

> Source/WebCore/editing/TextCheckingHelper.cpp:235
> -uint64_t TextCheckingParagraph::automaticReplacementLength() const
> +uint64_t TextCheckingParagraph::automaticReplacementLength(bool oldBehaviour) const
>  {
> -    if (!m_automaticReplacementLength)
> -        m_automaticReplacementLength = characterCount(m_automaticReplacementRange);
> +    if (!m_automaticReplacementLength) {
> +        auto ordering = documentOrder(paragraphRange().start, m_automaticReplacementRange.start);
> +        if (oldBehaviour && is_gt(ordering))
> +            m_automaticReplacementLength = characterCount({ paragraphRange().start, m_automaticReplacementRange.end });
> +        else
> +            m_automaticReplacementLength = characterCount(m_automaticReplacementRange);
> +    }
>      return *m_automaticReplacementLength;
>  }

Same issue.

[Wenson Hsieh]

<rdar://problem/68170353>

[Megan Gardner]

Created attachment 408456 [details]
Patch

[Megan Gardner]

Created attachment 408474 [details]
Patch

[Megan Gardner]

Created attachment 408476 [details]
Patch

[Darin Adler]

Comment on attachment 408476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408476&action=review

review+ assuming tests pass on all Mac platform EWS testers (I wasn’t patient enough)

> Source/WebCore/editing/Editor.cpp:2828
> -        } else if (resultEndLocation <= automaticReplacementEndLocation && resultEndLocation >= paragraph.automaticReplacementStart()
> +        } else if (automaticReplacementStartLocation <= resultEndLocation && resultEndLocation <= automaticReplacementEndLocation

Not sure why it’s important to reorder the two checks, here. Maybe we could have just added "true" on this line and then not done the other refactoring above. Just added true here, and added true above, without introducing the new local variable.

> Source/WebCore/editing/TextCheckingHelper.cpp:211
> +uint64_t TextCheckingParagraph::automaticReplacementStart(bool oldBehaviour) const

Hope this really is temporary: Boolean not explained at all in the header, spelling here is British English. Not clear what "old" means.

Likely none of that matters if we quickly find a more straightforward fix.

> Source/WebCore/editing/TextCheckingHelper.cpp:214
> +        return characterCount({ m_automaticReplacementRange.start, m_automaticReplacementRange.start });

This is just a way to write:

   return 0;

> LayoutTests/platform/ios/TestExpectations:130
>  # Requires support for testing text replacement
>  editing/spelling/text-replacement-after-typing-to-word.html [ Skip ]
> +editing/spelling/text-replacement-first-word-second-line.html [ Skip ]

Seems like these should be (eventually) moved into a directory so we can disable/enable them all at once if we add text replacement to a platform. I imagine we might want more than 2 tests, even.

[Megan Gardner]

Comment on attachment 408476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408476&action=review

>> Source/WebCore/editing/Editor.cpp:2828
>> +        } else if (automaticReplacementStartLocation <= resultEndLocation && resultEndLocation <= automaticReplacementEndLocation
> 
> Not sure why it’s important to reorder the two checks, here. Maybe we could have just added "true" on this line and then not done the other refactoring above. Just added true here, and added true above, without introducing the new local variable.

I think this is significantly easier to read, both with the variable and the reordering.

>> Source/WebCore/editing/TextCheckingHelper.cpp:211
>> +uint64_t TextCheckingParagraph::automaticReplacementStart(bool oldBehaviour) const
> 
> Hope this really is temporary: Boolean not explained at all in the header, spelling here is British English. Not clear what "old" means.
> 
> Likely none of that matters if we quickly find a more straightforward fix.

I also hope we can find a better fix quickly.

>> Source/WebCore/editing/TextCheckingHelper.cpp:214
>> +        return characterCount({ m_automaticReplacementRange.start, m_automaticReplacementRange.start });
> 
> This is just a way to write:
> 
>    return 0;

of course, no need to actually call this.

>> LayoutTests/platform/ios/TestExpectations:130
>> +editing/spelling/text-replacement-first-word-second-line.html [ Skip ]
> 
> Seems like these should be (eventually) moved into a directory so we can disable/enable them all at once if we add text replacement to a platform. I imagine we might want more than 2 tests, even.

I will keep it in mind for later.

[Megan Gardner]

Created attachment 408490 [details]
Patch

[Darin Adler]

(In reply to Megan Gardner from comment #10)
> > Not sure why it’s important to reorder the two checks, here. Maybe we could have just added "true" on this line and then not done the other refactoring above. Just added true here, and added true above, without introducing the new local variable.
> 
> I think this is significantly easier to read, both with the variable and the
> reordering.

Reordering, OK. Variable, I don’t see how, it’s the same words, just removes the ().

[EWS]

Committed r266909: <https://trac.webkit.org/changeset/266909>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408490 [details].