Bug 230204 – [macOS] Avoid computing post-layout EditorState data unless necessary

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

.

[Radar WebKit Bug Importer]

<rdar://problem/83078713>

[Wenson Hsieh]

Created attachment 438094 [details]
Patch

[Wenson Hsieh]

Created attachment 438095 [details]
Just for EWS

[Wenson Hsieh]

Created attachment 438144 [details]
For EWS

[Devin Rousso]

Comment on attachment 438144 [details]
For EWS

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

r=me, neat stuff :)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1234
> -    result.isContentEditable = selection.isContentEditable();
> +    result.isContentEditable = selection.hasEditableStyle();

These appear to have different `UserSelectAllTreatment`.  Is that a problem?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1327
> +    auto isEditable = frame->selection().selection().hasEditableStyle() ? EditorStateIsContentEditable::Yes : EditorStateIsContentEditable::No;

ditto (:1231)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3561
>      m_hasEverFocusedElementDueToUserInteractionSincePageTransition = false;

NIT: Shouldn't this really be inside a `HAVE(TOUCH_BAR)`?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5979
> -    m_hasEverFocusedElementDueToUserInteractionSincePageTransition |= m_userIsInteracting;
> +    if (m_userIsInteracting && m_focusedElement)
> +        m_hasEverFocusedElementDueToUserInteractionSincePageTransition = true;

This seems like a actual functional difference.  Is it something we can/should test?

> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:1089
> +    if (m_needsFontAttributes)

NIT: it'd be nice if we could add comments explaining why these things require computing post layout data

[Wenson Hsieh]

Comment on attachment 438144 [details]
For EWS

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1234
>> +    result.isContentEditable = selection.hasEditableStyle();
> 
> These appear to have different `UserSelectAllTreatment`.  Is that a problem?

I don't think this will be an issue since we already use this option when grabbing the `rootEditableElement()`, computing editing style, and other adjacent codepaths.

(`isContentRichlyEditable()` also uses this option, so it would be consistent with that)

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3561
>>      m_hasEverFocusedElementDueToUserInteractionSincePageTransition = false;
> 
> NIT: Shouldn't this really be inside a `HAVE(TOUCH_BAR)`?

Moved this into `HAVE(TOUCH_BAR)` instead.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5979
>> +        m_hasEverFocusedElementDueToUserInteractionSincePageTransition = true;
> 
> This seems like a actual functional difference.  Is it something we can/should test?

It's not actually clear to me what the functional difference would be, since this should only prevent us from setting `m_hasEverFocusedElementDueToUserInteractionSincePageTransition` to true in cases where we wouldn't have tried to show the text editing controls on the Touch Bar anyways.

>> Source/WebKit/WebProcess/WebPage/mac/WebPageMac.mm:1089
>> +    if (m_needsFontAttributes)
> 
> NIT: it'd be nice if we could add comments explaining why these things require computing post layout data

This is because font attributes are plumbed through post-layout data. I'll add comments clarifying this.

[Wenson Hsieh]

Created attachment 438162 [details]
For EWS

[EWS]

Committed r282419 (241678@main): <https://commits.webkit.org/241678@main>

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