Skip to content

[iPhone] nba.com: Fullscreen broken when requesting Desktop Website #47204

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
Merged
merged 1 commit into from
Jun 27, 2025
Merged

[iPhone] nba.com: Fullscreen broken when requesting Desktop Website #47204

merged 1 commit into from
Jun 27, 2025
+123 −3

Conversation

jernoble
Copy link
Contributor

@jernoble jernoble commented Jun 25, 2025 •
edited by webkit-early-warning-system
Loading

ca89498

[iPhone] nba.com: Fullscreen broken when requesting Desktop Website
rdar://133321190
https://bugs.webkit.org/show_bug.cgi?id=294992

Reviewed by Eric Carlson.

Adds a quirk that, because the Destop version of nba.com uses
webkitRequestFullscreen() unconditionally, finds the biggest video inside the
requesting element and takes that video into fullscreen instead.

* Source/WebCore/bindings/js/JSElementCustom.cpp:
(WebCore::JSElement::shouldEnableWebkitRequestFullScreen):
* Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:
(GenerateHeader):
(GenerateRuntimeEnableConditionalString):
* Source/WebCore/bindings/scripts/IDLAttributes.json:
* Source/WebCore/dom/DocumentFullscreen.cpp:
(WebCore::DocumentFullscreen::requestFullscreen):
* Source/WebCore/dom/Element+Fullscreen.idl:
* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::shouldEnterNativeFullscreenWhenCallingElementRequestFullscreenQuirk const):
(WebCore::handleNBAQuirks):
(WebCore::Quirks::determineRelevantQuirks):
* Source/WebCore/page/Quirks.h:
* Source/WebCore/page/QuirksData.h:

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

456b86a

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@jernoble jernoble self-assigned this Jun 25, 2025
@jernoble jernoble added the Media Bugs related to the HTML 5 Media elements. label Jun 25, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 0a40a62)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ❌ 🛠 ios ❌ 🛠 mac ❌ 🛠 wpe ⏳ 🛠 win
❌ 🧪 bindings ❌ 🛠 ios-sim ❌ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ⏳ 🧪 win-tests
❌ 🧪 webkitperl ❌ 🧪 ios-wk2 ❌ 🧪 api-mac ❌ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ⏳ 🛠 wpe-cairo
❌ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🛠 gtk
❌ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
❌ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
⏳ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
❌ 🛠 tv ⏳ 🛠 mac-safer-cpp
⏳ 🛠 tv-sim
⏳ 🛠 watch
❌ 🛠 watch-sim

@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from 0a40a62 to 3ca02c2 Compare June 25, 2025 21:13
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 3ca02c2)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ❌ 🛠 mac ❌ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ❌ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ❌ 🧪 api-mac ❌ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt ❌ 🧪 mac-wk1 ⏳ 🛠 wpe-cairo
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ❌ 🛠 gtk
✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ❌ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ❌ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
✅ 🛠 tv ❌ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
⏳ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from 3ca02c2 to 95e584a Compare June 25, 2025 22:17
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 95e584a)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios   🛠 mac ❌ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ❌ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ❌ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt   🧪 mac-wk1 ⏳ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc ⏳ 🧪 api-ios   🧪 mac-wk2 ⏳ 🛠 gtk
  🛠 vision ❌ 🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
❌ 🛠 vision-sim   🧪 mac-wk2-stress ⏳ 🧪 api-gtk
⏳ 🧪 vision-wk2   🧪 mac-intel-wk2 ⏳ 🛠 playstation
❌ 🛠 tv ⏳ 🛠 mac-safer-cpp
❌ 🛠 tv-sim
⏳ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from 95e584a to af05622 Compare June 25, 2025 22:27
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ❌ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt   🧪 mac-wk1 ⏳ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc ⏳ 🧪 api-ios   🧪 mac-wk2   🛠 gtk
✅ 🛠 vision ⏳ 🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
✅ 🧪 vision-wk2 ⏳ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
✅ 🛠 tv ⏳ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
⏳ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from af05622 to da9c241 Compare June 25, 2025 22:55
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ❌ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
❌ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@webkit-ews-buildbot
Copy link
Collaborator

