Fullscreen request should only be allowed for HTML element, <svg>, and <math>

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: xidorn, Assigned: wisniewskit)

Tracking

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

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

()

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

3 years ago
Other elements should not be allowed to enter fullscreen per spec.
Priority: -- → P3
Assignee

Comment 1

3 years ago
Here's a patch to make it so. Note that I'm also allowing XUL, as it seems necessary in Firefox's case. Try seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5af7dd7ef4c
Attachment #8795997 - Flags: review?(ehsan)
Reporter

Comment 2

3 years ago
Comment on attachment 8795997 [details] [diff] [review]
1305928-only_allow_xul_svg_mathml_and_xhtml-namespaced_elements_to_requestFullscreen.diff

Review of attachment 8795997 [details] [diff] [review]:
-----------------------------------------------------------------

It's wrong. Per spec, only HTML elements, and <svg> and <math> should be allowed. Other SVG elements and MathML elements should not be allowed either.

The condition should be something like
> !aElement->IsHTMLElement() && !aElement->IsSVGElement(nsGkAtoms::svg) && !aElement->IsMathMLElement(nsGkAtoms::math)

[Although I cannot give r+, but I think I can definitely r- any fullscreen-related patch :) You may want to ask smaug to review fullscreen changes, as he reviewed most of my changes to fullscreen, so he's probably more familiar with this stuff than ehsan.]
Attachment #8795997 - Flags: review?(ehsan) → review-
Reporter

Updated

3 years ago
Summary: Fullscreen request should only be allowed for HTML element, <svg>, and <mathml> → Fullscreen request should only be allowed for HTML element, <svg>, and <math>
Assignee

Comment 3

3 years ago
Ah yes, you're right. Thanks for checking! Here's a new version which uses that logic instead.

Unfortunately it turns out that there's a bug elsewhere that causes a test failure; trying to requestFullscreen on a <math> element triggers an assertion:

>[Child 28382] ###!!! ASSERTION: unexpected frame list: 'aListID == kPrincipalList', file /media/ssd/mozilla/central/mozilla-central/layout/mathml/nsMathMLContainerFrame.h, line 429
>#01: nsFrameConstructorState::ConstructBackdropFrameFor(nsIContent*, nsIFrame*) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:1238)
>#02: nsFrameConstructorState::AddChild(nsIFrame*, nsFrameItems&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, bool, bool, bool, nsIFrame*) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:1291)
>#03: nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:3924)
>#04: nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:6132)
>#05: nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, nsFrameItems&) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:10544 (discriminator 2))
>#06: nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, bool) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:7452)
>#07: nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:7097)
>#08: nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:7100)
>#09: nsCSSFrameConstructor::CreateNeededFrames() (/media/ssd/mozilla/central/mozilla-central/layout/base/nsCSSFrameConstructor.cpp:7118)
>#10: mozilla::RestyleManager::ProcessPendingRestyles() (/media/ssd/mozilla/central/mozilla-central/layout/base/RestyleManager.cpp:796)
>#11: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (/media/ssd/mozilla/central/mozilla-central/layout/base/nsPresShell.cpp:4116)
(Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af59e8e2ade&selectedJob=28268661)

I won't have time to figure out right away, but figured I'd at least post the updated patch for now.
Attachment #8795997 - Attachment is obsolete: true
Reporter

Comment 4

3 years ago
nsMathMLContainerFrame::{AppendFrames,InsertFrames,RemoveFrame} need to redirect the call to nsContainerFrame if aListID is kBackdropList.
Reporter

Comment 5

3 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> nsMathMLContainerFrame::{AppendFrames,InsertFrames,RemoveFrame} need to
> redirect the call to nsContainerFrame if aListID is kBackdropList.

This should be in a separate patch, FWIW.
Assignee

Comment 6

3 years ago
Sure, here's a patch to do that (assuming that removing the assertion is safe once that redirection is added).
Assignee

Comment 7

3 years ago
And here's a revised version of the first patch.

A try run with both applied seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4df94338f94

Can you think of any other issues to look into, or should I request review?
Attachment #8796351 - Attachment is obsolete: true
Reporter

Comment 8

3 years ago
Oh, sorry, I was wrong. You should do the redirection only for SetInitialChild. Doing this in the other 3 methods is actually unnecessary.
Assignee

Comment 9

3 years ago
Hmm, sorry for dragging you into this, but that doesn't seem sufficient :)

With this as the code:

>  SetInitialChildList(ChildListID     aListID,
>                      nsFrameList&    aChildList) override
>  {
>    if (aListID == kPrincipalList) {
>      nsContainerFrame::SetInitialChildList(aListID, aChildList);
>    } else {
>      nsBlockFrame::SetInitialChildList(aListID, aChildList);
>    }
>    // re-resolve our subtree to set any mathml-expected data
>    nsMathMLContainerFrame::RebuildAutomaticDataForChildren(this);
>  }

I'm getting a lot of failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ec44a5ababa&selectedJob=28359200

For instance, for the web platform test: compat/webkit-text-fill-color-property-004.html

> 0:07.36 PROCESS_OUTPUT: ProcessReader (pid:16980) "[Child 17028] ###!!! ASSERTION: aForFrame not found in block, someone lied to us: 'isValid', file /media/ssd/mozilla/central/mozilla-central/layout/generic/nsTextFrame.cpp, line 1418"
> 0:07.36 PROCESS_OUTPUT: ProcessReader (pid:16980) "[Child 17028] ###!!! ASSERTION: Someone lied to us about the block: 'backIterator.GetContainer() == block', file /media/ssd/mozilla/central/mozilla-central/layout/generic/nsTextFrame.cpp, line 1420"

Do you suppose it would be sufficient if I also change the methods you suggested as well? If this warrants more investigation, I'll have to familiarize myself with the code a bit more before I can update the patch.

Also, I'm guessing that I ought to be changing both the block and inline versions of the classes, just in case? (nsMathMLmathInlineFrame as well as nsMathMLmathBlockFrame)
Reporter

Comment 10

3 years ago
(In reply to Thomas Wisniewski from comment #9)
> With this as the code:
> 
> >  SetInitialChildList(ChildListID     aListID,
> >                      nsFrameList&    aChildList) override
> >  {
> >    if (aListID == kPrincipalList) {
> >      nsContainerFrame::SetInitialChildList(aListID, aChildList);
> >    } else {
> >      nsBlockFrame::SetInitialChildList(aListID, aChildList);
> >    }

Apparently you are doing wrong thing in this code. You should not skip nsBlockFrame if the aListID is kPrincipalList. I'll submit a patch for this part.
Assignee

Comment 12

3 years ago
Comment on attachment 8796723 [details] [diff] [review]
1305928-part2-only_allow_<math>_<svg>_xul_and_html_elements_to_requestFullscreen.diff

Alright, I guess I might as well request review for the other patch as well.
Attachment #8796723 - Flags: review?(bugs)
Assignee

Updated

3 years ago
Attachment #8796722 - Attachment is obsolete: true
Comment on attachment 8796723 [details] [diff] [review]
1305928-part2-only_allow_<math>_<svg>_xul_and_html_elements_to_requestFullscreen.diff

"Request for fullscreen was denied because requesting element is not <svg>, <math>, or an HTML element."

Should it be '...the requesting element...'?


I assume you tested that other browsers limit fullscreen the same way.
Attachment #8796723 - Flags: review?(bugs) → review+
Assignee

Comment 14

3 years ago
>Should it be '...the requesting element...'?

I was just matching the pre-existing fullscreen error-messages, which don't have the "the" in them. However, I don't mind adding it to all of the similar messages, if that's preferable?


>I assume you tested that other browsers limit fullscreen the same way.

Not yet, but the ones which do not have this restriction are the ones with only a prefixed implementation of requestFullscreen. You can see the related spec-discussion here (it all seems reasonable to me): https://github.com/whatwg/fullscreen/issues/22
Flags: needinfo?(bugs)
No need to add 'the' if others don't use it either.
Flags: needinfo?(bugs)
s/others/other error messages/
Assignee

Comment 17

3 years ago
Just in case, by "not yet" I meant that the prefixed-only implementations don't yet support this restriction, not that I haven't yet tested whether they do.
Comment on attachment 8796783 [details] [diff] [review]
part 1 - accept backdrop frame list in MathML block frame

Review of attachment 8796783 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

I don't really know our mathml layout code, but this seems minimal & reasonable enough; r=me
Attachment #8796783 - Flags: review?(dholbert) → review+
Reporter

Comment 19

3 years ago
FWIW, the element type check has been moved out from fullscreen element ready check, because type of element is not mutatable, so we only need to check it once. (Fullscreen element ready check is run twice)

You may want to update the patch to follow that change as well. That isn't a big deal, though.
Assignee

Comment 20

3 years ago
No problem. Here's a new version of the patch which just moves the logic out into the RequestFullscreen method as per spec. I'm not sure if that's a change warranting a re-review, so I requested one (Sorry smaug! Only the nsDocument.cpp changes were affected between the two versions).

I'm also doing a fresh try run just in case (for both patches, of course): https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a5a4014caad7d3eae5254921311312c784a8e14
Attachment #8796723 - Attachment is obsolete: true
Attachment #8797726 - Flags: review?(bugs)
Attachment #8797726 - Flags: review?(bugs) → review+
Assignee

Comment 21

3 years ago
Here's a trivial rebase, just in case. Carrying over r+ and requesting check-in for both patches.
Assignee: nobody → wisniewskit
Attachment #8797726 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 22

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7935b34e3003
part 1 - Accept backdrop frame list in MathML container frame. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5c067f0275
Part 2: Only allow <svg>, <math>, XUL, and HTML elements to requestFullscreen. r=smaug
Keywords: checkin-needed

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7935b34e3003
https://hg.mozilla.org/mozilla-central/rev/da5c067f0275
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Articles updated:

https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullScreen
https://developer.mozilla.org/en-US/Firefox/Releases/52

This should be fully updated in the docs now; please let me know if I’m mistaken.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.