Bug 216607 – webkitfullscreenchange does not fire for shadow DOM elements

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

Created attachment 408915 [details]
Test case.

I attached an example. In the example there are two elements that can be shown in fullscree mode (by clicking the respecitve buttons). The fullscreenchange (and webkitfullscreenchange) events are then captured and logged.

In Chrome and Firefox clicking one and then the other would produce:
webkitfullscreenchange fullscreen-content-regular
webkitfullscreenchange [leaving]
webkitfullscreenchange container-shadow-root
webkitfullscreenchange [leaving]

In Safari 14.0.1 (WebKit 15610.2.3.1) this produces:
webkitfullscreenchange fullscreen-content-regular
webkitfullscreenchange [leaving]

That is, webkitfullscreenchange event is not fired for the element in shadow DOM.

[Radar WebKit Bug Importer]

<rdar://problem/69010476>

[Tetsuharu Ohzeki [UTC+9]]

I'll take this

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 409826 [details]
WIP

[Tetsuharu Ohzeki [UTC+9]]

I just understand that we had https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/shadow-dom/fullscreen-in-shadow-webkitCurrentFullScreenElement.html but we could not catch this bug, because WebKit still does not support `onfullscreenchange` event which does not contain `webkit` prefix.

I seem we need to add a new test.

[Darin Adler]

Comment on attachment 409826 [details]
WIP

Can we add a regression test for this please?

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 409830 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Darin Adler from comment #5)
> Comment on attachment 409826 [details]
> WIP
> 
> Can we add a regression test for this please?


Yes! I added in https://bugs.webkit.org/attachment.cgi?id=409830&action=diff
How about this?

[Darin Adler]

Comment on attachment 409830 [details]
Patch

r=me assuming the new regression test passes; please don’t land before EWS confirms

[Tetsuharu Ohzeki [UTC+9]]

Sure!

Some ports disables tests for fullscreen under LayoutTests/fast/shadow-dom/.
I might need to update expectations for their ports.

After EWS confirms, I'll add cq?

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 409844 [details]
Patch

This disabled the added regression test for ios-wk2 as like other fullscreen tests.

[Ryosuke Niwa]

Comment on attachment 409844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409844&action=review

> Source/WebCore/ChangeLog:3
> +        Set `composed` flag to true on firing ` webkitfullscreenchange` event.

This line should match the bug title:
webkitfullscreenchange does not fire for shadow DOM elements

> Source/WebCore/ChangeLog:9
> +        This is defined by the step 3-2 of
> +        https://fullscreen.spec.whatwg.org/#run-the-fullscreen-steps.

In this section, you should describe what caused the bug and how you fixed it. e.g.

The bug was caused by `webkitfullscreenchange` event being fired without composed flag set.
Fixed the bug by making it composed so that event listeners outside s shadow tree could observe it.

> LayoutTests/ChangeLog:3
> +        Set composed flag to true on firing webkitfullscreenchange event.

Ditto about this line matching the bug title.

> LayoutTests/ChangeLog:8
> +        Set composed flag to true on firing webkitfullscreenchange event.

This is not the change you made in LayoutTests directory.
Here, you should describe that you're adding a regression test. e.g.

Added a regression test for making an element inside a shadow tree full screen,
and listening to `webkitfullscreenchange` outside the shadow tree.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:12
> +let shadowHost = document.getElementById('host');
> +let shadowRoot = shadowHost.attachShadow({mode: 'closed'});

