Bug 91554 – viewport units (vw, vh) don't work in CSS transforms

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Mike Lawther]

As reported in http://code.google.com/p/chromium/issues/detail?id=137617:

1. See attached viewport-animations.html
2. The third div should animate something like the first two. It does not.
3. I've included the fourth div as a reference that has no animation applied just a transform using vh units, just to show that this isn't specifically an animation issue, but a transform issue (though animations should also work.)

Joe - since you implemented the viewport units, can you take a look at this please?

[Mike Lawther]

Created attachment 152870 [details]
Testcase

[Mike Lawther]

Ping. Joe - are you able to look into this one?

[Joe Thomas]

(In reply to comment #2)
> Ping. Joe - are you able to look into this one?

I am busy with some other work these days. I will take a look at it late next week. Sorry for the delay.

[Timothy Loh]

Created attachment 188450 [details]
Patch

[Timothy Loh]

(In reply to comment #4)
> Created an attachment (id=188450) [details]
> Patch

This seems to fix the problem, but I think there has to be a better way to do this. Making viewport units work in calc() will take additional plumbing. I imagine that adding a RenderView pointer to Length objects would make this much nicer, but I imagine that we want to keep these objects as small as possible.

Does anyone more experienced with the code base have some suggestions on how to do this better?

[Dean Jackson]

Comment on attachment 188450 [details]
Patch

Can we do this without passing a RenderView into platform code? That's why we pass the extents as a Rect to handle percentages.

[Mike Lawther]

Hi Dino - just for some historical context: those floatValueForLength() functions were added as a result of https://bugs.webkit.org/show_bug.cgi?id=27160#c59 where there was the question of unnecessarily calculating the viewport size. 

It seems the way forward here is to pass the viewport extents as an IntSize down into the platform code to avoid any layering violations. It should be non-optional to avoid bugs like this one. It does mean calling the viewportSize() function a lot. If it's demonstrably a performance problem, then caching it or something might be the go.

[Timothy Loh]

Created attachment 192654 [details]
WIP

[Timothy Loh]

Okay, I've tried to do this by changing the optional RenderView* parameter in the valueForLength functions to a required const IntSize&. Unfortunately, the number of files and functions I've had to touch seems much too high to resolve the issues with viewport units (the patch also makes these units compatible with calc, borders, and some other places). It doesn't seem right to have to pass the viewport size around everywhere, but the only other idea I have is to have the viewport size accessible globally (is this actually a workable/decent idea?)

Perhaps one of the more experienced developers here could chime in with some suggestions on how else to attack this problem?

[Simon Fraser (smfr)]

Everywhere in rendering code you should be able to get at the viewport size. In platform code, you can either pass it in, or get it via some "client" callback interface.

[Timothy Loh]

(In reply to comment #10)
> Everywhere in rendering code you should be able to get at the viewport size. In platform code, you can either pass it in, or get it via some "client" callback interface.

Hi Simon,
My problem here isn't getting the viewport size to the appropriate places -- the WIP patch I uploaded does this and seems to work fine. I'm just wondering if there's a better approach to this problem than poking at a hundred files and even more functions. But if the approach seems okay to you, I can tidy up / add tests and put it up for review?

[Ojan Vafai]

This patch looks good to me. I think it'd be good to break it up into a couple smaller pieces, but they don't need to be tiny given then mechanical nature of this patch. So, ideally you'd do something like the following set of patches to make it easier for the reviewer to verify correctness:

1. Add the valueForLength methods to RenderObject.h and change all the callers in the rendering code to use these new methods that don't require passing in the RenderView (exactly as you have in your current patch).
2. Change the Length.h versions of all these functions to take an optional viewport size instead of a RenderView.
3. Change the viewport size argument to be required and add tests.

The first two won't change any behavior and just need to pass the existing tests. 

That sound ok?

[Timothy Loh]

(In reply to comment #12)
> This patch looks good to me. I think it'd be good to break it up into a couple smaller pieces, but they don't need to be tiny given then mechanical nature of this patch. So, ideally you'd do something like the following set of patches to make it easier for the reviewer to verify correctness:
> 
> 1. Add the valueForLength methods to RenderObject.h and change all the callers in the rendering code to use these new methods that don't require passing in the RenderView (exactly as you have in your current patch).
> 2. Change the Length.h versions of all these functions to take an optional viewport size instead of a RenderView.
> 3. Change the viewport size argument to be required and add tests.
> 
> The first two won't change any behavior and just need to pass the existing tests. 
> 
> That sound ok?

Sounds fine. I think there were a few places in getting calc working with viewport units which required some slight non-mechanical changes, so I might put that as a separate patch between your suggested steps 2 and 3.

[Joe Thomas]

In the original implementation of vw, vh, vmin in bug 27160, I tried passing viewport as an argument to Length functions but later got reverted as it regressed Parser/html5-full-render. So we decided to go by RenderView.

[Timothy Loh]

(In reply to comment #14)
> In the original implementation of vw, vh, vmin in bug 27160, I tried passing viewport as an argument to Length functions but later got reverted as it regressed Parser/html5-full-render. So we decided to go by RenderView.

While I'm changing the Length functions to take IntSize parameters, I'm also adding valueForLength methods to RenderObject, which call the original function but won't compute the viewport size unless the Length object is a viewport percentage. The html5-full-render test doesn't seem to have regressed from this patch.

[Bem Jones-Bey]

It looks like this was fixed by the patch for Bug 87846.

I will turn the test in this patch into a layout test, though.

[Brent Fulgham]

Safari, Chrome, and Firefox all agree on rendering for this test case. I don't believe there is any remaining compatibility issue.