Add a pref to disable unprefixed Fullscreen API

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
6 days ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla49
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47+ fixed, firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(7 attachments, 1 obsolete attachment)

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
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
3.76 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Microsoft recently reached out to us and talked about some issues they found with unprefixed Fullscreen API, and the plan that they are going to disable that by default with the next Edge release.

I discussed with Olli and Ehsan about this offline today, and they think Beta is not the right place for us to investigate this kind of breakage, and we should step back and disable it by default for Beta and Release, then wait for further information.

This bug is for adding the pref and disable it for Beta and Release.
(Assignee)

Comment 1

3 years ago
[Tracking Requested - why for this release]: Because this is the release we initially unprefix this API.
status-firefox46: --- → unaffected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox47: --- → ?
Keywords: dev-doc-needed, site-compat
(Assignee)

Updated

3 years ago
Summary: Add a pref to disable unprefixed Fullscreen API and disable it on non-release versions → Add a pref to disable unprefixed Fullscreen API
(Assignee)

Updated

3 years ago
Blocks: 1268797
Whiteboard: btpp-active
(Assignee)

Updated

3 years ago
Blocks: 1269276
(Assignee)

Comment 8

3 years ago
try push looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9db611754533

Note that this try push commit disables unprefixed Fullscreen API by default (ignoring the version), so it should be more similiar to what we would see in release versions.
(Assignee)

Comment 9

3 years ago
Comment on attachment 8747614 [details]
MozReview Request: Bug 1268749 part 1 - Add pref to disable unprefixed fullscreen api. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49963/diff/1-2/
(Assignee)

Comment 10

3 years ago
Comment on attachment 8747615 [details]
MozReview Request: Bug 1268749 part 2 - Make pseudo-classes able to present conditionally like properties. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49965/diff/1-2/
(Assignee)

Comment 11

3 years ago
Comment on attachment 8747616 [details]
MozReview Request: Bug 1268749 part 3 - Hide :fullscreen pseudo-class from content when unprefixed API is disabled. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49967/diff/1-2/
(Assignee)

Comment 12

3 years ago
Comment on attachment 8747617 [details]
MozReview Request: Bug 1268749 part 4 - Hide unprefixed Fullscreen API from content when disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49969/diff/1-2/
(Assignee)

Comment 13

3 years ago
Comment on attachment 8747618 [details]
MozReview Request: Bug 1268749 part 5 - Avoid dispatching unprefixed fullscreen events to content if unprefixed API is disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49971/diff/1-2/
(Assignee)

Comment 14

3 years ago
Comment on attachment 8747619 [details]
MozReview Request: Bug 1268749 part 6 - Add test for behavior when unprefixed api is disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49973/diff/1-2/
Attachment #8747614 - Flags: review?(bugs) → review+
Comment on attachment 8747614 [details]
MozReview Request: Bug 1268749 part 1 - Add pref to disable unprefixed fullscreen api. r=smaug

https://reviewboard.mozilla.org/r/49963/#review46989
Comment on attachment 8747617 [details]
MozReview Request: Bug 1268749 part 4 - Hide unprefixed Fullscreen API from content when disabled. r=smaug

https://reviewboard.mozilla.org/r/49969/#review46991
Attachment #8747617 - Flags: review?(bugs) → review+
Comment on attachment 8747618 [details]
MozReview Request: Bug 1268749 part 5 - Avoid dispatching unprefixed fullscreen events to content if unprefixed API is disabled. r=smaug

https://reviewboard.mozilla.org/r/49971/#review46995

Those fixed, and _tested_, r+

::: dom/events/EventListenerManager.cpp:288
(Diff revision 2)
>    listener->mTypeAtom = aTypeAtom;
>    listener->mFlags = aFlags;
>    listener->mListenerIsHandler = aHandler;
>    listener->mHandlerIsString = false;
>    listener->mAllEvents = aAllEvents;
> +  listener->mIsChrome = NS_IsMainThread() &&

NS_IsMainThread() check is a bit slow, and you have that information already in mIsMainThreadELM member variable in EventListenerManager, so use that please.

