Bug 210750 - Text manipulation sometimes fails to replace text in title elements
Summary: Text manipulation sometimes fails to replace text in title elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-20 09:47 PDT by Wenson Hsieh
Modified: 2020-04-20 13:20 PDT (History)
9 users (show)

See Also:


Attachments
Patch (8.93 KB, patch)
2020-04-20 10:16 PDT, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (8.92 KB, patch)
2020-04-20 12:52 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2020-04-20 09:47:12 PDT
<rdar://problem/61066103>
Comment 1 Wenson Hsieh 2020-04-20 10:16:37 PDT
Created attachment 396986 [details]
Patch
Comment 2 Tim Horton 2020-04-20 12:10:11 PDT
Comment on attachment 396986 [details]
Patch

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

> Source/WebCore/editing/TextManipulationController.cpp:481
> +        if (replacementTokens.size() > 1 && (!item.element || !canPerformTextManipulationByReplacingEntireTextContent(*item.element)))

What's all the null checking? Didn't we just do this with `element` above? Why not just use that, too?

> Source/WebCore/editing/TextManipulationController.cpp:488
> +            if (i)

Sad that we have to avoid range-based for just for this, but alas
Comment 3 Wenson Hsieh 2020-04-20 12:12:53 PDT
Comment on attachment 396986 [details]
Patch

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

>> Source/WebCore/editing/TextManipulationController.cpp:481
>> +        if (replacementTokens.size() > 1 && (!item.element || !canPerformTextManipulationByReplacingEntireTextContent(*item.element)))
> 
> What's all the null checking? Didn't we just do this with `element` above? Why not just use that, too?

Good catch! Changed this to just use `element` and avoid the redundant null check.

>> Source/WebCore/editing/TextManipulationController.cpp:488
>> +            if (i)
> 
> Sad that we have to avoid range-based for just for this, but alas

:P
Comment 4 Darin Adler 2020-04-20 12:37:36 PDT
Comment on attachment 396986 [details]
Patch

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

>>> Source/WebCore/editing/TextManipulationController.cpp:488
>>> +            if (i)
>> 
>> Sad that we have to avoid range-based for just for this, but alas
> 
> :P

I think we should add a StringBuilder feature for appending things with separators. This comes up a *lot*.

> Source/WebCore/editing/TextManipulationController.cpp:489
> +                newValue.appendLiteral(" ");

Appending a space is more efficiently done with ' '.
Comment 5 Wenson Hsieh 2020-04-20 12:49:58 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 396986 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396986&action=review
> 
> >>> Source/WebCore/editing/TextManipulationController.cpp:488
> >>> +            if (i)
> >> 
> >> Sad that we have to avoid range-based for just for this, but alas
> > 
> > :P
> 
> I think we should add a StringBuilder feature for appending things with
> separators. This comes up a *lot*.

+1 to this.

I also looked for a helper somewhere in WTF to join Vector<String> with a separator, but couldn’t find any.

> 
> > Source/WebCore/editing/TextManipulationController.cpp:489
> > +                newValue.appendLiteral(" ");
> 
> Appending a space is more efficiently done with ' '.

Good point — changed to newValue.append(' ');
Comment 6 Wenson Hsieh 2020-04-20 12:52:39 PDT
Created attachment 397006 [details]
Patch for landing
Comment 7 EWS 2020-04-20 13:19:59 PDT
Committed r260393: <https://trac.webkit.org/changeset/260393>

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