fix interaction of viewport units (vh,vw,vmin,vmax) with scrollbars

RESOLVED FIXED in mozilla23

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
11 months ago

People

(Reporter: dbaron, Assigned: seth)

Tracking

({dev-doc-complete})

Trunk
mozilla23
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=CSS])

Attachments

(3 attachments, 6 obsolete attachments)

Around October 27 there was a discussion at the Test the Web Forward event about vh/vw/vmin/vmax units (as specified) not having the correct interaction with scrollbars that authors expect.

We implemented what the spec says, but we should probably figure out whether we're ok shipping these units with this issue as things are.
(Assignee)

Comment 1

6 years ago
Not sure about the specific discussion you refer to here, but I think in general the problems I've seen mentioned relate to the fact that the presence or absence of a scrollbar affects the viewport size. I think the OS X Lion+ / iOS / Windows 8 / Windows Phone / Android / Ubuntu Unity overlay scrollbars nicely solve this problem. Imagine we just standard on this type of scrollbar on all platforms, especially since that's clearly the trend moving forward; do the issues go away?
The issue was indeed that 100vw doesn't fit in the width of the viewport if there's a scrollbar, and it does indeed go away if we switch to scrollbars that don't occupy space on all platforms.  But I don't think we've successfully switched on any platforms yet, though we need to.
(Assignee)

Comment 3

6 years ago
Created attachment 699920 [details]
Viewport units / scrollbar interaction test.

I think there are two issues here, actually:

- Should 100vw take into account the scrollbar? (My intuition: yes. Anything else will be contrary to web dev's expectations.)
- How to handle a dependency cycle involving the scrollbar's presence or absence depending on the values of viewport units.

Here's a test that checks both of these things informally.
(Assignee)

Comment 4

6 years ago
I checked this test in the newest release of Safari (6.0.2). It doesn't take scrollbars into account at all when computing viewport units. (That is: 100vw is the full width of the viewport, as if the scrollbar wasn't there.)
(Assignee)

Comment 5

6 years ago
Chrome (23.0.1271.101) is the same, unsurprisingly.
(Assignee)

Comment 6

6 years ago
Same behavior in IE (10.0.9200.16466).
(Assignee)

Comment 7

6 years ago
So it's pretty clear what the other browser vendors have decided on. Is this what we want too? It certainly does simplify things, though it has some clear downsides (for example: making an image the full width of the viewport will result in some of it being offscreen if a scrollbar is present).

I have an idea for how we could handle the dependency cycle issue cleanly, but it's irrelevant if we're happy with 100vw ignoring the existence of scrollbars.
(Assignee)

Comment 8

6 years ago
Created attachment 700043 [details]
Viewport units / scrollbar interaction test.

Noticed a typo in the original test, though I don't think this matters as it would only make a difference if the user agents took the scrollbars into account when computing the viewport units, and none of them seem to.
Attachment #699920 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 700086 [details]
Viewport units / scrollbar interaction test.

Whoops! Uploaded the wrong file.
Attachment #700043 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
I doublechecked and I still see the same behavior in Safari. (BTW, this is also the same behavior that we exhibit right now.)
Added dev-doc-needed, we should improve our <length> doc with this case (once fixed on our side).
Keywords: dev-doc-needed
The working group resolved that the scrollbar widths *should* be subtracted for overflow:scroll, but should not be subtracted for overflow:auto.  This discussion is minuted (though not very clearly) in:
http://lists.w3.org/Archives/Public/www-style/2013Jan/0616.html

So this is ready to fix, and we should probably do so sooner rather than later.
(Assignee)

Comment 13

5 years ago
Will jump on this ASAP.
(Assignee)

Updated

5 years ago
Assignee: nobody → seth
(Assignee)

Updated

5 years ago
Attachment #700086 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 725702 [details] [diff] [review]
(Part 1) - Test for viewport unit interactions with overflow scroll and auto.

Here are reftests that check for the committee's recommended behavior. Note that we already pass for the 'overflow: auto' case, because our current implementation ignores the presence of scrollbars in all cases. As expected, we fail the 'overflow: scroll' test.
Attachment #725702 - Flags: review?(dbaron)
Comment on attachment 725702 [details] [diff] [review]
(Part 1) - Test for viewport unit interactions with overflow scroll and auto.

I think you should also add a test with overflow-x:scroll;overflow-y:auto and the reverse.

It's also a perfectly good thing to check in failing tests with the "fails" annotation, and then remove the "fails" annotation when the patch to fix them lands.

Also, given the complexity of the clientWidth stuff, it might be worth throwing a != test in there.

r=dbaron
Attachment #725702 - Flags: review?(dbaron) → review+
(Assignee)

Comment 16

5 years ago
Thanks for the review!

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #15)
> I think you should also add a test with overflow-x:scroll;overflow-y:auto
> and the reverse.

Yeah, that's a good idea. I'll add those.

> It's also a perfectly good thing to check in failing tests with the "fails"
> annotation, and then remove the "fails" annotation when the patch to fix
> them lands.

Sounds good. Then I can land part 1 (once your comments are addressed) without waiting for the actual implementation to run through try and so forth.

