Closed Bug 743198 Opened 12 years ago Closed 8 years ago

Unprefix the DOM fullscreen API

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
blocking-kilimanjaro -
Tracking Status
firefox47 --- fixed

People

(Reporter: Ms2ger, Assigned: xidorn)

References

(Depends on 2 open bugs, Blocks 4 open bugs, )

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(11 files)

58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
... after fixing any spec conformance issues.
I'd love to unprefix the fullscreen API.

Differences are new stacking layer, ::backdrop pseudo-element, and some style changes.
Depends on: 746437
blocking-kilimanjaro: --- → ?
Keywords: dev-doc-needed
updating to the spec sounds non-trivial? Is that work likely to happen in the Firefox 15 cycle?
Yes, we need to implement some new features, it's probably not hard, but needs time.

I suspect I'll be press-ganged into implementing MPAPI plugins for mobile, so this probably won't make it into FF15.
not going to block k9o on un-prefixing but if there are full-screen capabilities missing that we need to have a successful app ecosystem, we'll block on those.
blocking-kilimanjaro: ? → -
Blocks: unprefix
I think we should consider supporting both the camel-cased and non-camel-cased variants of the unprefixed APIs, i.e., `requestFullscreen` (spec) `requestFullScreen` (prefixed APIs). 

One can easily find libraries [1][2] and snippets of code [2][3] that will break if Firefox ever drops support for the mozFullScreen* APIs.

[1] https://github.com/kayahr/jquery-fullscreen-plugin/blob/master/jquery.fullscreen.js
[2] https://github.com/eikes/jquery.fullscreen.js/blob/master/jquery.fullscreenslides.js
[3] http://stackoverflow.com/questions/1125084/how-to-make-in-javascript-full-screen-windows-stretching-all-over-the-screen/7525760#7525760
[4] http://stackoverflow.com/questions/19379153/google-maps-v3-requestfullscreen/19591838#19591838

I wrote a little bit about this at https://miketaylr.com/posts/2013/11/just-uppercase-everything.html. Sadly I think we're past the point of evangelizing the proper capitalization from the spec. :/
(In reply to Mike Taylor [:miketaylr] from comment #5)
> I think we should consider supporting both the camel-cased and
> non-camel-cased variants of the unprefixed APIs, i.e., `requestFullscreen`
> (spec) `requestFullScreen` (prefixed APIs). 

I think that's the wrong approach. Stick to the spec for the non-prefixed version, and keep the prefixed for a while (though SCREAM in the console!). Two variants won't fix the situation at all, it'll make it worse!
> Stick to the spec for the non-prefixed version, and keep the prefixed for a while (though SCREAM in the console!).

You'll have to keep the prefixed version forever. Deployed sites aren't going to look at a console warning and update anything.
We'll try it first. If that doesn't work out, we'll spec and implement the alias.
(In reply to Mike Taylor [:miketaylr] from comment #7)
> You'll have to keep the prefixed version forever. Deployed sites aren't
> going to look at a console warning and update anything.

Hook it up to telemetry, evangelize, and see if it works (within a reasonable timeframe, e.g. 1 year). Then decide about removing the prefixed and adding the aliased API.
(In reply to Florian Bender from comment #9)
> Hook it up to telemetry, 

Who make this happen? That sounds like a good idea to collect the extent of damage. Maybe also something that Seif/Hallvord and Stefan could be interested in.

> evangelize, 

There are two sides here: the Tech Evanglist team (or the a priori team) and the Web Compatibility team (or the a posteriori team), not the same type of public, actions. First one goes to conference and teach the fun things, the second one (mike, me and others) tries to unclog the pipe. Not. Easy. ;)


> and see if it works (within a
> reasonable timeframe, e.g. 1 year). Then decide about removing the prefixed
> and adding the aliased API.


:)
(In reply to Karl Dubost :karlcow from comment #10)
> There are two sides here: the Tech Evanglist team (or the a priori team) and
> the Web Compatibility team (or the a posteriori team), not the same type of
> public, actions. First one goes to conference and teach the fun things, the
> second one (mike, me and others) tries to unclog the pipe. Not. Easy. ;)

Either way, that needs to be part of the plan. ;) Only adding an alias because devs are lazy should be the last resort, even after seriously considering to Let It Break™ – that may just wake them up. This can be tested for a couple of cycles of Nightly + Aurora (+ Beta?, not propagating to Release!) so people report the problem while most users are not affected (this "only" needs a pref for the unprefixed API).
In any case, this bug needs to be fixed before we can even start to look at supporting aliases. Any takers?
Intent to implement and ship for blink with the specced (i.e. non-camelcased version) API: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GLl6aWs9-EM
Will anything break if we do the spec-compliance fixes and not keep the old behaviour, in either prefixed or unprefixed versions?  It seemed like some of the comments above are suggesting that we should keep all the current behaviour in the prefixed version; I don't think that's necessary.