::: dom/events/EventListenerManager.cpp:289
(Diff revision 2)
>    listener->mFlags = aFlags;
>    listener->mListenerIsHandler = aHandler;
>    listener->mHandlerIsString = false;
>    listener->mAllEvents = aAllEvents;
> +  listener->mIsChrome = NS_IsMainThread() &&
> +                        (!nsContentUtils::GetCurrentJSContext() ||

I would use here LegacyIsCallerChromeOrNativeCode

::: dom/events/EventListenerManager.cpp:685
(Diff revision 2)
>      if (mIsMainThreadELM) {
>        return aListener->mTypeAtom == aEvent->mSpecifiedEventType;
>      }
>      return aListener->mTypeString.Equals(aEvent->mSpecifiedEventTypeString);
>    }
> +  if (!nsContentUtils::IsUnprefixedFullscreenApiEnabled() &&

This breaks dispatching fullscreenchange or fullscreenerror using scripts in the page, or more exactly, this breaks page to handle them.

But adding a check that if event isn't trusted, then listener should be called, then this should be fine.

So, before the nsContentUtils check,  add
aEvent->IsTrusted() &&

Also, please test at least manually that dispatching those events from web page works.
Attachment #8747618 - Flags: review?(bugs) → review+
Comment on attachment 8747619 [details]
MozReview Request: Bug 1268749 part 6 - Add test for behavior when unprefixed api is disabled. r=smaug

https://reviewboard.mozilla.org/r/49973/#review46997

::: dom/html/test/file_fullscreen-unprefix-disabled-inner.html:69
(Diff revision 2)
> +  document.removeEventListener("mozfullscreenerror", errorFullscreen);
> +  // Wait a short time before exiting this test to confirm that there is
> +  // really no unwanted event gets dispatched.
> +  setTimeout(() => opener.finish(), 200);
> +}
> +

Please add a check to this test that dispatching fullscreenchange and fullscreenerror events from scripts and getting listeners called still works.
Attachment #8747619 - Flags: review?(bugs) → review+
Comment on attachment 8747615 [details]
MozReview Request: Bug 1268749 part 2 - Make pseudo-classes able to present conditionally like properties. r=heycam

https://reviewboard.mozilla.org/r/49965/#review47139

We should have similar static_asserts to those in nsCSSProps.cpp that check that if ENABLED_IN_CHROME is set, ENABLED_IN_UA_SHEETS is also set.

::: layout/style/nsCSSParser.cpp:5917
(Diff revision 2)
>    CSSPseudoElementType pseudoElementType =
>      nsCSSPseudoElements::GetPseudoType(pseudo);
>    CSSPseudoClassType pseudoClassType =
> -    nsCSSPseudoClasses::GetPseudoType(pseudo);
> +    nsCSSPseudoClasses::GetPseudoType(pseudo, AgentRulesEnabled(),
> +                                      ChromeRulesEnabled());

I think having the method take two booleans can make it hard to understand what the arguments are, and makes it easier to get the order of argument passing wrong.

I think we should be consistent with the way we check for CSS properties being enabled.  So instead of using two booleans here, we should use nsCSSProps::EnabledState (maybe lifted out to a top-level type).  And we should probably use the same design for nsCSSPeudoElements::GetPseudoType too.

This can be a followup.

::: layout/style/nsCSSPseudoClasses.h:13
(Diff revision 2)
>  #ifndef nsCSSPseudoClasses_h___
>  #define nsCSSPseudoClasses_h___
>  
>  #include "nsStringFwd.h"
>  
> -// This pseudo-class is accepted only in UA style sheets.
> +// The following two flags along with the pref defines where the this

s/where the/where/ (I see that typo in nsCSSProps.h, too).
Attachment #8747615 - Flags: review?(cam) → review+
Comment on attachment 8747616 [details]
MozReview Request: Bug 1268749 part 3 - Hide :fullscreen pseudo-class from content when unprefixed API is disabled. r=heycam

https://reviewboard.mozilla.org/r/49967/#review47143
Attachment #8747616 - Flags: review?(cam) → review+
(Assignee)

Updated

3 years ago
Blocks: 1269975
(Assignee)

Comment 21

