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)

defect

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)

Attached file testzor.html
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.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Version: 55 Branch → Trunk
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
I imagine this is intentional, right, Xidorn?
Flags: needinfo?(xidorn+moz)
Blocks: 1269157
Has Regression Range: --- → yes
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)
I think we can fix this by adding the [Unscopable] extended attribute.
Hmmm, okay, I didn't know we have that... I guess we can add that to the Fullscreen API spec.
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)
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)
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)
This isn't riding to 55beta so preemptively updating status.
P2 -> fix-optional for 56 at this stage.
Blocks: 746437
See Also: → 1401877
> 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
Also given comment 14, are there remaining reasons to not land this change?
Flags: needinfo?(xidorn+moz)
Assuming you are having test for it... no, there is no reason not to land it.
Flags: needinfo?(xidorn+moz)
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+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/221bee012f86
Add Unscopable to Document.fullscreen. r=bz
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?
> 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 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+
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
testing/web-platform/tests/dom/interfaces.html fails for the same reason...

Why adding a [Unscopable] is so difficult :(
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)
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)
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)
idorn, this should solve your problems for the moment...
Flags: needinfo?(bzbarsky) → needinfo?(xidorn+moz)
Yes, that solves this problem here. Are you sending it to review?
Flags: needinfo?(xidorn+moz)
I think you can just land it.  It's pretty self-explanatory.  If you want, r=you would work too.  ;)
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
Assignee: nobody → xidorn+moz
I Idon't have any better ideas, but I do agree we should consider automating all the idl stuff completely.
Flags: needinfo?(james)
QA Whiteboard: [good first verify]
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]
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).
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
Flags: needinfo?(philip)
You need to log in before you can comment on or make changes to this bug.