1268798 - Give fullscreenElement and fullscreenEnabled attributes a dummy setter rather than making them readonly

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

There is a pattern like
> document.fullscreenElement = document.fullscreenElement || document.mozFullScreenElement || ...;
and
> document.fullscreenEnabled = document.fullscreenEnabled || document.mozFullScreenEnabled || ...;

This would throw in strict mode. We may want to give them a dummy setter rather than marking them readonly so that they would never throw.

Comment 1

[Mike Taylor [:miketaylr]]

(vaguely reminds me of https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/5S7qeLSXT5Q)

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mike Taylor [:miketaylr] from comment #1)
> (vaguely reminds me of
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/5S7qeLSXT5Q)

So removing prefixed API would be another breaking change... I think we don't have plan to remove them currently, but we may eventually want to do that, or get something speced, as stated in bug 1249225.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

heycam added a "[LenientSetter]" extended attribute to WebIDL, which we may want to use.

But it seems to me implementing this could be non-trivial, and we may not want to uplift this patch. It means we'll not be able to ship unprefixed Fullscreen API for another cycle.

Comment 4

[Anne (:annevk)]

Xidorn, we could implement it as a no-op setter for now and use "any" in IDL and add support for [LenientSetter] as a cleanup later. If that's easier to uplift. But I suppose there's no real rush either.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

No, there is no real rush.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1268798 part 1 - Fix exception whitelist in WebIDL parser. r?khuey

Review commit: https://reviewboard.mozilla.org/r/50757/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50757/

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1268798 part 2 - Add result summary for WebIDL parser test. r?khuey

Review commit: https://reviewboard.mozilla.org/r/50759/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50759/

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1268798 part 3 - Add LenientSetter extended attribute. r?khuey

Review commit: https://reviewboard.mozilla.org/r/50761/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50761/

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1268798 part 4 - Mark Document.fullscreenElement and fullscreenEnabled with LenientSetter. r?smaug

Review commit: https://reviewboard.mozilla.org/r/50763/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50763/

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8749127 [details]
MozReview Request: Bug 1268798 part 1 - Fix exception whitelist in WebIDL parser. r?khuey

Please explain why you want to use repr() instead of str().

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8749130 [details]
MozReview Request: Bug 1268798 part 4 - Mark Document.fullscreenElement and fullscreenEnabled with LenientSetter. r?smaug

https://reviewboard.mozilla.org/r/50763/#review48155

Assuming [LenientSetter] is implemented correctly in other patches, r+.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> Please explain why you want to use repr() instead of str().

When I ran runtests.py manually, I get
> Rule 'OtherOrComma' defined, but not used
exception which is in the whitelist.

Then I investigate a bit, and found that the format string uses `%r` rather than `"%s"`, so I changed that. Later I found that when this file is called from the build system, it is still use `"%s"`. I guess that is because I installed a newer version of ply or something locally, while the build system uses an in-tree dependency.

I think `%r` actually makes more sense here, as it would escape the string properly, and save two bytes in the code :) And given the newer version uses that, I expect we may want to switch to that at some point as well.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

khuey, part 1 and 2 are not necessary to this bug, so if you are not comfortable with them, I can just skip them and land part 3 and part 4.

I need part 1 to run tests locally, and part 2 is to make unexpected result in tests listed more explicitly after finish testing. Neither of them affects adding [LenientSetter] extended attribute.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8749127 [details]
MozReview Request: Bug 1268798 part 1 - Fix exception whitelist in WebIDL parser. r?khuey

https://reviewboard.mozilla.org/r/50757/#review48655

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8749128 [details]
MozReview Request: Bug 1268798 part 2 - Add result summary for WebIDL parser test. r?khuey

https://reviewboard.mozilla.org/r/50759/#review48657

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8749129 [details]
MozReview Request: Bug 1268798 part 3 - Add LenientSetter extended attribute. r?khuey

https://reviewboard.mozilla.org/r/50761/#review48659

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Thanks for reviewing!

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d79c89578a9

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf82e81dd0e4cf9597d36563f65149b25be45e39
Bug 1268798 part 1 - Fix exception whitelist in WebIDL parser. r=khuey

https://hg.mozilla.org/integration/mozilla-inbound/rev/020c9ed2224f9c074974cfbd05a5b8a1934879c6
Bug 1268798 part 2 - Add result summary for WebIDL parser test. r=khuey

https://hg.mozilla.org/integration/mozilla-inbound/rev/e84a5823b6082794edec9644fbba489cf4fcf841
Bug 1268798 part 3 - Add LenientSetter extended attribute. r=khuey

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2de04de7164db3dcb0f3c18394dd9092a8c1610
Bug 1268798 part 4 - Mark Document.fullscreenElement and fullscreenEnabled with LenientSetter. r=smaug

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf82e81dd0e4
https://hg.mozilla.org/mozilla-central/rev/020c9ed2224f
https://hg.mozilla.org/mozilla-central/rev/e84a5823b608
https://hg.mozilla.org/mozilla-central/rev/d2de04de7164

Comment 21

[Jean-Yves Perrier [:teoli]]

Added a comment in:
https://developer.mozilla.org/en-US/docs/Web/API/Document/fullscreenElement
https://developer.mozilla.org/en-US/docs/Web/API/Document/fullscreenEnabled
and
https://developer.mozilla.org/en-US/Firefox/Releases/49#Others

I also documented this in this meta-page about how to use webidl to document things properly:
https://developer.mozilla.org/en-US/docs/MDN/Contribute/Howto/Write_an_API_reference/Information_contained_in_a_WebIDL_file#Not_throwing_exceptions