Safer C++ Build #41607 (da9c241)

❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@jernoble jernoble removed the merging-blocked Applied to prevent a change from being merged label Jun 26, 2025
@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from da9c241 to c1a4173 Compare June 26, 2025 15:43
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 26, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc ✅ 🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from c1a4173 to 89993c7 Compare June 26, 2025 16:35
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 26, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 89993c7)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 wpe-cairo
  🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
✅ 🛠 tv   🛠 mac-safer-cpp
✅ 🛠 tv-sim
⏳ 🛠 watch
✅ 🛠 watch-sim

@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from 89993c7 to bd1b260 Compare June 26, 2025 17:27
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 26, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@jernoble jernoble requested a review from a team June 26, 2025 20:10
aestes
aestes reviewed Jun 26, 2025
View reviewed changes
Copy link
Contributor

@aestes aestes left a comment

Choose a reason for hiding this comment

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

Can we write a bindings test for CustomEnabledBy?

@jernoble jernoble changed the title [iPhone] nba.com: Multiple taps required to play videos [iPhone] nba.com: Fullscreen broken when requesting Desktop Website Jun 26, 2025
@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from bd1b260 to d9fe6a6 Compare June 26, 2025 21:34
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 26, 2025 •
edited
Loading

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

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

aestes
aestes reviewed Jun 26, 2025
View reviewed changes
Source/WebCore/dom/DocumentFullscreen.cpp Outdated
// Translate the request to enter fullscreen into requesting native fullscreen
// for the largest inner video element.
auto maybeVideoList = element->querySelectorAll("video"_s);
if (maybeVideoList.hasException())
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to call completionHandler here too.

@jernoble jernoble force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from d9fe6a6 to 456b86a Compare June 26, 2025 23:56
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 26, 2025 •
edited
Loading

EWS run on current version of this PR (hash 456b86a)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@jernoble jernoble added the merge-queue Applied to send a pull request to merge-queue label Jun 27, 2025
@jernoble jernoble marked this pull request as ready for review June 27, 2025 20:28
@jernoble jernoble requested review from cdumez and rniwa as code owners June 27, 2025 20:28
@jernoble
[iPhone] nba.com: Fullscreen broken when requesting Desktop Website
ca89498 
rdar://133321190
https://bugs.webkit.org/show_bug.cgi?id=294992

Reviewed by Eric Carlson.

Adds a quirk that, because the Destop version of nba.com uses
webkitRequestFullscreen() unconditionally, finds the biggest video inside the
requesting element and takes that video into fullscreen instead.

* Source/WebCore/bindings/js/JSElementCustom.cpp:
(WebCore::JSElement::shouldEnableWebkitRequestFullScreen):
* Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:
(GenerateHeader):
(GenerateRuntimeEnableConditionalString):
* Source/WebCore/bindings/scripts/IDLAttributes.json:
* Source/WebCore/dom/DocumentFullscreen.cpp:
(WebCore::DocumentFullscreen::requestFullscreen):
* Source/WebCore/dom/Element+Fullscreen.idl:
* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::shouldEnterNativeFullscreenWhenCallingElementRequestFullscreenQuirk const):
(WebCore::handleNBAQuirks):
(WebCore::Quirks::determineRelevantQuirks):
* Source/WebCore/page/Quirks.h:
* Source/WebCore/page/QuirksData.h:

Canonical link: https://commits.webkit.org/296749@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/iPhone-nba-com-Multiple-taps-required-to-play-videos branch from 456b86a to ca89498 Compare June 27, 2025 20:43
@webkit-commit-queue
Copy link
Collaborator

Committed 296749@main (ca89498): https://commits.webkit.org/296749@main

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

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

@aestes aestes aestes left review comments

@eric-carlson eric-carlson eric-carlson approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

Assignees

@jernoble jernoble

Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jernoble @webkit-early-warning-system @webkit-ews-buildbot @webkit-commit-queue @aestes @eric-carlson