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.

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

Test snapshotting of allowfullscreen attribute #4354

Merged
merged 5 commits into from May 6, 2021
Merged

Test snapshotting of allowfullscreen attribute #4354

merged 5 commits into from May 6, 2021

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Dec 16, 2016 •
edited

Test for whatwg/html#2187

Edit: this ought to follow whatwg/html#3287 but I don't know right now (2020-01-27) whether it does.

@zcorpan zcorpan requested a review from foolip December 16, 2016 14:16
@wpt-pr-bot
Copy link
Collaborator

Notifying @aliams, @foolip, @jernoble, and @upsuper. (Learn how reviewing works.)

@zcorpan zcorpan mentioned this pull request Dec 16, 2016
Closed
4 tasks
@wpt-stability-bot

This comment has been minimized.

Sign in to view
@wpt-stability-bot

This comment has been minimized.

Sign in to view
foolip
foolip approved these changes Dec 16, 2016
View reviewed changes
Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Seems to be testing what it intends to. Can you see who passes and fails?

fullscreen/api/document-fullscreen-enabled-removing-allowfullscreen.sub.html Outdated

let i = 0;

iframe.src = "http://{{domains[www1]}}:{{ports[http][0]}}/fullscreen/api/echo-fullscreenEnabled.html";
Copy link
Member

Choose a reason for hiding this comment

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

I don't recognize this syntax, is this using a wptserve feature, to make it a cross-origin iframe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 16, 2016

Gecko and Edge 13 fail because of live behavior.

Chromium and WebKit have prefixed names. Testing with webkitFullscreenEnabled instead they still fail because of live behavior.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 16, 2016

Should also test the timing of when it's snapshotted; at document creation time vs completed response headers.

@zcorpan zcorpan mentioned this pull request Dec 24, 2017
Merged
@foolip
Copy link
Member

foolip commented Oct 28, 2019

@zcorpan can you rebase to trigger new CI runs?

@zcorpan zcorpan force-pushed the zcorpan/allowed-to-use-snapshot branch from c050814 to 55e4b85 Compare October 30, 2019 22:33
@zcorpan
Copy link
Member Author

zcorpan commented Oct 30, 2019

Rebased.

@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the zcorpan/allowed-to-use-snapshot branch January 24, 2020 18:08
@gsnedders gsnedders restored the zcorpan/allowed-to-use-snapshot branch January 24, 2020 18:48
@Hexcles Hexcles reopened this Jan 24, 2020
@zcorpan zcorpan requested a review from clelland January 27, 2020 14:20
@zcorpan zcorpan removed their assignment Jan 27, 2020
@zcorpan zcorpan force-pushed the zcorpan/allowed-to-use-snapshot branch from 55e4b85 to fc6c02d Compare January 27, 2020 14:24
@zcorpan zcorpan removed their assignment Jan 27, 2020
@zcorpan
Copy link
Member Author

zcorpan commented Jan 27, 2020

I rebased again to rerun CI checks.

@gsnedders
Copy link
Member

@zcorpan

ERROR:lint:fullscreen/api/document-fullscreen-enabled-setting-allowfullscreen-timing.sub.html:26: setTimeout used (SET TIMEOUT)

@wpt-pr-bot wpt-pr-bot requested a deployment to wpt-preview-4354 July 15, 2020 07:16 Pending
@zcorpan zcorpan force-pushed the zcorpan/allowed-to-use-snapshot branch from 02772dc to 14a654d Compare November 13, 2020 14:00
@zcorpan
Copy link
Member Author

zcorpan commented Nov 13, 2020

Lint error was fixed, but CI still failed. The logs were no longer available. Rebasing again.

whatwg/html#3287 replaced whatwg/html#2187 . @clelland , do these tests match what the spec now requires?

@foolip foolip force-pushed the zcorpan/allowed-to-use-snapshot branch from 14a654d to 347c7a9 Compare May 6, 2021 08:28
foolip
foolip approved these changes May 6, 2021
View reviewed changes
fullscreen/api/document-fullscreen-enabled-setting-allowfullscreen-timing.sub.html
@@ -0,0 +1,30 @@
<!DOCTYPE html>
<title>Document#fullscreenEnabled setting allowfullscreen after document creation, before response</title>
Copy link
Member

Choose a reason for hiding this comment

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

Nice and evil test, I like it!

@foolip
Copy link
Member

foolip commented May 6, 2021

The spec for this ended up as https://html.spec.whatwg.org/multipage/iframe-embed-object.html#allowed-to-use. The warning there summarizes it well:

Because they only influence the permissions policy of the nested browsing context's active document, the allow and allowfullscreen attributes only take effect when the nested browsing context of the iframe is navigated. Adding or removing them has no effect on an already-loaded document.

@foolip foolip merged commit 7f6dfe9 into master May 6, 2021
@foolip foolip deleted the zcorpan/allowed-to-use-snapshot branch May 6, 2021 08:46
@zcorpan
Copy link
Member Author

zcorpan commented May 6, 2021

Thanks @foolip!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foolip foolip foolip approved these changes

@aliams aliams Awaiting requested review from aliams

@upsuper upsuper Awaiting requested review from upsuper

@clelland clelland Awaiting requested review from clelland

Assignees

@zcorpan zcorpan

Labels
fullscreen wg-css
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@zcorpan @wpt-pr-bot @wpt-stability-bot @foolip @gsnedders @marcoscaceres @Hexcles @moz-wptsync-bot