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

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla49
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active, URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
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.
Assignee: nobody → bugzilla
Whiteboard: btpp-active
(Assignee)

Comment 2

2 years ago
(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.
(Assignee)

Comment 3

2 years ago
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

2 years ago
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.
(Assignee)

Comment 5

2 years ago
No, there is no real rush.
(Assignee)

Updated

2 years ago
Blocks: 1269276
(Assignee)

Comment 6

2 years ago
Created attachment 8749127 [details]
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/
Attachment #8749127 - Flags: review?(khuey)
Attachment #8749128 - Flags: review?(khuey)
Attachment #8749129 - Flags: review?(khuey)
Attachment #8749130 - Flags: review?(bugs)
(Assignee)

Comment 7

2 years ago
Created attachment 8749128 [details]
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/
(Assignee)

Comment 8

2 years ago
Created attachment 8749129 [details]
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/
(Assignee)

Comment 9

2 years ago
Created attachment 8749130 [details]
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 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().
Flags: needinfo?(bugzilla)
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+.
Attachment #8749130 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

2 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(bugzilla)
(Assignee)

Comment 13

2 years ago
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 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
Attachment #8749127 - Flags: review?(khuey) → review+
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
Attachment #8749128 - Flags: review?(khuey) → review+
Comment on attachment 8749129 [details]
MozReview Request: Bug 1268798 part 3 - Add LenientSetter extended attribute. r?khuey

https://reviewboard.mozilla.org/r/50761/#review48659
Attachment #8749129 - Flags: review?(khuey) → review+
(Assignee)

Comment 17

2 years ago
Thanks for reviewing!

Comment 20

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.