1201798 - Hide viewport scrollbar for fullscreen without touching the overflow property

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Currently we hide the viewport scrollbar for fullscreen via specifying rule:
> :not(xul|*):root:-moz-full-screen-ancestor {
>   overflow: hidden !important;
> }
in ua.css.

However, we want to completely remove the :-moz-full-screen-ancestor pseudo-class and thus it is necessary to find an alternative.

We could add a new private selector for that, but making top layer be on top of the scrollbar is probably more conceptually perfect?

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Since change to "overflow" property triggers force frame reconstruction, changing it on the root for fullscreen likely slows down the overall time we need to enter fullscreen.

It seems we are able to paint the top layer over the scrollbar, so I think we should do so instead of relying on this rule.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

It seems WHATWG doesn't want to make the top layer be painted over the scrollbar, because they want to allow scrolling for <dialog>. But as we really want to get rid of :-moz-full-screen-ancestor pseudo-class, we need to find a way to hide the scrollbar without touching the overflow property.

Comment 3

[Markus Stange [:mstange]]

I'd like to suggest :not(xul|*):root::scrollbar { display: none; }. Not sure how serious I'm about that.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I don't think we have ::scrollbar pseudo-element, do we?

Also that way still needs a pseudo-class to select :root when we are in fullscreen.

I have patches on hand which manually overrides the scrollbar style for fullscreen. I'd submit those patches if editors agree with my proposal here: https://github.com/whatwg/fullscreen/issues/23#issuecomment-140944836

Anne, any thoughts about that?

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #4)
> I have patches on hand which manually overrides the scrollbar style for
> fullscreen. I'd submit those patches if editors agree with my proposal here:

I mean, submit patches for review.

Comment 6

[Anne (:annevk)]

It seems reasonable, but I defer to roc (and Hixie) when it comes to styling. It's not really my area.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. r=roc

Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. r=roc

Bug 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8665280 [details]
MozReview Request: Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. r=roc

https://reviewboard.mozilla.org/r/20199/#review18155

::: layout/base/nsPresContext.h:677
(Diff revision 1)
> +  nsIContent* PropagateScrollToViewport();

The corresponding change to nsPresContext.cpp seems to be missing!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8665281 [details]
MozReview Request: But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. r=roc

https://reviewboard.mozilla.org/r/20201/#review18173

This looks like a good approach, but if PropagateScrollToViewport gets called again after an element goes fullscreen (e.g. due to frame reconstruction of the entire document), do we keep the scrollbar properly hidden? It seems to me we would not.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> ::: layout/base/nsPresContext.h:677
> (Diff revision 1)
> > +  nsIContent* PropagateScrollToViewport();
> 
> The corresponding change to nsPresContext.cpp seems to be missing!

Good catch... Have no idea how can that happen...

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> 
> This looks like a good approach, but if PropagateScrollToViewport gets
> called again after an element goes fullscreen (e.g. due to frame
> reconstruction of the entire document), do we keep the scrollbar properly
> hidden? It seems to me we would not.

Yeah, I think you're right.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8665280 [details]
MozReview Request: Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. r=roc

Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8665281 [details]
MozReview Request: But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. r=roc

But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8665280 [details]
MozReview Request: Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. r=roc

https://reviewboard.mozilla.org/r/20199/#review18273

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8665281 [details]
MozReview Request: But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. r=roc

https://reviewboard.mozilla.org/r/20201/#review18275

You probably should add a test for that issue I mentioned where frame reconstruction interacts with fullscreen.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

There is one tricky edge case for this impl which has different behavior with the old style-based one:
* :root don't have "overflow" style, and
* <body> have "overflow: scroll", and
* request fullscreen on <body>

In this case, previously, <body> will have scrollbars because the "overflow: hidden" on :root is propagated to the viewport, so <body> keeps its style.

However, with this patch, the scroll style of <body> is instead propagated to the viewport, and thus <body> no longer has scrollbars, and the scrollbars on the viewport is also suppressed. (In this case, even if the scrollbars are not suppressed, they are useless as well, because the fullscreened <body> is fixed and has width and height which just fit the viewport.)

I'm not quite sure whether it is something desirable. Probably no.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

But I guess this behavior is probably spec-conforming, if we say "hide the scrollbar on viewport for fullscreen". Also, having "overflow" on <body> and requesting fullscreen on it is probably rare?

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8665280 [details]
MozReview Request: Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. r=roc

Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. r=roc

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8665281 [details]
MozReview Request: But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. r=roc

But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. r=roc

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1201798 part 3 - Add test for viewport scrollbar on fullscreen.

Bug 1201798 part 3 - Add test for viewport scrollbar on fullscreen.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8667168 [details]
MozReview Request: Bug 1201798 part 3 - Add test for viewport scrollbar on fullscreen.

https://reviewboard.mozilla.org/r/20685/#review18557

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=36edbbf6d1a4

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de8b305fb6f904932e587882c248f5eed88731cb
Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. r=roc

https://hg.mozilla.org/integration/mozilla-inbound/rev/df5ce21dade97b0703c742efcf15fbc15fbfd615
Bug 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. r=roc

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b090a69694cb058d9ea438ef68d0c8b1bbbb3fa
Bug 1201798 part 3 - Add test for viewport scrollbar on fullscreen. r=roc

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de8b305fb6f9
https://hg.mozilla.org/mozilla-central/rev/df5ce21dade9
https://hg.mozilla.org/mozilla-central/rev/8b090a69694c