Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 1 • 8 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

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 2 • 8 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.]

	Thomas Wisniewski Assignee
	Comment 3 • 8 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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 4 • 8 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 5 • 8 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.

	Thomas Wisniewski Assignee
	Comment 6 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 7 • 8 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?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 8 • 8 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.

	Thomas Wisniewski Assignee
	Comment 9 • 8 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)

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 10 • 8 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.

	Thomas Wisniewski Assignee
	Comment 12 • 8 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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 14 • 8 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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 8 years ago

s/others/other error messages/

	Thomas Wisniewski Assignee
	Comment 17 • 8 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.

	Daniel Holbert [:dholbert]
	Comment 18 • 8 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 19 • 8 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.

	Thomas Wisniewski Assignee
	Comment 20 • 8 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

	Thomas Wisniewski Assignee
	Comment 21 • 8 years ago

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

	Pulsebot
	Comment 22 • 8 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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 8 years ago
bugherder

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

	Kohei Yoshino [:kohei]
	Comment 24 • 8 years ago

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/

	Eric Shepherd [:sheppy]
	Comment 25 • 7 years ago

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.