Who can prioritize getting this bug fixed?  Seems like an easy win towards keeping us in "modern web API" land.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #15)

> Who can prioritize getting this bug fixed?  Seems like an easy win towards
> keeping us in "modern web API" land.

... and compatibility with Chrome. Woudn't it be helpful to fix this in sync with Blink crbug.com/383813 ?
The implementation needs to be updated. Thus the (now) standardized API may just be re-implement (reusing as much as possible of the existing implementation, of course) behind a new flag (or without one) and the existing API surface can live for another few releases (marked as deprecated) before it is disabled and (later) removed. Sounds like a plan?

Chris, any way we can put this on someone's agenda? The prefixing is a nuisance and source of mistakes. Since all other browsers ship the (prefixed, but) standardized version and at least Blink is on the verge of un-prefixing, it would be nice if Fx can update and unprefix soon!
Flags: needinfo?(cpearce)
Xidorn Quan has taken over fullscreen API work, and he will update and unprefix the API soon.
Flags: needinfo?(cpearce)
Awesome news, thanks!
I don't want to unprefix this API after I finish updating to the latest spec. Let it block on several individual bugs, and unprefix it eariler since IE/Edge has benn shipping an unprefixed API.
Depends on: top-layer, ::backdrop, 1199519, 1187801
No longer depends on: 746437
Depends on: 1212299
Depends on: 1212302
Depends on: 1215365
Bug 1199519 doesn't really matter. This is actually only blocked on backdrop.
No longer depends on: 1199519
I'm not quite sure whether I should use MozReview for this bug... I have 11 patches on hand, which means it could be an annoying spam when updating patches via MozReview. But it seems to me reviewer would benefit from the better comparison highlight MozReview provides for some of my pending patches.

As there are still some patches pending in the blocker bug 1064843, I guess I can hold my patches for a while, and hopefully MozReview would get improved soon. (It seems bug 1226028 has a good progress, so it could happen at some point.)
If you're going to ask review from me, use whatever is easiest to you. If I find MozReview too difficult or not useful, I'll just download the diffs and do old style reviewing.
(I don't use bugzilla's 'diff', but 'details')
Depends on: 1236979
This method actually never throws. IsFullScreenDoc() simply checks
whether GetFullScreenElement() returns nullptr, which means if that
method returns true, the "!el" check would never fail.

Review commit: https://reviewboard.mozilla.org/r/33607/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33607/
Attachment #8715675 - Flags: review?(bugs)
The main motivation of this patch is to fix the prefixed function name
used in one string. But updated string should have different a different
identifier, otherwise it might be ignored. Since we should eventually
prefer using word "fullscreen" over "full-screen", it is easier to just
change all of them together.

Review commit: https://reviewboard.mozilla.org/r/33627/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33627/
Attachment #8715685 - Flags: review?(bugs)
Attachment #8715681 - Flags: review?(cam) → review+
Comment on attachment 8715681 [details]
MozReview Request: Bug 743198 part 7 - Add :fullscreen pseudo class.

https://reviewboard.mozilla.org/r/33619/#review30219

::: layout/style/nsCSSPseudoClassList.h:164
(Diff revision 1)
>  CSS_STATE_PSEUDO_CLASS(mozFullScreen, ":-moz-full-screen", 0, "", NS_EVENT_STATE_FULL_SCREEN)

I assume we're not in a position to remove the prefixed version?
(In reply to Cameron McCormack (:heycam) (busy Jan 30 – Feb 6) (away Feb 8–9) from comment #37)
> Comment on attachment 8715681 [details]
> MozReview Request: Bug 743198 part 7 - Add :fullscreen pseudo class.
> 
> https://reviewboard.mozilla.org/r/33619/#review30219
> 
> ::: layout/style/nsCSSPseudoClassList.h:164
> (Diff revision 1)
> >  CSS_STATE_PSEUDO_CLASS(mozFullScreen, ":-moz-full-screen", 0, "", NS_EVENT_STATE_FULL_SCREEN)
> 
> I assume we're not in a position to remove the prefixed version?

No, I don't think we are. If we want to add the webkit-prefixed one, probably we can remove the prefixed version of ourselves' :)
Comment on attachment 8715675 [details]
MozReview Request: Bug 743198 part 1 - Drop [Throws] of document.mozFullScreenElement.

