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.

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.

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.

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.)

Chrome (23.0.1271.101) is the same, unsurprisingly.

Same behavior in IE (10.0.9200.16466).

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.

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.

Whoops! Uploaded the wrong file.

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).

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.

Will jump on this ASAP.

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.

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

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.

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.

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.

Added the new tests suggested in the review comments.

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=412fb4d15db2

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

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

Pushed to inbound. https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7e194aac6a https://hg.mozilla.org/integration/mozilla-inbound/rev/ccd3d04a3d43

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

I've updated the documentation: https://developer.mozilla.org/en-US/docs/Web/CSS/length#Viewport-percentage_lengths and https://developer.mozilla.org/en-US/Firefox/Releases/23#CSS

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?