> Also, given the complexity of the clientWidth stuff, it might be worth
> throwing a != test in there.

Presumably you mean a check that the overflow: auto case produces different results from the overflow: scroll case? I'll add that too.
(In reply to Seth Fowler [:seth] from comment #16)
> Presumably you mean a check that the overflow: auto case produces different
> results from the overflow: scroll case? I'll add that too.

Yep.  Probably worth checking visually that it's only the expected difference.
(Assignee)

Comment 18

5 years ago
Created attachment 725742 [details] [diff] [review]
(Part 2) - Take scrollbars into account for viewport units if 'overflow: scroll' is set.

A draft of the actual implementation. This _would_ be working fine, except that it doesn't work until the window is resized. When the values for the viewport units are initially calculated, bodyElem->ClientWidth() and similar methods return 0. I haven't found anything that will let me obtain the size of a scrollbar at that time, either. (Neither the root element nor the body element of the document have a scroll frame attached at that point, for example.) As soon as I resize the window, though, things work perfectly.

I'd really love a way to get the size of scrollbars that's available at the time viewport units are initially calculated. There's no actual dependency on layout being run here. I would hate to have to wait for layout to run once just to be able to get scrollbar sizes or the size of the client area, and then force layout to run _again_, since the computed values of viewport units will have changed.
(Assignee)

Comment 19

5 years ago
Created attachment 725814 [details] [diff] [review]
(Part 1) - Take scrollbars into account for viewport units if 'overflow: scroll' is set.

OK, figured out how to get scrollbar sizes without depending on layout. The missing piece of the puzzle was nsPresShell::GetReferenceRenderingContext(). Everything works fine now, and all reftests pass.
Attachment #725814 - Flags: review?(dbaron)
(Assignee)

Updated

5 years ago
Attachment #725742 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #725702 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 725815 [details] [diff] [review]
(Part 2) - Tests for viewport unit interactions with overflow values of scroll and auto.

Added the new tests suggested in the review comments.
(Assignee)

Comment 22

5 years ago
David, do you concur that once this cooks on Nightly for a bit we should uplift it to any versions that have viewport units support?
Comment on attachment 725814 [details] [diff] [review]
(Part 1) - Take scrollbars into account for viewport units if 'overflow: scroll' is set.

Gaving 'scroll' as a value is actually relatively rare compared to the other values.  So this code:

>+  // Check for 'overflow: scroll' styles on the root scroll frame. If we find
>+  // any, the standard requires us to take scrollbars into account.
>+  nsIScrollableFrame* scrollFrame =
>+    aPresContext->PresShell()->GetRootScrollFrameAsScrollable();
>+  if (scrollFrame) {
>+    // Gather style and scrollbar size information.
>+    nsRefPtr<nsRenderingContext> context =
>+      aPresContext->PresShell()->GetReferenceRenderingContext();
>+    nsMargin sizes(scrollFrame->GetDesiredScrollbarSizes(aPresContext, context));
>+    nsPresContext::ScrollbarStyles styles(scrollFrame->GetScrollbarStyles());
>+
>+    if (styles.mHorizontal == NS_STYLE_OVERFLOW_SCROLL) {
>+      // 'overflow-x: scroll' means we must consider the horizontal scrollbar,
>+      // which affects the scale of viewport height units.
>+      viewportSize.height -= sizes.TopBottom();
>+    }
>+
>+    if (styles.mVertical == NS_STYLE_OVERFLOW_SCROLL) {
>+      // 'overflow-y: scroll' means we must consider the vertical scrollbar,
>+      // which affects the scale of viewport width units.
>+      viewportSize.width -= sizes.LeftRight();
>+    }
>+  }

should call GetScrollbarStyles first after the null-check of scrollFrame, and then skip the GetReferenceRenderingContext and GetDesiredScrollbarSizes calls unless one of the styles is NS_STYLE_OVERFLOW_SCROLL.

r=dbaron with that
Attachment #725814 - Flags: review?(dbaron) → review+
(Assignee)

Comment 24

5 years ago
Created attachment 732131 [details] [diff] [review]
(Part 1) - Take scrollbars into account for viewport units if 'overflow: scroll' is set.

Thanks for the review, David! Here's an updated patch.
(Assignee)

Updated

5 years ago
Attachment #725814 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
Created attachment 732163 [details] [diff] [review]
Followup - Disable inequality test on platforms without scrollbars.

Unfortunately the two cases we're checking in the '!=' test look the same on platforms with overlay scrollbars, causing spurious test failures. I pushed a followup patch that disables that test on Android and B2G.
https://hg.mozilla.org/mozilla-central/rev/6d7e194aac6a
https://hg.mozilla.org/mozilla-central/rev/ccd3d04a3d43
https://hg.mozilla.org/mozilla-central/rev/73ca02e5cf97
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Whiteboard: [DocArea=CSS]

Comment 30

a year ago
The current ED of the spec says only that the root element's "overflow" property affects switches the viewport units from excluding scrollbars width to including them and back. Currently, in Firefox (version 54 on Windows 10) the "overflow" property of the "body" element does the same. Is it correct?
See Also: → bug 1393603
You need to log in before you can comment on or make changes to this bug.