Hide viewport scrollbar for fullscreen without touching the overflow property

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
19 days ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

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

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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?
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Comment 2

3 years ago
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: 1126230
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.
(Assignee)

Comment 4

3 years ago
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)
(Assignee)

Comment 5

3 years ago
(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

3 years ago
It seems reasonable, but I defer to roc (and Hixie) when it comes to styling. It's not really my area.
Flags: needinfo?(annevk)
(Assignee)

Comment 7

3 years ago
Created 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)
(Assignee)

Comment 8

3 years ago
Created attachment 8665281 [details]
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.
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.
(Assignee)

Comment 11

3 years ago
(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.
(Assignee)

Comment 12

3 years ago
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)
(Assignee)

Comment 13

3 years ago
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.
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Comment 17

3 years ago
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?
(Assignee)

Comment 18

3 years ago
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
(Assignee)

Comment 19

3 years ago
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
(Assignee)

Comment 20

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

Bug 1201798 part 3 - Add test for viewport scrollbar on fullscreen.
Attachment #8667168 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 23

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/de8b305fb6f9
https://hg.mozilla.org/mozilla-central/rev/df5ce21dade9
https://hg.mozilla.org/mozilla-central/rev/8b090a69694c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

19 days ago
Depends on: 1479262
You need to log in before you can comment on or make changes to this bug.