David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Description • 12 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 1 • 12 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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 2 • 12 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 3 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 4 • 11 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.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 11 years ago

Chrome (23.0.1271.101) is the same, unsurprisingly.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 6 • 11 years ago

Same behavior in IE (10.0.9200.16466).

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 7 • 11 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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 8 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 9 • 11 years ago

Whoops! Uploaded the wrong file.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 10 • 11 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.)

	Jean-Yves Perrier [:teoli]
	Comment 11 • 11 years ago

Added dev-doc-needed, we should improve our <length> doc with this case (once fixed on our side).

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 12 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 13 • 11 years ago

Will jump on this ASAP.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 14 • 11 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 15 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 16 • 11 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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 17 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 18 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 19 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 20 • 11 years ago

Added the new tests suggested in the review comments.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 21 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 22 • 11 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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 23 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 24 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 25 • 11 years ago

Pushed to inbound.

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 26 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ca02e5cf97

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/6d7e194aac6a
https://hg.mozilla.org/mozilla-central/rev/ccd3d04a3d43
https://hg.mozilla.org/mozilla-central/rev/73ca02e5cf97

	Jean-Yves Perrier [:teoli]
	Comment 29 • 10 years ago

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

	Ilya
	Comment 30 • 7 years 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?