Bug 199349

Created attachment 373192 [details] Patch

Comment on attachment 373192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373192&action=review Looks good. Is this ready for review? > Source/WebCore/page/ios/ContentChangeObserver.cpp:84 > + for (auto* parent = node.parentNode(); parent && i < 3; parent = parent->parentNode(), ++i) { What is magic about the grandparent node? Should this be encoded as a constant explaining it’s importance?

(In reply to Brent Fulgham from comment #2) > Comment on attachment 373192 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373192&action=review > > Looks good. Is this ready for review? Almost. > > > Source/WebCore/page/ios/ContentChangeObserver.cpp:84 > > + for (auto* parent = node.parentNode(); parent && i < 3; parent = parent->parentNode(), ++i) { > > What is magic about the grandparent node? Should this be encoded as a > constant explaining it’s importance? Will explain it in the final patch. Opacity behaves different in the context of hittesting than display: none or visibility: hidden.

Created attachment 373202 [details] Patch

Created attachment 373207 [details] Patch

Created attachment 373208 [details] Patch

Comment on attachment 373208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373208&action=review > Source/WebCore/ChangeLog:9 > + Synthetic click event should not be dispatched to a node that is initially hidden and becomes visible by the touchStart event. Quality "hidden" here as including opacity:0. Also say that we're deliberately choose a different behavior from macOS.

Created attachment 373247 [details] Patch

Comment on attachment 373247 [details] Patch Clearing flags on attachment: 373247 Committed r247015: <https://trac.webkit.org/changeset/247015>

All reviewed patches have been landed. Closing bug.

<rdar://problem/52477135>

Comment on attachment 373247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373247&action=review > Source/WebCore/page/ios/ContentChangeObserver.cpp:85 > + constexpr static unsigned numberOfAncestorsToCheckForOpacity = 4; The static is absolutely unnecessary here. constexpr static == constexpr unless the compiler is a piece of nonsensical garbage 😃. In my opinion I would remove the static. > Source/WebCore/page/ios/ContentChangeObserver.cpp:86 > + unsigned i = 0; In my opinion, it is more idiomatic to define this **in*** the for loop ()s <— also scopes it! > Source/WebCore/page/ios/ContentChangeObserver.cpp:88 > + if (!parent->renderStyle() || !parent->renderStyle()->opacity()) Use C++17 if init-statement? 🤷‍♂️ (My line of thinking...dumb compiler will do two calls instead of hoisting the load and doing one for renderStyle(); init-statement is unambiguous what to do...)