Closed Bug 1274146 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
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
Attached 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.
We can probably register a system listener in content.js, and dispatch a custom event to notify the video controls.
Assignee: nobody → bugzilla
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/
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?
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.
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)
Attachment #8754660 - Attachment is obsolete: true
Attachment #8754660 - Flags: review?(gijskruitbosch+bugs)
Attachment #8754661 - Attachment is obsolete: true
Attachment #8754661 - Flags: review?(gijskruitbosch+bugs)
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?
(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.
(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.
Depends on: 1274520
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)
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 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)
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.
(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
Attachment #8754671 - Attachment is obsolete: true
Attachment #8754672 - Attachment is obsolete: true
This should have been fixed by bug 1274520.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.