Bug 232764

Summary: [iOS] Mail compose becomes unresponsive after pasting in text and attempting to type
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, cdumez, ews-watchlist, ggaren, megan_gardner, mifenton, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
For EWS
none
Rebase on trunk
ggaren: review+
For landing none

Description Wenson Hsieh 2021-11-05 13:28:40 PDT
rdar://84669661
Comment 1 Wenson Hsieh 2021-11-05 14:11:37 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-11-05 14:16:24 PDT
Created attachment 443437 [details]
Rebase on trunk
Comment 3 Geoffrey Garen 2021-11-05 14:54:33 PDT
Comment on attachment 443437 [details]
Rebase on trunk

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

r=me

> Source/WebCore/editing/VisibleUnits.cpp:1511
> +    case TextGranularity::SentenceGranularity: {
> +        auto boundaryInDirection = useDownstream ? endOfSentence : startOfSentence;
> +        boundary = vp == boundaryInDirection(vp) ? vp : boundaryInDirection(useDownstream ? previousSentencePosition(vp) : nextSentencePosition(vp));
>          break;
> +    }

To my taste, this would read better as 

If (vp == boundaryInDirection(vp)) {
    boundary = vp;
    break;
}

auto position = useDownstream ? previousSentencePosition(vp) : nextSentencePosition(vp);
boundary = boundaryInDirection(position);
break;

More granular to step in a debugger and diff in a source control tool this way too.
Comment 4 Wenson Hsieh 2021-11-05 14:56:28 PDT
Comment on attachment 443437 [details]
Rebase on trunk

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

>> Source/WebCore/editing/VisibleUnits.cpp:1511
>> +    }
> 
> To my taste, this would read better as 
> 
> If (vp == boundaryInDirection(vp)) {
>     boundary = vp;
>     break;
> }
> 
> auto position = useDownstream ? previousSentencePosition(vp) : nextSentencePosition(vp);
> boundary = boundaryInDirection(position);
> break;
> 
> More granular to step in a debugger and diff in a source control tool this way too.

Sounds good! Will change to that.

> Tools/ChangeLog:9
> +        Add a layout test that exercises the hang (and also verifies that we don't hit any debug assertions in the

(self-nit: s/layout test/API test/)
Comment 5 Geoffrey Garen 2021-11-05 15:04:13 PDT
If you self-nit then you also need to self-high-five when the patch lands :P
Comment 6 Wenson Hsieh 2021-11-05 16:02:26 PDT
Created attachment 443455 [details]
For landing
Comment 7 Wenson Hsieh 2021-11-05 17:46:47 PDT
Comment on attachment 443455 [details]
For landing

(In reply to Geoffrey Garen from comment #5)
> If you self-nit then you also need to self-high-five when the patch lands :P

👏
Comment 8 EWS 2021-11-05 17:51:23 PDT
Committed r285359 (243918@main): <https://commits.webkit.org/243918@main>

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