Closed Bug 1201798 Opened 4 years ago Closed 4 years ago

Hide viewport scrollbar for fullscreen without touching the overflow property

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files)

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?
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.
Blocks: 1187769
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.
No longer depends on: top-layer
Summary: Make top layer be painted on top of the viewport scrollbar → Hide viewport scrollbar for fullscreen without touching the overflow property
I'd like to suggest :not(xul|*):root::scrollbar { display: none; }. Not sure how serious I'm about that.
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?
Flags: needinfo?(annevk)
(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.
It seems reasonable, but I defer to roc (and Hixie) when it comes to styling. It's not really my area.
Flags: needinfo?(annevk)
Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method.
Attachment #8665280 - Flags: review?(roc)
Bug 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule.
Attachment #8665281 - Flags: review?(roc)
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!
Attachment #8665280 - Flags: review?(roc)
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.
(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 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.
Attachment #8665280 - Flags: review?(roc)
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.
Attachment #8665281 - Attachment description: MozReview Request: Bug 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. → MozReview Request: But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule.
Attachment #8665281 - Flags: review?(roc)
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
Attachment #8665280 - Flags: review?(roc) → review+
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.
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.
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 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
Attachment #8665280 - Attachment description: MozReview Request: Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. → MozReview Request: Bug 1201798 part 1 - Move PropagateScrollToViewport() from nsCSSFrameConstructor to nsPresContext as a public method. r=roc
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
Attachment #8665281 - Attachment description: MozReview Request: But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. → MozReview Request: But 1201798 part 2 - Update viewport scrollbar override for fullscreen and remove the leagcy css rule. r=roc
Bug 1201798 part 3 - Add test for viewport scrollbar on fullscreen.
Attachment #8667168 - Flags: review?(roc)
Assignee: nobody → quanxunzhen
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
Attachment #8667168 - Flags: review?(roc) → review+
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
Depends on: 1479262
You need to log in before you can comment on or make changes to this bug.