[Webkit-unassigned] [Bug 216607] webkitfullscreenchange does not fire for shadow DOM elements

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Sep 27 12:02:08 PDT 2020


https://bugs.webkit.org/show_bug.cgi?id=216607

--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 409844
  --> https://bugs.webkit.org/attachment.cgi?id=409844
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.');

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200927/1fcee3bf/attachment.htm>


More information about the webkit-unassigned mailing list