Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Media Controls not fading on click/tap when up. #13675

Merged
merged 1 commit into from
May 11, 2023
Merged

Media Controls not fading on click/tap when up. #13675

merged 1 commit into from
May 11, 2023

Conversation

megangardner
Copy link
Contributor

@megangardner megangardner commented May 10, 2023 •
edited by webkit-early-warning-system

4c392f8

Media Controls not fading on click/tap when up.
https://bugs.webkit.org/show_bug.cgi?id=256565
rdar://109126598

Reviewed by Aditya Keerthi.

The code to check if a tap/click is in/on a media element was returing
true because of the "backdrop" element, which should not count as a
media control, even though it is part of the shadowdom. We should not
have pointer-events active on this element.
Also, I removed the code about also checking the container, as it was not
being used anywhere.

* Source/WebCore/Modules/modern-media-controls/controls/media-controls.js:
(MediaControls.prototype.isPointInControls):

Canonical link: https://commits.webkit.org/263987@main

802fce9

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@megangardner megangardner self-assigned this May 10, 2023
@megangardner megangardner added the Media Bugs related to the HTML 5 Media elements. label May 10, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash a205297)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

pxlcoder
pxlcoder reviewed May 10, 2023
View reviewed changes
Source/WebCore/Modules/modern-media-controls/controls/media-controls.js Outdated
if (includeContainer && this.element === tappedElement)
return true;

if (tappedElement.classList.contains("backdrop"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set pointer-events: none on the backdrop element instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try it!

@megangardner megangardner force-pushed the eng/Media-Controls-not-fading-on-clicktap-when-up- branch from a205297 to 802fce9 Compare May 10, 2023 04:41
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 802fce9. Live statuses available at the PR page, #13675

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 802fce9)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 10, 2023
pxlcoder
pxlcoder approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@pxlcoder pxlcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Commit message is missing the changed file: Source/WebCore/Modules/modern-media-controls/controls/media-controls.css.

@megangardner megangardner added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels May 11, 2023
@megangardner
Media Controls not fading on click/tap when up.
4c392f8 
https://bugs.webkit.org/show_bug.cgi?id=256565
rdar://109126598

Reviewed by Aditya Keerthi.

The code to check if a tap/click is in/on a media element was returing
true because of the "backdrop" element, which should not count as a
media control, even though it is part of the shadowdom. We should not
have pointer-events active on this element.
Also, I removed the code about also checking the container, as it was not
being used anywhere.

* Source/WebCore/Modules/modern-media-controls/controls/media-controls.js:
(MediaControls.prototype.isPointInControls):

Canonical link: https://commits.webkit.org/263987@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Media-Controls-not-fading-on-clicktap-when-up- branch from 802fce9 to 4c392f8 Compare May 11, 2023 23:01
@webkit-commit-queue
Copy link
Collaborator

Committed 263987@main (4c392f8): https://commits.webkit.org/263987@main

Reviewed commits have been landed. Closing PR #13675 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 4c392f8 into WebKit:main May 11, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pxlcoder pxlcoder pxlcoder approved these changes

@hortont424 hortont424 Awaiting requested review from hortont424

Assignees

@megangardner megangardner

Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
Milestone
No milestone
5 participants
@megangardner @webkit-early-warning-system @webkit-commit-queue @pxlcoder @webkit-ews-buildbot