3 years ago
Comment on attachment 8747614 [details]
MozReview Request: Bug 1268749 part 1 - Add pref to disable unprefixed fullscreen api. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49963/diff/2-3/
Attachment #8747614 - Attachment description: MozReview Request: Bug 1268749 part 1 - Add pref to disable unprefixed fullscreen api. r?smaug → MozReview Request: Bug 1268749 part 1 - Add pref to disable unprefixed fullscreen api. r=smaug
Attachment #8747615 - Attachment description: MozReview Request: Bug 1268749 part 2 - Make pseudo-classes able to present conditionally like properties. r?heycam → MozReview Request: Bug 1268749 part 2 - Make pseudo-classes able to present conditionally like properties. r=heycam
Attachment #8747616 - Attachment description: MozReview Request: Bug 1268749 part 3 - Hide :fullscreen pseudo-class from content when unprefixed API is disabled. r?heycam → MozReview Request: Bug 1268749 part 3 - Hide :fullscreen pseudo-class from content when unprefixed API is disabled. r=heycam
Attachment #8747617 - Attachment description: MozReview Request: Bug 1268749 part 4 - Hide unprefixed Fullscreen API from content when disabled. r?smaug → MozReview Request: Bug 1268749 part 4 - Hide unprefixed Fullscreen API from content when disabled. r=smaug
Attachment #8747618 - Attachment description: MozReview Request: Bug 1268749 part 5 - Avoid dispatching unprefixed fullscreen events to content if unprefixed API is disabled. r?smaug → MozReview Request: Bug 1268749 part 5 - Avoid dispatching unprefixed fullscreen events to content if unprefixed API is disabled. r=smaug
Attachment #8747619 - Attachment description: MozReview Request: Bug 1268749 part 6 - Add test for behavior when unprefixed api is disabled. r?smaug → MozReview Request: Bug 1268749 part 6 - Add test for behavior when unprefixed api is disabled. r=smaug
(Assignee)

Comment 22

3 years ago
Comment on attachment 8747615 [details]
MozReview Request: Bug 1268749 part 2 - Make pseudo-classes able to present conditionally like properties. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49965/diff/2-3/
(Assignee)

Comment 23

3 years ago
Comment on attachment 8747616 [details]
MozReview Request: Bug 1268749 part 3 - Hide :fullscreen pseudo-class from content when unprefixed API is disabled. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49967/diff/2-3/
(Assignee)

Comment 24

3 years ago
Comment on attachment 8747617 [details]
MozReview Request: Bug 1268749 part 4 - Hide unprefixed Fullscreen API from content when disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49969/diff/2-3/
(Assignee)

Comment 25

3 years ago
Comment on attachment 8747618 [details]
MozReview Request: Bug 1268749 part 5 - Avoid dispatching unprefixed fullscreen events to content if unprefixed API is disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49971/diff/2-3/
(Assignee)

Comment 26

3 years ago
Comment on attachment 8747619 [details]
MozReview Request: Bug 1268749 part 6 - Add test for behavior when unprefixed api is disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49973/diff/2-3/
(Assignee)

Comment 27

3 years ago
https://reviewboard.mozilla.org/r/49965/#review47139

> I think having the method take two booleans can make it hard to understand what the arguments are, and makes it easier to get the order of argument passing wrong.
> 
> I think we should be consistent with the way we check for CSS properties being enabled.  So instead of using two booleans here, we should use nsCSSProps::EnabledState (maybe lifted out to a top-level type).  And we should probably use the same design for nsCSSPeudoElements::GetPseudoType too.
> 
> This can be a followup.

Followup filed bug 1269975 and bug 1269976.
(Assignee)

Comment 28

3 years ago
Comment on attachment 8747615 [details]
MozReview Request: Bug 1268749 part 2 - Make pseudo-classes able to present conditionally like properties. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49965/diff/3-4/
(Assignee)

Comment 29

3 years ago
Comment on attachment 8747616 [details]
MozReview Request: Bug 1268749 part 3 - Hide :fullscreen pseudo-class from content when unprefixed API is disabled. r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49967/diff/3-4/
(Assignee)

Comment 30

