script: Implement DocumentOrShadowRoot.FullscreenDocument#42401
script: Implement DocumentOrShadowRoot.FullscreenDocument#42401simonwuelker merged 1 commit intoservo:mainfrom
Conversation
|
🔨 Triggering try run (#21776261668) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21776261668): Flaky unexpected result (30)
Stable unexpected results that are known to be intermittent (33)
Stable unexpected results (3)
|
|
|
sebsebmc
left a comment
There was a problem hiding this comment.
Please update the comments to have the full line from the specs and not just step numbers.
|
🔨 Triggering try run (#21811194081) for Linux (WPT) |
|
Checking for possible additional WPT changes. |
|
Test results for linux-wpt from try job (#21811194081): Flaky unexpected result (33)
Stable unexpected results that are known to be intermittent (26)
Stable unexpected results (2)
|
|
|
stevennovaryo
left a comment
There was a problem hiding this comment.
Looking good! Some suggestions.
We just need to check the reason for the failure, which I believe related to the non-compliant fullscreen element.
| @@ -4599,7 +4601,7 @@ impl VirtualMethods for Element { | |||
|
|
|||
| let doc = self.owner_document(); | |||
|
|
|||
| let fullscreen = doc.GetFullscreenElement(); | |||
| let fullscreen = doc.fullscreen_element(); | |||
| if fullscreen.as_deref() == Some(self) { | |||
There was a problem hiding this comment.
This change was necessary to fix the test regressions. I suspect the reason was because now Document::GetFullscreenElement is implemented as per the spec, which may return null if the fullscreen element is removed from the document while Document::fullscreen_element is still pointing to the removed element. Changing it to return the Document::fullscreen_element field fixed the failing tests.
stevennovaryo
left a comment
There was a problem hiding this comment.
Thanks! LGTM with a nit.
simonwuelker
left a comment
There was a problem hiding this comment.
LGTM with a couple of nits
| let retargeted = fullscreen_element? | ||
| .upcast::<EventTarget>() | ||
| .retarget(node.upcast()); | ||
| let candidate = retargeted.downcast::<Element>().unwrap(); |
There was a problem hiding this comment.
This unwrap is scary. Please add a comment that it's okay because the retarget algorithm returns either fullscreen_element or a shadow host in its list of shadow-including ancestors, and all of those are elements.
There was a problem hiding this comment.
Added this comment:
It's safe to unwrap downcasting to
Elementbecauseretargeteither returnsfullscreen_elementor a host offullscreen_elementand hosts are always elements.
Details such as shadow-including ancestors seem like not essential here as any kind of host is always an element.
Signed-off-by: Onur Sahin <[email protected]>
Implement DocumentOrShadowRoot.FullscreenDocument.
Changes
Document.fullscreenElementand declaring it inDocumentOrShadowRootmixin as per the specDocumentOrShadowroot.get_fullscreen_elementwhich is used byDocumentandShadowRoot.shadowroot-fullscreen-element.html.inisince the test now passes.Testing
Enable
shadowroot-fullscreen-element.htmlWPT test.Passing WPT run: https://github.com/onsah/servo/actions/runs/21872882492
Fixes
#42234