Closed
Bug 1117579
Opened 9 years ago
Closed 9 years ago
DOM fullscreen mode broken in freeciv online
Categories
(Core :: DOM: Core & HTML, defect)
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)
6.62 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
6.82 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Comment 1•9 years ago
|
||
[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
status-firefox35:
--- → unaffected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
tracking-firefox36:
--- → ?
tracking-firefox37:
--- → ?
Flags: needinfo?(vladimir)
Keywords: regressionwindow-wanted
Updated•9 years ago
|
OS: Mac OS X → All
Comment 2•9 years ago
|
||
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.
Comment 3•9 years ago
|
||
A backout for 36 would be the safest solution. I asked for a backout in bug 1036606 for 36.
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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-
Assignee | ||
Comment 6•9 years ago
|
||
Updated, along with tests.
Attachment #8544133 -
Attachment is obsolete: true
Attachment #8544196 -
Flags: review?(bzbarsky)
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
Patch with change requested above, for the record.
Assignee | ||
Comment 10•9 years ago
|
||
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?
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ce36368608
https://hg.mozilla.org/mozilla-central/rev/c3ce36368608
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•9 years ago
|
Attachment #8544687 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 13•9 years ago
|
||
Landed on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3f4db0105f79
Updated•9 years ago
|
Reporter | ||
Comment 15•9 years ago
|
||
I can verify with the last two nightly builds on Mac this is working again for my freeciv use case. Thanks!
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•