When there is any media element with controls presents, prefixed fullscreen events won't be triggered

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

3 years ago
This is because in the current videocontrols.xml, there is code
> addListener(this.video.ownerDocument, "fullscreenchange", this.onFullscreenChange);

Our event handling code would skip dispatching prefixed fullscreen event when there is any listener of the unprefixed event on the same node.

Given fullscreen events can be caught only on document and window, it is highly likely that the content puts listener on the same node.

We may change that code to listen to "mozfullscreenchange" instead in bug 1273468, but that would make the issue reversed: the video controls would not respond to fullscreenchange event properly.

One solution could be, making the video control put a system listener for fullscreen events rather than normal listener. But I have no idea whether it is feasible.
Assignee

Updated

3 years ago
Summary: When there is any media element with controls present, prefixed fullscreen events won't be triggered → When there is any media element with controls presents, prefixed fullscreen events won't be triggered
Assignee

Comment 1

3 years ago
Posted file testcase
Steps to reproduce:
1. click the "Show controls" button
2. click the "Fullscreen" button
3. exit fullscreen

Expected result:
two lines of "XXXXXX: mozfullscreenchange" should be displayed above the buttons

Actual result:
nothing appears there


If you click the "Fullscreen" button before clicking "Show controls", you can see the events logged properly.
Assignee

Comment 2

3 years ago
We can probably register a system listener in content.js, and dispatch a custom event to notify the video controls.
Assignee

Updated

3 years ago
Assignee: nobody → bugzilla
Assignee

Comment 4

3 years ago
Handler for fullscreenchange event attached normally may affect or be
affected by other event handlers from the content. More specifically,
a handler listening on "fullscreenchange" event on a target would stop
any handler on the same target listening on "mozfullscreenchange" from
being invoked.

The main idea of this patch is to use a system event listener to avoid
affecting event handling in content.

Review commit: https://reviewboard.mozilla.org/r/54142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54142/
Assignee

Comment 5

3 years ago
The remaining questions are:
1. Can we only setup that listener when there is a video controls binding attached, so that we don't have any overhead if there is no video controls at all?
2. Do we need to duplicate the code for Fennec, presumably in mobile/android/chrome/content/content.js?
Assignee

Comment 6

3 years ago
I guess alternatively, I can move the whole logic in content.js to C++ code, so that we do not have listener overhead, and it is guaranteed to work on both desktop and mobile.
Assignee

Comment 7

3 years ago
Fullscreen change triggers resize reflow, and setFullscreenButtonState
should be called by setupInitialState() then when the video control is
rebound to the element.

Review commit: https://reviewboard.mozilla.org/r/54166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54166/
Attachment #8754671 - Flags: review?(gijskruitbosch+bugs)
Attachment #8754672 - Flags: review?(gijskruitbosch+bugs)
Attachment #8754672 - Flags: review?(bugs)
Assignee

Updated

3 years ago
Attachment #8754660 - Attachment is obsolete: true
Attachment #8754660 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

3 years ago
Attachment #8754661 - Attachment is obsolete: true
Attachment #8754661 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 9

3 years ago
Looks like doing this in C++ is actually simpler.
Comment on attachment 8754672 [details]
MozReview Request: Bug 1274146 part 2 - Dispatch an independent event to video control for entering fullscreen. r?smaug,gijs

https://reviewboard.mozilla.org/r/54168/#review50898

We don't want to dispatch random events to web content (the event propagates from native anonymous content to non-native anonymous).
Attachment #8754672 - Flags: review?(bugs)
Is this something we need to fix for beta?
Assignee

Comment 12

3 years ago
(In reply to Olli Pettay [:smaug] from comment #10)
> We don't want to dispatch random events to web content (the event propagates
> from native anonymous content to non-native anonymous).

But "bubbles" is set to false, would that still be propagated to content?

(In reply to Olli Pettay [:smaug] from comment #11)
> Is this something we need to fix for beta?

As far as unprefixed API is disabled, no.
Listeners in capture phase. 'bubbles' is only about bubble phase.
Assignee

Comment 14

3 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #12)
> (In reply to Olli Pettay [:smaug] from comment #11)
> > Is this something we need to fix for beta?
> 
> As far as unprefixed API is disabled, no.

But I consider this as a blocker of enabling unprefixed API as the listener here may affect or be affected by the content.
Assignee

Updated

3 years ago
Depends on: 1274520

Comment 15

3 years ago
Comment on attachment 8754671 [details]
MozReview Request: Bug 1274146 part 1 - Not call setFullscreenButtonState() in fullscreenchange handler. r?gijs

https://reviewboard.mozilla.org/r/54166/#review50906

::: toolkit/content/widgets/videocontrols.xml
(Diff revision 1)
>  
>                  onFullscreenChange: function () {
>                      if (this.isVideoInFullScreen()) {
>                          Utils._hideControlsTimeout = setTimeout(this._hideControlsFn, this.HIDE_CONTROLS_TIMEOUT_MS);
>                      }
> -                    this.setFullscreenButtonState();

Based on Neil's latest comment in bug 1187296 I don't think this change is correct. Why is it necessary?
Attachment #8754671 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 16

3 years ago
https://reviewboard.mozilla.org/r/54166/#review50906

> Based on Neil's latest comment in bug 1187296 I don't think this change is correct. Why is it necessary?

For spliting behavior change into a separate patch... Anyway, this change is not necessary if we fix bug 1274520.

Comment 17

3 years ago
Comment on attachment 8754672 [details]
MozReview Request: Bug 1274146 part 2 - Dispatch an independent event to video control for entering fullscreen. r?smaug,gijs

https://reviewboard.mozilla.org/r/54168/#review50910

::: dom/base/nsDocument.cpp:12186
(Diff revision 1)
> +    for (auto i : MakeRange(kids->Length())) {
> +      nsIContent* item = kids->Item(i);

Rather than iterating this way, if we're sending a system level event, we might as well just send it to the video element and listen for it there in the binding.
Attachment #8754672 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 18

3 years ago
https://reviewboard.mozilla.org/r/54168/#review50910

> Rather than iterating this way, if we're sending a system level event, we might as well just send it to the video element and listen for it there in the binding.

What is a "system level event"? I don't think videocontrols can catch any chrome-only event.

Comment 19

3 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #18)
> https://reviewboard.mozilla.org/r/54168/#review50910
> 
> > Rather than iterating this way, if we're sending a system level event, we might as well just send it to the video element and listen for it there in the binding.
> 
> What is a "system level event"?

The same thing bug 1274520 talks about.
bug 1274520 doesn't talk about any system only events. bug 1274520  is about videocontrols handling plain normal DOM events (which do have the default group handling and system group handling) in wrong group.
But once that bug is fixed, we could add listener to system group and then flag the event with
mOnlySystemGroupDispatchInContent
Assignee

Updated

3 years ago
Attachment #8754671 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8754672 - Attachment is obsolete: true
Assignee

Comment 21

3 years ago
This should have been fixed by bug 1274520.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.