Use const since you're not going to change the value of these variables?

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:16
> +    const p = new Promise((_, reject) => {

Please don't use one letter variable like p. Also, this can be more concisely written as:
return new Promise ((resolve, reject) => setTimeout(reject, millisec));

Also, the usual variable name we use for milliseconds is either ms or milliseconds.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:23
> +    const p = new Promise((resolve) => {

There is no need for this local variable. Just return this new promise directly.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:24
> +        subroot.addEventListener('webkitfullscreenchange', (e) => {

Please don't use one letter variable like e.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:26
> +            debug(`event.currentTarget is ${e.currentTarget}`);

We should really be asserting that event is equal to sub-root here.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:36
> +    finalizeTest(shadowRoot),

These functions don't really finalize tests. They log when webkitfullscreenchange is fired.
logFullScreenChangeEvent?

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:40
> +function goFullscreen() {

Use async function.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:45
> +        timeout(2000),

2s might be too short of a timeout when this test is ran with other tests in EWS / buildbot.
There is really no need for this explicit timeout code.
run-webkit-tests and WebKitTestRunner/DumpRenderTree each has its own watch dog timer to timeout a test.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:47
> +    ]).catch(() => {
> +        testFailed('webkitfullscreenchange was not fired');

I don't like that this would mean that if the test fails for any other reason, we would still log this message. That's misleading.
Instead, logFullScreenChangeEvent should record the fact events were fired on each node,
and then we should be asserting that that has happened here.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:54
> +let button = document.querySelector('button');
> +button.onclick = goFullscreen;

Just add the event handler (onclick) in the HTML above.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:61
> +}

Add an instruction on how to run the test manually within a browser when window.eventSender is not available.
e.g.
else
      document.write('To test manually, click "Enter Fullscreen" above.');

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 409886 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Ryosuke Niwa from comment #11)
> Comment on attachment 409844 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409844&action=review
> > LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:54
> > +let button = document.querySelector('button');
> > +button.onclick = goFullscreen;
> 
> Just add the event handler (onclick) in the HTML above.
 
This is complex by the order of script execution because `goFullscreen` is not defined at `<button onclick=""/>` specified.

By the way, does js-test.js not have a simple assertion which does not require a string to its arguments like NodeJS' `assert()`?

[Peng Liu]

Comment on attachment 409886 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409886&action=review

> LayoutTests/ChangeLog:15
> +        disabled for ios-wk2.

We may need to enable some tests for iPads, but not in this patch.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:17
> +        subroot.addEventListener('webkitfullscreenchange', (event) => {

Nit.
s/(event)/event

[Darin Adler]

(In reply to Tetsuharu Ohzeki from comment #13)
> By the way, does js-test.js not have a simple assertion which does not
> require a string to its arguments like NodeJS' `assert()`?

I’m not sure if it does. But you are welcome to add one. The js-test.js script is maintained by all of us contributing to WebKit together, and I, at least, would be open to enhancements to it.

[Darin Adler]

Comment on attachment 409886 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409886&action=review

> Source/WebCore/ChangeLog:8
> +        This bug was caused by that `webkitfullscreenchange` event being fired

"that" -> "the"

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 409921 [details]
Patch

[Tetsuharu Ohzeki [UTC+9]]

Comment on attachment 409921 [details]
Patch

Fixed nits

[Ryosuke Niwa]

(In reply to Tetsuharu Ohzeki from comment #13)
> (In reply to Ryosuke Niwa from comment #11)
> > Comment on attachment 409844 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409844&action=review
> > > LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:54
> > > +let button = document.querySelector('button');
> > > +button.onclick = goFullscreen;
> > 
> > Just add the event handler (onclick) in the HTML above.
>  
> This is complex by the order of script execution because `goFullscreen` is
> not defined at `<button onclick=""/>` specified.

There is nothing complex about this. Event handlers are not evaluated until it's triggered so it's perfectly fine to refer to a function that's defined later.

[Ryosuke Niwa]

Comment on attachment 409921 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409921&action=review

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:41
> +const button = document.querySelector('button');
> +button.onclick = goFullscreen;

Like I clarified above, there is no need for this event handler to be defined in the script.

> LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:50
> +} else {
> +    document.write('To test manually, click "Enter Fullscreen" above.');
> +}

Nit: There should be no curly brackets around document.write since it's a single one statement:
https://bugs.webkit.org/show_bug.cgi?id=216607

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Ryosuke Niwa from comment #19)
> (In reply to Tetsuharu Ohzeki from comment #13)
> > (In reply to Ryosuke Niwa from comment #11)
> > > Comment on attachment 409844 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=409844&action=review
> > > > LayoutTests/fast/shadow-dom/fullscreen-in-shadow-event-should-propagate.html:54
> > > > +let button = document.querySelector('button');
> > > > +button.onclick = goFullscreen;
> > > 
> > > Just add the event handler (onclick) in the HTML above.
> >  
> > This is complex by the order of script execution because `goFullscreen` is
> > not defined at `<button onclick=""/>` specified.
> 
> There is nothing complex about this. Event handlers are not evaluated until
> it's triggered so it's perfectly fine to refer to a function that's defined
> later.

Ah, I see. I misunderstood.

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 409941 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 409941 [details]
Patch

By the way, you can use "webkit-patch upload --commit-queue" to also set cq? when you upload a patch for a code review.

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Ryosuke Niwa from comment #23)
> Comment on attachment 409941 [details]
> Patch
> 
> By the way, you can use "webkit-patch upload --commit-queue" to also set cq?
> when you upload a patch for a code review.


I did not know that option.
I'll try it the next time.

[EWS]

Committed r267724: <https://trac.webkit.org/changeset/267724>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409941 [details].