Several fixes to manual fullscreen tests #13102
Conversation
upsuper
force-pushed
the
fullscreen-fix
branch
from
5410c69 to
923f836
Compare
| @@ -20,7 +20,7 @@ | |||
| const events = []; | |||
| const callback = t.step_func(event => { | |||
| // fullscreenElement should have changed before either event is fired. | |||
| assert_equals(document.fullscreenElement, null, `fullscreenElement in {event.type} event`); | |||
| assert_equals(document.fullscreenElement, null, `fullscreenElement in ${event.type} event`); | |||
| iframes[1].contentDocument.onfullscreenerror = t.unreached_func("fullscreenchange error"); | ||
| }); | ||
| }); | ||
| window.onload = function() { |
There was a problem hiding this comment.
Per spec, is it really possible for the iframe content to change, given that there's nothing to load? I don't think it is. If there's an interop problem here, can you check if there's another test targeting that specifically, and link to that here to make clear that waiting for load is a workaround?
There was a problem hiding this comment.
I have no idea. Maybe worth opening an issue and looping in some other people as I'm not very familiar with the processing model.
There was a problem hiding this comment.
I filed a Gecko bug for this. Hopefully DOM people would figure that out.
There was a problem hiding this comment.
Based on the discussion with bz, it seems the spec is clear on this case that the content document shouldn't be changed, because iframe without src shouldn't navigate after its initial handling. I've submitted #13236 for testing that.
Since this test is unrelated, I think we can have this workaround to help improve test coverage.
| // if promises aren't supported treat it as executed. | ||
| promise_executed = true; | ||
| } | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
Can you add a comment about this extra step is needed, something similar to the description. Also wrap in t.step_func in case something within throws an exception.
| async_test(t => { | ||
| const ancestor = document.getElementById('ancestor'); | ||
| const iframe = ancestor.firstChild; | ||
|
|
||
| const initialStyle = getComputedStyle(iframe); | ||
| assert_equals(initialStyle.border, '1px solid rgb(0, 0, 255)', 'initial border style'); |
There was a problem hiding this comment.
If this didn't work, that's also because of a pre-existing interop issue. Do you know which behavior is correct per spec, and can you make sure there's another test that fails in whichever browser got this wrong?
There was a problem hiding this comment.
I think this is still an ongoing debate, and the spec isn't super clear about this (as many other things like in CSSOM). So I don't think we need to worry about the test here.
There was a problem hiding this comment.
Do you have a link to where the debate is happening?
There was a problem hiding this comment.
It's w3c/csswg-drafts#2529.
There was a problem hiding this comment.
Looks like the working group had resolved on the behavior of Blink, but the issue is still open. Hopefully there will be a wpt test added with the spec change.
There was a problem hiding this comment.
I sent #13238 to keep track of that.
These tests are made to start after onload so that iframe content doesn't change anymore: * fullscreen/api/element-ready-check-containing-iframe-manual.html * fullscreen/api/element-ready-check-not-allowed-manual.html * fullscreen/api/element-request-fullscreen-and-exit-iframe-manual.html * fullscreen/model/move-to-fullscreen-iframe-manual.html fullscreen/rendering/ua-style-iframe-manual.html is updated to check each subproperties individually rather than shorthand properties. fullscreen/api/element-request-fullscreen-timing-manual.html is changed so that the second test starts after an animation frame to work around Gecko throttling rAF before the first paint.
upsuper
force-pushed
the
fullscreen-fix
branch
from
923f836 to
1c6e14b
Compare
This discrepency was discovered in a Fullscreen test: #13102 The spec issue for resolving this is: w3c/csswg-drafts#2529
There was a problem hiding this comment.
Thanks for the fixes! I added links to https://bugzil.la/1493878 to explain the extra wait, but now ready to merge.
|
Thanks! |
These tests are made to start after onload so that iframe content
doesn't change anymore:
fullscreen/rendering/ua-style-iframe-manual.html is updated to check
each subproperties individually rather than shorthand properties.
fullscreen/api/element-request-fullscreen-timing-manual.html is changed
so that the second test starts after an animation frame to work around
Gecko throttling rAF before the first paint.