Bug 199349 – Tapping on the bottom part of youtube video behaves as if controls were visible

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

<rdar://problem/51955744>

[zalan]

Created attachment 373192 [details]
Patch

[Brent Fulgham]

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?

[zalan]

(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.

[zalan]

Created attachment 373202 [details]
Patch

[zalan]

Created attachment 373207 [details]
Patch

[zalan]

Created attachment 373208 [details]
Patch

[Simon Fraser (smfr)]

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.

[zalan]

Created attachment 373247 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 373247 [details]
Patch

Clearing flags on attachment: 373247

Committed r247015: <https://trac.webkit.org/changeset/247015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52477135>

[Daniel Bates]

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...)