https://reviewboard.mozilla.org/r/33607/#review30729
Attachment #8715675 - Flags: review?(bugs) → review+
Comment on attachment 8715676 [details]
MozReview Request: Bug 743198 part 2 - Add unprefixed Fullscreen API to Document and Element.

https://reviewboard.mozilla.org/r/33609/#review30739
Attachment #8715676 - Flags: review?(bugs) → review+
Comment on attachment 8715677 [details]
MozReview Request: Bug 743198 part 3 - Refactor nsDocument::IsFullScreenEnabled to be static local function GetFullscreenError.

https://reviewboard.mozilla.org/r/33611/#review30751

::: dom/base/nsDocument.cpp:12030
(Diff revision 1)
> -  return IsFullScreenEnabled(nsContentUtils::IsCallerChrome(), false);
> +  return !GetFullscreenError(this, nsContentUtils::IsCallerChrome());

Interesting case where MozReview's "Moved to line" effectively just was making reading harder since there were anyhow many lines which weren't moved, but were new.
Attachment #8715677 - Flags: review?(bugs) → review+
Comment on attachment 8715678 [details]
MozReview Request: Bug 743198 part 4 - Rename LogFullScreenDenied to nsIDocument::DispatchFullscreenError, and reuse it in Element::RequestFullscreen.

https://reviewboard.mozilla.org/r/33613/#review30757

