Bug 216607 - webkitfullscreenchange does not fire for shadow DOM elements
Summary: webkitfullscreenchange does not fire for shadow DOM elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Mac macOS 10.15
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2020-09-16 05:13 PDT by naktinis
Modified: 2020-09-28 21:10 PDT (History)
12 users (show)

See Also:


Attachments
Test case. (2.41 KB, text/html)
2020-09-16 05:13 PDT, naktinis
no flags Details
WIP (1.93 KB, patch)
2020-09-27 06:45 PDT, Tetsuharu Ohzeki [UTC+9]
no flags Details | Formatted Diff | Diff
Patch (5.50 KB, patch)
2020-09-27 07:58 PDT, Tetsuharu Ohzeki [UTC+9]
no flags Details | Formatted Diff | Diff
Patch (6.53 KB, patch)
2020-09-27 10:35 PDT, Tetsuharu Ohzeki [UTC+9]
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2020-09-28 07:40 PDT, Tetsuharu Ohzeki [UTC+9]
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2020-09-28 14:09 PDT, Tetsuharu Ohzeki [UTC+9]
no flags Details | Formatted Diff | Diff
Patch (6.77 KB, patch)
2020-09-28 18:40 PDT, Tetsuharu Ohzeki [UTC+9]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description naktinis 2020-09-16 05:13:11 PDT
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.
Comment 1 Radar WebKit Bug Importer 2020-09-16 13:29:14 PDT
<rdar://problem/69010476>
Comment 2 Tetsuharu Ohzeki [UTC+9] 2020-09-27 06:30:30 PDT
I'll take this
Comment 3 Tetsuharu Ohzeki [UTC+9] 2020-09-27 06:45:06 PDT
Created attachment 409826 [details]
WIP
Comment 4 Tetsuharu Ohzeki [UTC+9] 2020-09-27 07:36:08 PDT
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.
Comment 5 Darin Adler 2020-09-27 07:41:00 PDT
Comment on attachment 409826 [details]
WIP

Can we add a regression test for this please?
Comment 6 Tetsuharu Ohzeki [UTC+9] 2020-09-27 07:58:35 PDT
Created attachment 409830 [details]
Patch
Comment 7 Tetsuharu Ohzeki [UTC+9] 2020-09-27 08:10:04 PDT
(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?
Comment 8 Darin Adler 2020-09-27 08:12:15 PDT
Comment on attachment 409830 [details]
Patch

r=me assuming the new regression test passes; please don’t land before EWS confirms
Comment 9 Tetsuharu Ohzeki [UTC+9] 2020-09-27 08:16:30 PDT
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?
Comment 10 Tetsuharu Ohzeki [UTC+9] 2020-09-27 10:35:11 PDT
Created attachment 409844 [details]
Patch

This disabled the added regression test for ios-wk2 as like other fullscreen tests.
Comment 11 Ryosuke Niwa 2020-09-27 12:02:08 PDT
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.');
Comment 12 Tetsuharu Ohzeki [UTC+9] 2020-09-28 07:40:50 PDT
Created attachment 409886 [details]
Patch
Comment 13 Tetsuharu Ohzeki [UTC+9] 2020-09-28 07:45:43 PDT
(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()`?
Comment 14 Peng Liu 2020-09-28 11:06:41 PDT
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
Comment 15 Darin Adler 2020-09-28 11:56:33 PDT
(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.
Comment 16 Darin Adler 2020-09-28 11:58:11 PDT
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"
Comment 17 Tetsuharu Ohzeki [UTC+9] 2020-09-28 14:09:37 PDT
Created attachment 409921 [details]
Patch
Comment 18 Tetsuharu Ohzeki [UTC+9] 2020-09-28 14:10:17 PDT
Comment on attachment 409921 [details]
Patch

Fixed nits
Comment 19 Ryosuke Niwa 2020-09-28 15:18:04 PDT
(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.
Comment 20 Ryosuke Niwa 2020-09-28 15:20:09 PDT
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
Comment 21 Tetsuharu Ohzeki [UTC+9] 2020-09-28 18:38:35 PDT
(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.
Comment 22 Tetsuharu Ohzeki [UTC+9] 2020-09-28 18:40:33 PDT
Created attachment 409941 [details]
Patch
Comment 23 Ryosuke Niwa 2020-09-28 20:57:57 PDT
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.
Comment 24 Tetsuharu Ohzeki [UTC+9] 2020-09-28 21:05:26 PDT
(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.
Comment 25 EWS 2020-09-28 21:10:15 PDT
Committed r267724: <https://trac.webkit.org/changeset/267724>

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