2033397 - Keyboard lock state isn't updated correctly, while top-level document goes into fullscreen (without keyboard lock) first then requesting fullscreen (with keyboard lock) on OOP iframe

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Edgar Chen [:edgar]]

Requesting fullscreen in a OOP iframe while top-level is already in fullscreen doesn't even work on Safari.

Comment 2

[Simon Farre [:sfarre]]

Initially I was thinking this here is probably the culprit but I'm not entirely sure now after having looked it over again.

Worth looking into, is if the right boolean value is getting passed in here

Comment 3

[Edgar Chen [:edgar]]

Attachment: iframe.html

Comment 4

[Edgar Chen [:edgar]]

STR:

1. load https://codepen.io/editor/edgarchen-the-decoder/pen/019d9acf-e39e-7654-b81b-4dd06fbe6aa8
2. Click Fullscreen on top-level document
3. Wait for top-level document goes into fullscreen
4. Click Fullscreen with keyboardLock on the OOP-iframe
5. wait for OOP-iframe goes into fullscreen
6. Check initial warning and press Esc

Actual result:
Initial warning doesn't show the right message (pres and hold esc).
And pres Esc exits the fullscreen.

Expected result:
initial warning should show right message.
And pres Esc should not exit fullscreen.

Comment 5

[Simon Farre [:sfarre]]

Attachment: Bug 2033397 - Update keyboard lock appropriately r=edgar!

Update the keyboard lock without checking if we had a previous
fullscreen dock, because when we have OOP frames, they are not
accessible there (and therefore will not update the state properly).

Added web platform tests that test for this specific scenario.

Comment 6

[Edgar Chen [:edgar]]

Attachment: Bug 2033397 - Add brower fullscreen warning test;

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5a5e82b939cc
https://hg.mozilla.org/integration/autoland/rev/b8f056556e72
Update keyboard lock appropriately r=edgar,avandolder

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d87156d73579
https://hg.mozilla.org/integration/autoland/rev/09efcbda4ab9
Revert "Bug 2033397 - Update keyboard lock appropriately r=edgar,avandolder" for causing wpt failures at iframe.tentative

Comment 9

[Silaghi Andreea]

Backed out for causing wpt failures at iframe.tentative
Backout Link
Push with failures
Failure Log
Failure line TEST-UNEXPECTED-FAIL | /fullscreen/api/keyboard-lock-cross-origin-iframe.tentative.sub.html | Cross-origin iframe fullscreen with keyboardLock:browser — hold Escape exits - assert_equals: single Escape should not exit fullscreen when keyboardLock is browser expected Element node <iframe id="cross-origin-iframe" src="http://not-web-plat... but got null

Comment 10

[Simon Farre [:sfarre]]

Test ran accidentally on android, fixed in latest submission

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c42d01ef9b2d
https://hg.mozilla.org/integration/autoland/rev/fb53180f47a1
Update keyboard lock appropriately r=edgar,avandolder

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/59671 for changes under testing/web-platform/tests

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fb53180f47a1

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f365feaf064f
https://hg.mozilla.org/integration/autoland/rev/d58ccf489f6e
Add brower fullscreen warning test; r=avandolder

Comment 16

[Simon Farre [:sfarre]]

Attachment: Bug 2033397 - Update keyboard lock appropriately

Update keyboard lock state only when chrome document enters fullscreen,
and in the case where an optimization is performed when a content
process already sees itself to be in DOM fullscreen, then it should
update kblock state of the chrome document as well, so that the correct
warning is displayed.

Added web platform tests that test for this specific scenario.

Original Revision: https://phabricator.services.mozilla.com/D295315

Comment 17

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Potential buggy behavior with OOP iframes
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Keyboard lock related only effects
• String changes made/needed?: No
• Is Android affected?: no

Comment 18

[Simon Farre [:sfarre]]

Attachment: Bug 2033397 - Add brower fullscreen warning test;

Original Revision: https://phabricator.services.mozilla.com/D295810

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/06bf41eda843
https://hg.mozilla.org/integration/autoland/rev/9ddcf6ffba44
Revert "Bug 2033397 - Add brower fullscreen warning test; r=avandolder" for causing bc failures at browser_fullscreen_keyboard_lock_warning

Comment 20

[Silaghi Andreea]

Backed out for causing bc failures at fullscreen_keyboard_lock_warning.js
Backout Link
Push with failures
Failure Log
Failure line TEST-UNEXPECTED-FAIL | browser/base/content/test/fullscreen/browser_fullscreen_keyboard_lock_warning.js | test_keyboard_lock_change_warning_change_oop_iframe - Test timed out

Comment 21

[Simon Farre [:sfarre]]

Try run for mochitest-browser-chrome https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=43409

Its hard to tell if this is an intermittent failure or not, but given that it (seems) to only fail on windows 11 32 bit, then it's a bit suspect.

Comment 22

[Phabricator Automation]

Comment on attachment 9572426 [details]
Bug 2033397 - Add brower fullscreen warning test;

Revision D295810 was moved to bug 2037849. Setting attachment 9572426 [details] to obsolete.

Comment 23

[Edgar Chen [:edgar]]

This is reopened due the test patch got backed out.
We move the test patch into another bug to land (bug 2037849).
So I think we can close this one.

Comment 24

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/4fc6fe7f3251
https://hg.mozilla.org/releases/mozilla-beta/rev/4b9dca5503c2