3 years ago
Comment on attachment 8747617 [details]
MozReview Request: Bug 1268749 part 4 - Hide unprefixed Fullscreen API from content when disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49969/diff/3-4/
(Assignee)

Comment 31

3 years ago
Comment on attachment 8747618 [details]
MozReview Request: Bug 1268749 part 5 - Avoid dispatching unprefixed fullscreen events to content if unprefixed API is disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49971/diff/3-4/
(Assignee)

Comment 32

3 years ago
Comment on attachment 8747619 [details]
MozReview Request: Bug 1268749 part 6 - Add test for behavior when unprefixed api is disabled. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49973/diff/3-4/
(Assignee)

Comment 35

3 years ago
Comment on attachment 8747614 [details]
MozReview Request: Bug 1268749 part 1 - Add pref to disable unprefixed fullscreen api. r=smaug

Approval Request Comment
[Feature/regressing bug #]: bug 743198, unprefix Fullscreen API
[User impact if declined]: may see webcompat issue (see the blockers of bug 1269276)
[Describe test coverage new/current, TreeHerder]: new test is added for the behavior when unprefixed fullscreen api is disabled
[Risks and why]: this is a non-trivial change, so it could be somehow risky.
[String/UUID change made/needed]: n/a
Attachment #8747614 - Flags: approval-mozilla-beta?
Attachment #8747614 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 36

3 years ago
All six patches are needed to uplift.
(Assignee)

Comment 37

3 years ago
The webcompat issues are also tracked in:
* https://github.com/webcompat/web-bugs/issues/2498 (could be mitigated by bug 1268794)
* https://github.com/webcompat/web-bugs/issues/2499 (need website fix)
and some other related spec changes are in:
* https://github.com/whatwg/fullscreen/issues/2 (bug 1268798)
* https://github.com/whatwg/fullscreen/issues/38 (bug 1269157)
Comment on attachment 8747614 [details]
MozReview Request: Bug 1268749 part 1 - Add pref to disable unprefixed fullscreen api. r=smaug

Let's put this in aurora 48 in preparation for possible uplift to beta.
Sounds like it is important that we be able to test disabling this api on beta as soon as we can.
Attachment #8747614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Part 1 applied cleanly.
Part 2 hit the following:

grafting 341511:04e6d36ccc44 "Bug 1268749 part 2 - Make pseudo-classes able to present conditionally like properties. r=heycam"
merging layout/inspector/inDOMUtils.cpp                                                                                                                   
merging layout/style/nsCSSParser.cpp                                                                                                                  
merging layout/style/nsCSSPseudoClassList.h                                                                                                           
merging layout/style/nsCSSPseudoClasses.cpp                                                                                                           
merging layout/style/nsCSSPseudoClasses.h                                                                                                             
warning: conflicts while merging layout/inspector/inDOMUtils.cpp! (edit, then use 'hg resolve--mark')                                                                         
warning: conflicts while merging layout/style/nsCSSParser.cpp! (edit, then use 'hg resolve --mark')                                                                            
warning: conflicts while merging layout/style/nsCSSPseudoClasses.cpp! (edit, then use 'hg resolve --mark')                                                                     
abort: unresolved conflicts, can't continue 


Not sure if any of the other patches need rebasing after that.
Flags: needinfo?(bugzilla)
(Assignee)

Comment 40

3 years ago
I'll look into this next Monday. I wonder whether I can push to mozilla-aurora directly myself? It feels to me submitting the rebased patchset for aurora and beta isn't very efficient.
Flags: needinfo?(bugzilla) → needinfo?(wkocher)
(Assignee)

Comment 41

3 years ago
I shouldn't have cancelled my ni?...
Flags: needinfo?(bugzilla)
You can probably carry forward the the a+ and land them yourself if you'd prefer.
Flags: needinfo?(wkocher)
(Assignee)

Updated

3 years ago
status-firefox48: affected → fixed
(Assignee)

Comment 44

3 years ago
The conflict is from a renaming in bug 1250820.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #43)
> Pulsebot does not log changesets automatically for pushes to mozilla-aurora?
Correct. Landing to aurora usually means status flags need to get changed, and pulsebot isn't smart enough to try setting those, so just doesn't bother commenting at all.
Comment on attachment 8747614 [details]
MozReview Request: Bug 1268749 part 1 - Add pref to disable unprefixed fullscreen api. r=smaug

Recent regression with webcompat as a possible impact, let's uplift to Beta47 soon so we can test early and fix issues that we may find during the rest of this cycle, Beta47+
Attachment #8747614 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
tracking-firefox47: ? → +
Part 2 of what you pushed to aurora doesn't apply to beta. Do we need another reworked patch?
(Assignee)

Comment 48

3 years ago
I can do that again.
Flags: needinfo?(bugzilla)
(Assignee)

Comment 49

3 years ago
This time it is conflict because of bug 1025267. More specifically, the predecessor of ENABLED_IN_CHROME flag for pseudo-classes was not implemented at all before that bug.

Could we probably uplift that to beta as well? Uplifting this patch without that gets landed sounds like a mess.
Flags: needinfo?(rkothari)
Flags: needinfo?(mats)
I don't see a problem with that.  I've asked for beta approval in bug 1025267.
Flags: needinfo?(mats)
(Assignee)

Comment 51

3 years ago
Also, could you cc me in that bug so that I know when does that get uplifted? Or alternatively, could you comment in this bug when your patch gets uplifted there?
Flags: needinfo?(bugzilla) → needinfo?(mats)
Sure, CC:ed you on that bug.
Flags: needinfo?(mats)
(Assignee)

Comment 53

3 years ago
Thanks.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #49)
> This time it is conflict because of bug 1025267. More specifically, the
> predecessor of ENABLED_IN_CHROME flag for pseudo-classes was not implemented
> at all before that bug.
> 
> Could we probably uplift that to beta as well? Uplifting this patch without
> that gets landed sounds like a mess.

Sure. I've approved that one for uplift to Beta.
Flags: needinfo?(rkothari)
(Assignee)

Updated

3 years ago
Depends on: 1025267
(Assignee)

Comment 59

3 years ago
I experience various compile error when I tried to build with VS2015 locally. I wonder whether we should uplift all of those bugs to make it easier for people with VS2015 to uplift patches to Firefox 47.
(Assignee)

Comment 60

3 years ago
Which are bug 1254963, bug 1266614, bug 1266615, and the followup commit in bug 1251225. All of them are just some trivial changes which should not hurt any behavior.
(Assignee)

Comment 61

3 years ago
Looks like there is a new permafail after this patchset lands in beta.
(Assignee)

Comment 62

3 years ago
Two new permafails: Mn-e10s on various platforms, and M(rc2) on Android.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #62)
> Two new permafails: Mn-e10s on various platforms, and M(rc2) on Android.

Mn-e10s is practically permafailing even before you landed these, at least on opt/pgo builds: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&fromchange=f07d961162adaf999dcaffc8c06bebfe8c5b8067&group_state=expanded&selectedJob=1076147&filter-searchStr=mn-e10s

Can't help you with the rc2 bustage, though...
(Assignee)

Comment 64

3 years ago
ok, I guess rc2 is my fault, and that could fail when the current aurora turns to beta as well. It seems my try push in comment 8 didn't cover the android-specific tests.
(Assignee)

Comment 65

3 years ago
The issue with Android M(rc2) is that, Element.matches(":fullscreen") throws in chrome code at https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#808

Initially I thought that we can probably check whether caller is chrome in CSS parser so that we do not return invalid for :fullscreen there. But it doesn't seem to be the right way, because nsINode has a selector cache which stores parsed selectors, which means a query from chrome may change the result for content. And checking the document's URI doesn't work for this specific case because in this cause, the target in question is in content.

So I decided to avoid querying :fullscreen inside the chrome code to fix this issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out in https://hg.mozilla.org/releases/mozilla-beta/rev/cf7b0c21b647 until that rc2 permafail can be fixed.
Comment on attachment 8751256 [details] [diff] [review]
followup - Avoid querying :fullscreen selector for context menu

Redirecting
Attachment #8751256 - Flags: review?(mark.finkle) → review?(margaret.leibovic)
Comment on attachment 8751256 [details] [diff] [review]
followup - Avoid querying :fullscreen selector for context menu

Review of attachment 8751256 [details] [diff] [review]:
-----------------------------------------------------------------

You can land this if you leave the SelectorContext bit in, but if you make any of the other suggested changes, I'm happy to re-review.

::: mobile/android/chrome/content/browser.js
@@ -2318,5 @@
> -            return aElt.matches(aSelector);
> -          return false;
> -        }
> -      };
> -    },

