Closed Bug 1117579 Opened 5 years ago Closed 5 years ago

DOM fullscreen mode broken in freeciv online

Categories

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

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- unaffected
firefox36 + fixed
firefox37 + fixed

People

(Reporter: asa, Assigned: vlad)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Latest Firefox Mac nightly builds (and going back for at least a month or two) the DOM fullscreen mode is not working in the freeciv online game. 

Steps to reproduce: 

1. visit http://play.freeciv.org/ and start a new game.
2. switch to the game's "Options" tab and click the fullscreen button.

Results: nothing happens.

Expected results: game is put into fullscreen mode. 

Additional Info: This used to work in Firefox Mac and continues to work in Chrome on Mac.
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e5326d6771d3&tochange=c080f66ed9d2

Suspect Bug 1036606
Blocks: 1036606
Flags: needinfo?(vladimir)
OS: Mac OS X → All
mozRequestFullscreen is being called with the single JS argument |false|, which is not convertible to a dictionary.

The caller looks like this:

  /5\.1[\.\d]* Safari/.test(navigator.userAgent) ?
    a[z.request]() :
    a[z.request](k&&Element.ALLOW_KEYBOARD_INPUT)

where z.request is "mozRequestFullScreen", and k is false.  This is basically the same code pattern as in bug 1106351, albeit on a different site.  I expect this means it's fairly common in the wild, whether through copy/paste or a library, so we really need to back our bug 1036606 or modify the IDL it uses to allow any argument and ignore the non-convertible-to-dictionary ones or something.
A backout for 36 would be the safest solution. I asked for a backout in bug 1036606 for 36.
WebKit got to a non-standard extension to this before us.  This patch makes us more permissive, ignoring anything but a dictionary with the options that we want.

I'm trying to find some existing fullscreen tests so that I can extend them to test that bogus arguments are ignored.
Assignee: nobody → vladimir
Flags: needinfo?(vladimir)
Attachment #8544133 - Flags: review?(bzbarsky)
Comment on attachment 8544133 [details] [diff] [review]
be more permissive in requestFullScreen arguments

>+Element::MozRequestFullScreen(JSContext* cx, JS::Handle<JS::Value> options)

aCx, aOptions.

>+      fsOptions.Init(cx, options))

This can throw on cx, and if so that needs to propagate out to the binding.  That means you want to make this method [Throws] in the IDL, add an ErrorResult argument, and Throw(NS_ERROR_FAILURE) on it and return immediately if fxOptions.Init() returns false.

Alternately, you could JS_ClearPendingException(cx) if we just want to entirely suppress exceptions when initing the dictionary.  But that seems like a slightly odd behavior.
Attachment #8544133 - Flags: review?(bzbarsky) → review-
Updated, along with tests.
Attachment #8544133 - Attachment is obsolete: true
Attachment #8544196 - Flags: review?(bzbarsky)
Comment on attachment 8544196 [details] [diff] [review]
be more permissive in requestFullScreen arguments (v2)

>+  void MozRequestFullScreen(JSContext* aCx, JS::Handle<JS::Value> aOptions,

Maybe document that null aCx is allowed as long as aOptions is undefined?  And assert in the method that if !aCx then aOptions.isUndefined()?

r=me with that
Attachment #8544196 - Flags: review?(bzbarsky) → review+
Comment on attachment 8544687 [details] [diff] [review]
be more permissive in requestFullScreen arguments (v2.1)

Landing this on trunk as soon as the tree reopens.

Approval Request Comment
[Feature/regressing bug #]: 1036606
[User impact if declined]: Failures entering fullscreen on various sites, due to them passing bogus args to requestFullScreen
[Describe test coverage new/current, TBPL]: new tests cover passing bogus args and make sure they're ignored as possible; try run on trunk above
[Risks and why]: Very few risks; code is pretty isolated and is now set up to ignore bogus args (as before).
[String/UUID change made/needed]: none
Attachment #8544687 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c3ce36368608
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8544687 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1106351
I can verify with the last two nightly builds on Mac this is working again for my freeciv use case. Thanks!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.