::: dom/base/nsDocument.cpp:11597
(Diff revision 1)
>  nsDocument::FullscreenElementReadyCheck(Element* aElement,

So this is a change to functionality. The old code just logged the error, the new one dispatches event.
Dispatch is right per the spec, at least when element isn't in doc or doesn't have window.
Please make sure we have some tests, at least for the '!IsInDoc()' case

Not sure about aElement->OwnerDoc() != this case.
The spec doesn't talk about that case here.
I filed https://github.com/whatwg/fullscreen/issues/33

::: dom/base/nsDocument.cpp:11615
(Diff revision 1)
>      return false;

oh we have more fullscreen errors. it is a bit
misleading that we have GetFullscreenError, yet also some other error types.
Perhaps some followup patch fixes this.
Attachment #8715678 - Flags: review?(bugs) → review+
Comment on attachment 8715679 [details]
MozReview Request: Bug 743198 part 5 - Check legacy event listener only if listener is found in the current group.

https://reviewboard.mozilla.org/r/33615/#review30773

It is not clear to me why we need or want this atm.
I assume you'll make moz* events use legacy stuff, but have you ensured addons aren't using those events in system group?
Comment on attachment 8715679 [details]
MozReview Request: Bug 743198 part 5 - Check legacy event listener only if listener is found in the current group.

(MozReview is silly. It doesn't always add any marks to the patches I've reviewed. Anyhow, please checks whether addons are using the events in system group, and if not, then reask review https://mxr.mozilla.org/addons/)
Attachment #8715679 - Flags: review-
Comment on attachment 8715680 [details]
MozReview Request: Bug 743198 part 6 - Add unprefixed fullscreen events.

https://reviewboard.mozilla.org/r/33617/#review30775
Attachment #8715680 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load) from comment #43)
> Comment on attachment 8715679 [details]
> MozReview Request: Bug 743198 part 5 - Check legacy event listener only if
> non-system listener is found.
> 
> https://reviewboard.mozilla.org/r/33615/#review30773
> 
> It is not clear to me why we need or want this atm.
> I assume you'll make moz* events use legacy stuff, but have you ensured
> addons aren't using those events in system group?

Because we may attach system listener on events, e.g. in screen orientation api impl.

Also, what addons do should normally not affect what we do on the content. And in this case, with the patch, addons would at most receive both unprefixed event and prefixed event. However if we don't do this, the content may not receive its prefixed event if some addon registers on an unprefixed one.
Comment on attachment 8715682 [details]
MozReview Request: Bug 743198 part 8 - Use unprefixed Fullscreen API in chrome code.

https://reviewboard.mozilla.org/r/33621/#review30777

::: browser/base/content/browser-fullScreen.js:68
(Diff revision 1)
>          document.documentElement.setAttribute("OSXLionFullscreen", true);

Could we remove nsDocument::IsFullScreenDoc() which does just fullscreenElement null check.
(or maybe some patch here does that)

It is a bit misleading to use different method in JS than in C++ to check whether we're in fullscreen.

::: mobile/android/chrome/content/browser.js:474
(Diff revision 1)
> -    window.addEventListener("mozfullscreenchange", function(e) {
> +    window.addEventListener("fullscreenchange", function(e) {

trying to recall why this works in case content calls event.stopPropagation().

Not about this bug though.
Attachment #8715682 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load) from comment #47)
> Comment on attachment 8715682 [details]
> MozReview Request: Bug 743198 part 8 - Use unprefixed Fullscreen API in
> chrome code.
> 
> ::: mobile/android/chrome/content/browser.js:474
> (Diff revision 1)
> > -    window.addEventListener("mozfullscreenchange", function(e) {
> > +    window.addEventListener("fullscreenchange", function(e) {
> 
> trying to recall why this works in case content calls
> event.stopPropagation().
> 
> Not about this bug though.

We probably should change this to be in the system group otherwise it may affect content compatibility.
(In reply to Xidorn Quan [:xidorn] (UTC+8) (less responsive until Feb 22) from comment #46)
> (In reply to Olli Pettay [:smaug] (high review load) from comment #43)
> > Comment on attachment 8715679 [details]
> > MozReview Request: Bug 743198 part 5 - Check legacy event listener only if
> > non-system listener is found.
> > 
> > https://reviewboard.mozilla.org/r/33615/#review30773
> > 
> > It is not clear to me why we need or want this atm.
> > I assume you'll make moz* events use legacy stuff, but have you ensured
> > addons aren't using those events in system group?
> 
> Because we may attach system listener on events, e.g. in screen orientation
> api impl.
So? I don't understand the comment.
I guess I don't understand what the patch is trying to do.

> 
> Also, what addons do should normally not affect what we do on the content.
> And in this case, with the patch, addons would at most receive both
> unprefixed event and prefixed event. However if we don't do this, the
> content may not receive its prefixed event if some addon registers on an
> unprefixed one.
Oh, you're trying achieve that. Well, usually addons don't use system event group, so I don't understand how the
patch helps with that case.

It is indeed an issue with the webkit prefixed stuff too, that if some addon uses non-prefixed listener on a target Foo, and content adds prefixed listeners on that Foo, those prefixed listeners won't be called.
Please file a separate bug for that and CC me and dholbert.
Comment on attachment 8715684 [details]
MozReview Request: Bug 743198 part 10 - Add test for prefixed Fullscreen API.

https://reviewboard.mozilla.org/r/33625/#review30781

mostly rs+ to this. You really like to use all the new fancy JS features ;)
Attachment #8715684 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load) from comment #49)
> (In reply to Xidorn Quan [:xidorn] (UTC+8) (less responsive until Feb 22)
> from comment #46)
> > (In reply to Olli Pettay [:smaug] (high review load) from comment #43)
> > > Comment on attachment 8715679 [details]
> > > MozReview Request: Bug 743198 part 5 - Check legacy event listener only if
> > > non-system listener is found.
> > > 
> > > https://reviewboard.mozilla.org/r/33615/#review30773
> > > 
> > > It is not clear to me why we need or want this atm.
> > > I assume you'll make moz* events use legacy stuff, but have you ensured
> > > addons aren't using those events in system group?
> > 
> > Because we may attach system listener on events, e.g. in screen orientation
> > api impl.
> So? I don't understand the comment.
> I guess I don't understand what the patch is trying to do.

This is trying to continue dispatching a prefixed fullscreen event if unprefixed listeners are found only in the system group.

> > Also, what addons do should normally not affect what we do on the content.
> > And in this case, with the patch, addons would at most receive both
> > unprefixed event and prefixed event. However if we don't do this, the
> > content may not receive its prefixed event if some addon registers on an
> > unprefixed one.
> Oh, you're trying achieve that. Well, usually addons don't use system event
> group, so I don't understand how the
> patch helps with that case.

This is probably the first step of doing this, and it is more important for fullscreen events for two reasons:
1. there are only two targets each document could receive this event: window and document, so content script and chrome code more likely listen on the same target than for transition events;
2. we internally listen to the events as well without any addons (at least in Fennec).

> It is indeed an issue with the webkit prefixed stuff too, that if some addon
> uses non-prefixed listener on a target Foo, and content adds prefixed
> listeners on that Foo, those prefixed listeners won't be called.
> Please file a separate bug for that and CC me and dholbert.

I'll file a separate bug, but given the second reason above, I think we should address at least the internal part of the issue inside this bug.
(In reply to Olli Pettay [:smaug] (high review load) from comment #50)
> Comment on attachment 8715684 [details]
> MozReview Request: Bug 743198 part 10 - Add test for prefixed Fullscreen API.
> 
> https://reviewboard.mozilla.org/r/33625/#review30781
> 
> mostly rs+ to this. You really like to use all the new fancy JS features ;)

Yeah, they are cool and help making code more compact so why not :)
Attachment #8715683 - Flags: review?(bugs) → review+
Comment on attachment 8715683 [details]
MozReview Request: Bug 743198 part 9 - Use unprefixed Fullscreen API in tests.

https://reviewboard.mozilla.org/r/33623/#review30785
(In reply to Xidorn Quan [:xidorn] (UTC+8) (less responsive until Feb 22) from comment #51)
> > So? I don't understand the comment.
> > I guess I don't understand what the patch is trying to do.
> 
> This is trying to continue dispatching a prefixed fullscreen event if
> unprefixed listeners are found only in the system group.
oh, finally I understand.
Ok, what about system event group case then?

> 
> This is probably the first step of doing this,
Not sure how this is the first step. Most of the time addons and chrome code uses non-system group.
(whether or not that is correct is different thing.)

> 
> I'll file a separate bug, but given the second reason above, I think we
> should address at least the internal part of the issue inside this bug.
Sure. But need to get the flag handling in HandleEventInternal right.

Don't we want
bool hasListenersForCurrentGroup;
...
hasListenersForCurrentGroup = hasListenersForCurrentGroup ||
                              listener->mFlags.mInSystemGroup == aEvent->mFlags.mInSystemGroup;

(I wonder if we could even update the flag inside if (listener->IsListening(aEvent) ...) {}. Tiny bit riskier.)
Comment on attachment 8715685 [details]
MozReview Request: Bug 743198 part 11 - Update locales string to the new spelling as well as unprefixed API.

https://reviewboard.mozilla.org/r/33627/#review30791
Attachment #8715685 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load) from comment #54)
> (I wonder if we could even update the flag inside if
> (listener->IsListening(aEvent) ...) {}. Tiny bit riskier.)
Yeah, based on what blink does, we don't want this.
https://reviewboard.mozilla.org/r/33613/#review30757

> So this is a change to functionality. The old code just logged the error, the new one dispatches event.
> Dispatch is right per the spec, at least when element isn't in doc or doesn't have window.
> Please make sure we have some tests, at least for the '!IsInDoc()' case
> 
> Not sure about aElement->OwnerDoc() != this case.
> The spec doesn't talk about that case here.
> I filed https://github.com/whatwg/fullscreen/issues/33

No, it doesn't change the functionality. The old "LogFullScreenDenied" dispatches event as well. This change is a simple refactor to make the name less misleading and reuse it in an additional place, and the behavior issue is actually unrelated to this bug.
Blocks: 1248505
https://reviewboard.mozilla.org/r/33621/#review30777

> Could we remove nsDocument::IsFullScreenDoc() which does just fullscreenElement null check.
> (or maybe some patch here does that)
> 
> It is a bit misleading to use different method in JS than in C++ to check whether we're in fullscreen.

Filed new bug 1248505 to address this.
Comment on attachment 8715675 [details]
MozReview Request: Bug 743198 part 1 - Drop [Throws] of document.mozFullScreenElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33607/diff/1-2/
Comment on attachment 8715676 [details]
MozReview Request: Bug 743198 part 2 - Add unprefixed Fullscreen API to Document and Element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33609/diff/1-2/
Comment on attachment 8715677 [details]
MozReview Request: Bug 743198 part 3 - Refactor nsDocument::IsFullScreenEnabled to be static local function GetFullscreenError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33611/diff/1-2/
Comment on attachment 8715678 [details]
MozReview Request: Bug 743198 part 4 - Rename LogFullScreenDenied to nsIDocument::DispatchFullscreenError, and reuse it in Element::RequestFullscreen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33613/diff/1-2/
Comment on attachment 8715679 [details]
MozReview Request: Bug 743198 part 5 - Check legacy event listener only if listener is found in the current group.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33615/diff/1-2/
Attachment #8715679 - Attachment description: MozReview Request: Bug 743198 part 5 - Check legacy event listener only if non-system listener is found. → MozReview Request: Bug 743198 part 5 - Check legacy event listener only if listener is found in the current group.
Attachment #8715679 - Flags: review- → review?(bugs)
Comment on attachment 8715680 [details]
MozReview Request: Bug 743198 part 6 - Add unprefixed fullscreen events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33617/diff/1-2/
Comment on attachment 8715681 [details]
MozReview Request: Bug 743198 part 7 - Add :fullscreen pseudo class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33619/diff/1-2/
Comment on attachment 8715682 [details]
MozReview Request: Bug 743198 part 8 - Use unprefixed Fullscreen API in chrome code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33621/diff/1-2/
Comment on attachment 8715683 [details]
MozReview Request: Bug 743198 part 9 - Use unprefixed Fullscreen API in tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33623/diff/1-2/
Comment on attachment 8715684 [details]
MozReview Request: Bug 743198 part 10 - Add test for prefixed Fullscreen API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33625/diff/1-2/
Comment on attachment 8715685 [details]
MozReview Request: Bug 743198 part 11 - Update locales string to the new spelling as well as unprefixed API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33627/diff/1-2/
Attachment #8715679 - Flags: review?(bugs) → review+
Comment on attachment 8715679 [details]
MozReview Request: Bug 743198 part 5 - Check legacy event listener only if listener is found in the current group.

https://reviewboard.mozilla.org/r/33615/#review31643
https://hg.mozilla.org/integration/mozilla-inbound/rev/4616af79a640b3ce933ee9a807106e5926000057
Bug 743198 part 1 - Drop [Throws] of document.mozFullScreenElement. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/04c9910699f7eb883a66b17396a38c3056366fcd
Bug 743198 part 2 - Add unprefixed Fullscreen API to Document and Element. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8afaafd19faf81538822b0e224c153d919eade
Bug 743198 part 3 - Refactor nsDocument::IsFullScreenEnabled to be static local function GetFullscreenError. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ecb187ce925f72b7780f793157979be9722930d
Bug 743198 part 4 - Rename LogFullScreenDenied to nsIDocument::DispatchFullscreenError, and reuse it in Element::RequestFullscreen. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd1694d9700f850abe035f0c12935126cf01ba6d
Bug 743198 part 5 - Check legacy event listener only if listener is found in the current group. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/5210ded8bd67518ac2dae60f381d84682a421230
Bug 743198 part 6 - Add unprefixed fullscreen events. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/a72b6cfcb2e1e07dfcb80a7c4928fa15a8bb22c2
Bug 743198 part 7 - Add :fullscreen pseudo class. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/55ecf767b62d74c1337d3209e9df13aed1205c92
Bug 743198 part 8 - Use unprefixed Fullscreen API in chrome code. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/9508a74b97eeae06d3a38470d1a1e2438399a5f2
Bug 743198 part 9 - Use unprefixed Fullscreen API in tests. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/c997bcd70010037289c96debde95f370ee6ef45c
Bug 743198 part 10 - Add test for prefixed Fullscreen API. rs=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c07ed7d69124c3ff846dab68b96aad907a433d6
Bug 743198 part 11 - Update locales string to the new spelling as well as unprefixed API. r=smaug
Assignee: nobody → quanxunzhen
(In reply to Kohei Yoshino [:kohei] from comment #73)
> Posted the site compatibility doc:
> https://www.fxsitecompat.com/en-CA/docs/2016/fullscreen-api-has-been-unprefixed/

One correction:

Since the Fullscreen API has stayed in prefixed form for an extended time, and its spelling was changed from "FullScreen" to "Fullscreen", I don't think we are able to ever remove the prefixed API without affect web compatibility significantly.

Given this, at most
> prefixed methods and properties ... will be removed in the future.
should be
> prefixed methods and properties ... **may** be removed in the future.

We are not considering removing it at all at this point, actually.
Depends on: 1249225
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #74)
> > prefixed methods and properties ... will be removed in the future.
> should be
> > prefixed methods and properties ... **may** be removed in the future.

Thanks, updated the compat doc.
Blocks: 1249225
No longer depends on: 1249225
Blocks: 1255669
Depends on: 1260102
No longer depends on: 1260102
Depends on: 1268749
Depends on: 1268794
Depends on: 1268797
Depends on: 1268798
Depends on: 1269157
Blocks: 1269276
Depends on: 1269929
Resetting the dev-doc-needed, I just found bug 1268749 that indicates that these changes are not in Firefox 47 finally.
I've added the compatibility info to https://developer.mozilla.org/en-US/docs/Web/CSS/:fullscreen.

Sebastian
Depends on: 1307745
Depends on: 1401877
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: