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

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

Other elements should not be allowed to enter fullscreen per spec.

Comment 1

[Thomas Wisniewski]

Attachment: 1305928-only_allow_xul_svg_mathml_and_xhtml-namespaced_elements_to_requestFullscreen.diff

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

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.]

Comment 3

[Thomas Wisniewski]

Attachment: 1305928-only_allow_<math>_<svg>_xul_and_html_elements_to_requestFullscreen.diff

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 6

[Thomas Wisniewski]

Attachment: 1305928-part1-have_nsMathMLContainerFrame{AppendFrames,InsertFrames,RemoveFrame}_redirect_to_nsContainerFrame_for_kBackdropList.diff

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

Comment 7

[Thomas Wisniewski]

Attachment: 1305928-part2-only_allow_<math>_<svg>_xul_and_html_elements_to_requestFullscreen.diff

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?

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 9

[Thomas Wisniewski]

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)

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: part 1 - accept backdrop frame list in MathML block frame

Comment 12

[Thomas Wisniewski]

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.

Comment 13

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

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.

Comment 14

[Thomas Wisniewski]

>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

Comment 15

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

No need to add 'the' if others don't use it either.

Comment 16

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

s/others/other error messages/

Comment 17

[Thomas Wisniewski]

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 18

[Daniel Holbert [:dholbert]]

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

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 20

[Thomas Wisniewski]

Attachment: 1305928-part2-only_allow_<math>_<svg>_xul_and_html_elements_to_requestFullscreen.diff

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

Comment 21

[Thomas Wisniewski]

Attachment: 1305928-part2-only_allow_<math>_<svg>_xul_and_html_elements_to_requestFullscreen.diff

Here's a trivial rebase, just in case. Carrying over r+ and requesting check-in for both patches.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7935b34e3003
https://hg.mozilla.org/mozilla-central/rev/da5c067f0275

Comment 24

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/fullscreen-request-is-now-only-allowed-on-html-elements-svg-and-math/

Comment 25

[Eric Shepherd [:sheppy]]

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.