This API is used by a number of add-ons:
https://mxr.mozilla.org/addons/search?string=.selectorcontext%28&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=addons
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/contextmenus

So you should leave this. But you can add a comment explaining that it is necessary for add-ons, so that nobody else tries to remove it in the future.

@@ +2483,5 @@
> +          }
> +          return false;
> +        }
> +      };
> +    },

You should update the documentation here to include this new context type:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/contextmenus/add

You should also add documentation in code comments here to explain what values are supported for `aMode`, since it looks like this will always return false if a mode isn't specified.

Why not just default to returning true for any video that is matched, if a mode isn't specified, rather than requiring this "any" parameter?
Attachment #8751256 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 71

3 years ago
Attachment #8751256 - Attachment is obsolete: true
Attachment #8751532 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 72

3 years ago
(In reply to :Margaret Leibovic from comment #70)
> You should update the documentation here to include this new context type:
> https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/
> NativeWindow/contextmenus/add

I'll update this document once it lands. Add ni? myself for this.
Flags: needinfo?(bugzilla)
(Assignee)

Comment 73

3 years ago
Comment on attachment 8751532 [details] [diff] [review]
followup - Avoid querying :fullscreen selector for context menu

Carry your r+. I'm assuming you are fine with the changes you suggested. If you think there is any problem in this patch, feel free to ping me.
Flags: needinfo?(bugzilla) → needinfo?(margaret.leibovic)
Attachment #8751532 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 74

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa6df3f85dca80d04ca10c25f7f5f4743880b2b
Bug 1268749 followup - Avoid querying :fullscreen selector for context menu. r=margaret
(Assignee)

Comment 76

3 years ago
I don't think I canceled my own ni? for comment 72...
Flags: needinfo?(bugzilla)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #73)
> Comment on attachment 8751532 [details] [diff] [review]
> followup - Avoid querying :fullscreen selector for context menu
> 
> Carry your r+. I'm assuming you are fine with the changes you suggested. If
> you think there is any problem in this patch, feel free to ping me.

This looks good, thanks.
Flags: needinfo?(margaret.leibovic)

Comment 78

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7fa6df3f85dc
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 81

3 years ago
It seems there are lots of other contexts which are not documented in that page... Added the newly-added "videoContext" there, but I suppose others also need to be documented, so ni? Margaret again.
Flags: needinfo?(bugzilla) → needinfo?(margaret.leibovic)
(Assignee)

Comment 83

3 years ago
Oops, I forgot to correct the bug number when reland... sorry
(Assignee)

Comment 84

3 years ago
Errr... I didn't noticed the depends on item added...
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #81)
> It seems there are lots of other contexts which are not documented in that
> page... Added the newly-added "videoContext" there, but I suppose others
> also need to be documented, so ni? Margaret again.

Any improved documentation is a win. I don't have a strong opinion here.

The future for add-ons should really be web extensions, so if something isn't supported in web extensions, I'm not too keen on adding "official" support for it.
Flags: needinfo?(margaret.leibovic)

Comment 87

3 years ago
(In reply to Jean-Yves Perrier [:teoli] from comment #86)
> Updated:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/47#Others


https://developer.mozilla.org/en-US/Firefox/Releases/47#InterfacesAPIsDOM

"The DOM fullscreen API has been unprefixed" needs an update too
Keywords: dev-doc-complete → dev-doc-needed
Good catch, I removed the sentence as it is a dup of the other comment. Thanks
Keywords: dev-doc-needed → dev-doc-complete

Updated

3 years ago
Depends on: 1280545
Depends on: 1285418
I've added the compatibility info to https://developer.mozilla.org/en-US/docs/Web/CSS/:fullscreen.

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