Bug 91554 - viewport units (vw, vh) don't work in CSS transforms
Summary: viewport units (vw, vh) don't work in CSS transforms
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bem Jones-Bey
URL:
Keywords:
Depends on: 112315
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-17 16:28 PDT by Mike Lawther
Modified: 2017-07-18 08:27 PDT (History)
19 users (show)

See Also:


Attachments
Testcase (907 bytes, text/html)
2012-07-17 16:29 PDT, Mike Lawther
no flags Details
Patch (51.17 KB, patch)
2013-02-14 16:29 PST, Timothy Loh
no flags Details | Formatted Diff | Diff
WIP (317.16 KB, patch)
2013-03-12 00:04 PDT, Timothy Loh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2012-07-17 16:28:18 PDT
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?
Comment 1 Mike Lawther 2012-07-17 16:29:01 PDT
Created attachment 152870 [details]
Testcase
Comment 2 Mike Lawther 2012-08-09 23:31:43 PDT
Ping. Joe - are you able to look into this one?
Comment 3 Joe Thomas 2012-08-10 11:45:43 PDT
(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.
Comment 4 Timothy Loh 2013-02-14 16:29:57 PST
Created attachment 188450 [details]
Patch
Comment 5 Timothy Loh 2013-02-14 16:35:21 PST
(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?
Comment 6 Dean Jackson 2013-02-14 16:39:20 PST
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.
Comment 7 Mike Lawther 2013-02-17 23:14:39 PST
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.
Comment 8 Timothy Loh 2013-03-12 00:04:30 PDT
Created attachment 192654 [details]
WIP
Comment 9 Timothy Loh 2013-03-12 00:19:00 PDT
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?
Comment 10 Simon Fraser (smfr) 2013-03-12 11:23:07 PDT
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.
Comment 11 Timothy Loh 2013-03-12 16:08:56 PDT
(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?
Comment 12 Ojan Vafai 2013-03-13 16:53:11 PDT
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?
Comment 13 Timothy Loh 2013-03-13 17:03:59 PDT
(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.
Comment 14 Joe Thomas 2013-03-13 17:13:11 PDT
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.
Comment 15 Timothy Loh 2013-03-13 17:26:38 PDT
(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.
Comment 16 Bem Jones-Bey 2014-06-11 09:21:28 PDT
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.