Closed
Bug 1364025
Opened 7 years ago
Closed 6 years ago
Calling a function named fullscreen() from onclick handler fails with type error
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox53 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | disabled |
firefox56 | --- | wontfix |
firefox58 | --- | wontfix |
firefox59 | --- | wontfix |
firefox60 | --- | verified |
firefox61 | --- | verified |
People
(Reporter: vigo.von.harrach, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
(Keywords: regression, site-compat)
Attachments
(4 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170510113456 Steps to reproduce: Run the attached testcase in the current Nightly (55.0a1 (2017-05-10) (64-bit) on macOS 10.12.4), and click on the "click here" link. Actual results: Console reports "TypeError: fullscreen is not a function", and the function is not called. Expected results: The function should be called without errors. Changing the function's name from fullscreen() to e.g. xfullscreen() resolves the issue. So does calling the function from inside a <script>-block, or via (developer-)console. Regular FF (53) doesn't have this issue.
Updated•7 years ago
|
status-firefox53:
--- → unaffected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
Component: Untriaged → JavaScript Engine
Keywords: regression,
regressionwindow-wanted
Product: Firefox → Core
Version: 55 Branch → Trunk
Comment 1•7 years ago
|
||
Guess it's a DOM issue... There is the window.fullScreen property, and it's not longer case sensitive? https://developer.mozilla.org/en-US/docs/Web/API/Window/fullScreen
Component: JavaScript Engine → DOM
Comment 2•7 years ago
|
||
Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=117e8e24d7144071a15fd8a3c39e582614b81a4d&tochange=79d9809fed882064df4bf4978b289cc1c51ebda0
Updated•7 years ago
|
Assignee | ||
Comment 4•7 years ago
|
||
Ah, err... So the problem is that, we add an unprefixed fullscreen attribute to Document [1], because there are lots of code rely on this attribute. The attribute is currently behind pref "full-screen-api.unprefix.enabled" which is currently disabled for release / beta, but enabled for nightly. And what makes it worse here is that, inline event handler searches Document for name before searching Window [2]. I'm not sure what should be done here, as it reveals a new pattern of potential webcompat issue once we enable unprefixed fullscreen. Probably I should raise an issue to the spec to discuss this... vigo, do you see this issue on a real website? Or is it just from testing your own code? If no website really depends on this... we can probably close it as INVALID... otherwise, we probably need to do some further investigation. I guess we need to accept breakage one way or the other. And probably this case is less usual than accessing document.fullscreen, so we may have to live with this issue. cc Anne anyway. [1] https://fullscreen.spec.whatwg.org/#api [2] https://html.spec.whatwg.org/multipage/webappapis.html#getting-the-current-value-of-the-event-handler
Flags: needinfo?(xidorn+moz) → needinfo?(vigo.von.harrach)
Comment 5•7 years ago
|
||
I think we can fix this by adding the [Unscopable] extended attribute.
Assignee | ||
Comment 6•7 years ago
|
||
Hmmm, okay, I didn't know we have that... I guess we can add that to the Fullscreen API spec.
Comment 7•7 years ago
|
||
Created https://github.com/whatwg/fullscreen/pull/84.
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
> vigo, do you see this issue on a real website? Or is it just from testing > your own code? Both: the testcase is a reduced version of https://bloodyapp.sealmedia.de/webgl/seal.html (a Unity-based browser game), but the relevant code has been written by a colleague. Obviously, this is anecdotal evidence either way.
Flags: needinfo?(vigo.von.harrach)
Comment 9•7 years ago
|
||
Xidorn, now that the spec change has been merged, do we just need to update our WebIDL? (Sorry, I'm not sure if [Unscopable] will "just work".) Thanks!
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 10•7 years ago
|
||
I think so. But I suppose we would need a test for it as well (which should probably goes to web-platform-tests and fix w3c/web-platform-tests#5904). Since we are not going to enable unprefixed Fullscreen API at the moment, so it is not high priority. But we definitely want to fix this bug before we do that, so make it block that.
Blocks: 1269276
Flags: needinfo?(xidorn+moz)
Comment 11•7 years ago
|
||
This isn't riding to 55beta so preemptively updating status.
status-firefox56:
--- → affected
Comment 12•7 years ago
|
||
P2 -> fix-optional for 56 at this stage.
Comment 14•6 years ago
|
||
> But I suppose we would need a test for it as well I'm adding tests for @@unscopables to idlharness in bug 1437255. They catch this bug (which is why I ended up filing bug 1437265).
Depends on: 1437255
Comment 15•6 years ago
|
||
Also given comment 14, are there remaining reasons to not land this change?
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 16•6 years ago
|
||
Assuming you are having test for it... no, there is no reason not to land it.
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8950444 [details] Bug 1364025 - Add Unscopable to Document.fullscreen. https://reviewboard.mozilla.org/r/219706/#review225448 r=me. Thank you! You should probably merge on top of rev a6088a5f48ee (now on inbound) and remove the failure annotation for "Unscopable handled correctly for fullscreen property on Document" from testing/web-platform/meta/fullscreen/interfaces.html.ini
Attachment #8950444 -
Flags: review?(bzbarsky) → review+
Comment 19•6 years ago
|
||
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/221bee012f86 Add Unscopable to Document.fullscreen. r=bz
Comment 20•6 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4306093f2a12c73f733f013a04a845b003c286 for https://treeherder.mozilla.org/logviewer.html#?job_id=161882320&repo=mozilla-inbound for which Xidorn said to ni? you to tell us what the right thing to do is.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 21•6 years ago
|
||
So the issue here I think, is that payment-request/interfaces.https.html checks the IDL of Document, specifically, it checks the Document.prototype[@@unscopables]. However, it only checks dom.idl and payment-request.idl, so there is no definition of Document.fullscreen at all, and consequently it complains about unknown unscopable property. It feels like a test issue, but it seems to me this is somehow a common pattern for interfaces checking tests, and I have no idea what is the right way forward. Should we merge the interface of fullscreen.idl into dom.idl so that the tests would see the new unscopable? Or should we somehow fix the harness to avoid complaining about that?
Comment 22•6 years ago
|
||
> It feels like a test issue
It is. payment-request/interfaces.https.html isn't actually trying to test anything about dom.idl. It's just importing it so it has the definitions needed for payment-request.idl. So it should be using add_untested_idls(), not add_idls(), to add it. Similar for its EventHandler bits (and in fact, perhaps it should just be importing and adding via add_untested_idls /interfaces/html.idl instead of creating a duplicate and liable to go out of sync definition).
See, for example, how html/dom/interfaces.worker.js does things.
I'm happy to write a patch to this test, or review it, either way. Let me know please.
(The other option is to stop testing for unexpected unscopable properties, of course, but I'd rather not do that unless we really have to.)
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8950492 [details] Bug 1364025 followup - Fix Payment Request interface IDL test to not check dom.idl. https://reviewboard.mozilla.org/r/219770/#review225462 ::: testing/web-platform/tests/payment-request/interfaces.https.html:17 (Diff revision 1) > - const idlText = await fetch(url).then(r => r.text()); > - idlArray.add_idls(idlText); > - } > + const pr_idl = await fetch("/interfaces/payment-request.idl").then(r => r.text()); > + idlArray.add_untested_idls(dom_idl); > + idlArray.add_idls(pr_idl); > // typedef EventHandler from HTML > // https://html.spec.whatwg.org/#eventhandler > idlArray.add_idls(` This should become add_untested_idls.
Attachment #8950492 -
Flags: review?(bzbarsky) → review+
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•6 years ago
|
||
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8f43644be461 Add Unscopable to Document.fullscreen. r=bz https://hg.mozilla.org/integration/autoland/rev/d2a26faa3042 followup - Fix Payment Request interface IDL test to not check dom.idl. r=bz
Assignee | ||
Comment 29•6 years ago
|
||
testing/web-platform/tests/dom/interfaces.html fails for the same reason... Why adding a [Unscopable] is so difficult :(
Assignee | ||
Comment 30•6 years ago
|
||
Should we change `add_idls` in dom/interfaces.html to `add_untested_idls` as well? I have a feeling that we shouldn't since that test seems to be for testing that idl...
Flags: needinfo?(bzbarsky)
Comment 31•6 years ago
|
||
Backed out 2 changesets (bug 1364025) for wpt2 failures in /css/css-tables/table-model-fixup-2.html on a CLOSED TREE https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a41cadccb3ae2882dc486e722fd9f6dec15bf722&filter-classifiedState=unclassified https://hg.mozilla.org/integration/autoland/rev/a41cadccb3ae2882dc486e722fd9f6dec15bf722
Comment 32•6 years ago
|
||
No, in dom/interfaces.html we really do need to test things. I guess we could just remove the test that @@unscopables has no "extra" things... Given how things are set up right now with all the incomplete interfaces in various places in wpt I just don't see a sane way to make that test work sanely. :( I wonder whether James or Anne have any better ideas here.
Flags: needinfo?(james)
Flags: needinfo?(annevk)
Comment 33•6 years ago
|
||
We could also centralize the partials more. Philip has been looking into more IDL automation recently. Not sure what their current thinking is on this. Philip, the problem is that [Unscopable] is locally defined, but has a global effect, so unless you know about all partial interfaces the browser knows about, it's hard to test. I think Boris's solution of subset testing is probably the most reasonable intermediate step, but it does not seem entirely satisfactory indeed.
Flags: needinfo?(annevk) → needinfo?(philip)
Comment 34•6 years ago
|
||
idorn, this should solve your problems for the moment...
Flags: needinfo?(bzbarsky) → needinfo?(xidorn+moz)
Assignee | ||
Comment 35•6 years ago
|
||
Yes, that solves this problem here. Are you sending it to review?
Flags: needinfo?(xidorn+moz)
Comment 36•6 years ago
|
||
I think you can just land it. It's pretty self-explanatory. If you want, r=you would work too. ;)
Assignee | ||
Comment 37•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a280f99b6636c2e643c723afc6b49aee56ef4c7
Comment 38•6 years ago
|
||
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b24e3da0dba Remove some sanity-checks for unscopables, because idlharness is invoked with only fragmentary idl. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/8e54d2f48d4f Fix Payment Request interface IDL test to not check EventHandler. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/fd432ad90d28 Add Unscopable to Document.fullscreen. r=bz
Comment 39•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4b24e3da0dba https://hg.mozilla.org/mozilla-central/rev/8e54d2f48d4f https://hg.mozilla.org/mozilla-central/rev/fd432ad90d28
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
status-firefox58:
--- → wontfix
status-firefox59:
--- → wontfix
status-firefox-esr52:
--- → unaffected
Updated•6 years ago
|
Assignee: nobody → xidorn+moz
Comment 40•6 years ago
|
||
I Idon't have any better ideas, but I do agree we should consider automating all the idl stuff completely.
Flags: needinfo?(james)
Updated•6 years ago
|
QA Whiteboard: [good first verify]
Comment 41•6 years ago
|
||
I have reproduced this bug with Nightly 59.0a1 (2017-12-01) on Windows 10, 64 Bit! This bug's fix is verified with latest Beta! Build ID : 20180322152034 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
QA Whiteboard: [good first verify] → [good first verify] [bugday-20180321]
Comment 42•6 years ago
|
||
I have successfully reproduced this but on Firefox Nightly (2017-05-10) under macOS 10.12 using STR and attachment from Comment 0. The issue is no longer reproducible on Firefox 60.0b8 and latest Nightly (2018-04-01) under macOS 10.12, Ubnuntu 16.04 (x64) and Windows 10 (x64).
Updated•6 years ago
|
Keywords: dev-doc-needed
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Updated•4 years ago
|
Flags: needinfo?(philip)
You need to log in
before you can comment on or make changes to this bug.
Description
•