Bug 226775 – [iOS] Safari tab pill should toggle visibility when tapping on article text on adventure.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

rdar://78826820

[Wenson Hsieh]

Created attachment 430877 [details]
Patch

[Wenson Hsieh]

Created attachment 430885 [details]
Small adjustment

[Devin Rousso]

Comment on attachment 430885 [details]
Small adjustment

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

r=me as well :)

> Source/WebKit/ChangeLog:9
> +        Adjust the meaningful click heuristic to account for click event listeners added to the document node. See below

Do we also want to consider if the event listener was added to the `document.documentElement` or `window`?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:845
> +    if (is<HTMLBodyElement>(clickNode) || clickNode.isDocumentNode())

NIT: `is<Document>(clickNode)`

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1700
> +    auto frame = makeRefPtr(node.document().frame());

Is it actually necessary/desirable to keep `frame` alive for the lifetime of this function?  My understanding of the "rules" regarding this is that since `Frame::view` is a simple getter it's not necessary to keep the `frame` alive (this is also true for `FrameDestructionObserver::frame`).

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1735
> +    auto frame = makeRefPtr(node.document().frame());

ditto (:1700)

> LayoutTests/fast/events/ios/non-meaningful-click-when-tapping-document.html:6
> +body, html {

NIT: Is this actually necessary?

[Wenson Hsieh]

Comment on attachment 430885 [details]
Small adjustment

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

>> Source/WebKit/ChangeLog:9
>> +        Adjust the meaningful click heuristic to account for click event listeners added to the document node. See below
> 
> Do we also want to consider if the event listener was added to the `document.documentElement` or `window`?

Good point — I'll a check for the documentElement(). However, we don't seem to dispatch synthetic clicks in the case where the click event listener is added on `window`...

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:845
>> +    if (is<HTMLBodyElement>(clickNode) || clickNode.isDocumentNode())
> 
> NIT: `is<Document>(clickNode)`

Sure.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1700
>> +    auto frame = makeRefPtr(node.document().frame());
> 
> Is it actually necessary/desirable to keep `frame` alive for the lifetime of this function?  My understanding of the "rules" regarding this is that since `Frame::view` is a simple getter it's not necessary to keep the `frame` alive (this is also true for `FrameDestructionObserver::frame`).

My understanding is that it's considered better practice to use smart pointers to hold on to RefCounted objects in local variables, even if it's only being called with trivial getters.

>> LayoutTests/fast/events/ios/non-meaningful-click-when-tapping-document.html:6
>> +body, html {
> 
> NIT: Is this actually necessary?

Not all of them - just width and height. I'll remove the other properties, I suppose.

[Wenson Hsieh]

Created attachment 430891 [details]
Patch for landing

[EWS]

Committed r278633 (238616@main): <https://commits.webkit.org/238616@main>

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