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.

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.

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

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/

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

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).

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.

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?

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.

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

This should have been fixed by bug 1274520.