Bug 226775

Summary: [iOS] Safari tab pill should toggle visibility when tapping on article text on adventure.com
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, drousso, megan_gardner, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Small adjustment
thorton: review+
Patch for landing none

Description Wenson Hsieh 2021-06-08 12:38:16 PDT
rdar://78826820
Comment 1 Wenson Hsieh 2021-06-08 12:46:01 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-06-08 13:45:39 PDT
Created attachment 430885 [details]
Small adjustment
Comment 3 Devin Rousso 2021-06-08 13:53:49 PDT
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?
Comment 4 Wenson Hsieh 2021-06-08 14:22:02 PDT
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.
Comment 5 Wenson Hsieh 2021-06-08 14:23:01 PDT
Created attachment 430891 [details]
Patch for landing
Comment 6 EWS 2021-06-08 16:30:29 PDT
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].