rdar://78826820
Created attachment 430877 [details] Patch
Created attachment 430885 [details] Small adjustment
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 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.
Created attachment 430891 [details] Patch for landing
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].