Closed Bug 1117579 Opened 5 years ago Closed 5 years ago
DOM fullscreen mode broken in freeciv online
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
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
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.
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+
Try run with that change is going at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a968f6a64f80 -- I'll land once that's done.
Patch with change requested above, for the record.
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?
Attachment #8544687 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Landed on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3f4db0105f79
I can verify with the last two nightly builds on Mac this is working again for my freeciv use case. Thanks!
You need to log in before you can comment on or make changes to this bug.