Bug 121422 – CSS Unit vh, vw, vmin and vmax in box-shadow are not applied.

• Component: CSS
• Status: RESOLVED FIXED

CSS Unit vh, vw, vmin and vmax in border-shadow are not applied.

Created attachment 211759 [details]
CSS border-shadow with vh, vw units

Created attachment 212055 [details]
Patch

Created attachment 212059 [details]
Patch

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

Small change in the bug title and changelog.

[Build Bot]

Comment on attachment 212059 [details]
Patch

Attachment 212059 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2004049

New failing tests:
fast/css/shadow-viewport-units.html

[Build Bot]

Created attachment 212062 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212059 [details]
Patch

Attachment 212059 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1968168

New failing tests:
fast/css/shadow-viewport-units.html

[Build Bot]

Created attachment 212065 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212059 [details]
Patch

Attachment 212059 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1949172

New failing tests:
fast/css/shadow-viewport-units.html

[Build Bot]

Created attachment 212069 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 212117 [details]
Patch

[Darin Adler]

Comment on attachment 212117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212117&action=review

New feature looks pretty good. Some style mistakes that need to be fixed.

Also, do the tests cover all the code paths?S

> Source/WebCore/css/StyleResolver.cpp:4068
> +int StyleResolver::getViewportPercentageLength(CSSPrimitiveValue* item, int factor)

I’m not sure this function is a good idea. Instead I suggest we have four small helper functions, viewportHeight and viewportWidth functions and min and max ones. At each call site we already know which of these four cases we are in, so it's awkward to pass the value in and check the item inside this function.

> Source/WebCore/css/StyleResolver.cpp:4083
> +}
>  } // namespace WebCore

Should have a blank line here.

> Source/WebCore/css/StyleResolver.h:588
> +    int getViewportPercentageLength(CSSPrimitiveValue* item, int factor);

This is not right for WebKit coding style. The prefix get should not be used for a function that returns a value. And the argument name "item" should be omitted. Further "factor" is not a specific enough name to make clear what the argument is. I think maybe percentage is what is should be named.

> LayoutTests/fast/css/shadow-viewport-units.html:10
> +    var a = document.getElementById("a");	

Should not add trailing spaces.

> LayoutTests/fast/css/shadow-viewport-units.html:12
> +    document.body.appendChild(output);	

Should not add trailing spaces.

> LayoutTests/fast/css/shadow-viewport-units.html:13
> +    if (window.getComputedStyle(a).textShadow == "none")	    

Should not add trailing spaces.Should not add trailing spaces.

> LayoutTests/fast/css/shadow-viewport-units.html:15
> +    else	

Should not add trailing spaces.

Created attachment 212131 [details]
Patch

(In reply to comment #12)
> (From update of attachment 212117 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212117&action=review
> 
> New feature looks pretty good. Some style mistakes that need to be fixed.
> 
> Also, do the tests cover all the code paths?S
> 
> > Source/WebCore/css/StyleResolver.cpp:4068
> > +int StyleResolver::getViewportPercentageLength(CSSPrimitiveValue* item, int factor)
> 
> I’m not sure this function is a good idea. Instead I suggest we have four small helper functions, viewportHeight and viewportWidth functions and min and max ones. At each call site we already know which of these four cases we are in, so it's awkward to pass the value in and check the item inside this function.
> 
> > Source/WebCore/css/StyleResolver.cpp:4083
> > +}
> >  } // namespace WebCore
> 
> Should have a blank line here.
> 
> > Source/WebCore/css/StyleResolver.h:588
> > +    int getViewportPercentageLength(CSSPrimitiveValue* item, int factor);
> 
> This is not right for WebKit coding style. The prefix get should not be used for a function that returns a value. And the argument name "item" should be omitted. Further "factor" is not a specific enough name to make clear what the argument is. I think maybe percentage is what is should be named.
> 
> > LayoutTests/fast/css/shadow-viewport-units.html:10
> > +    var a = document.getElementById("a");	
> 
> Should not add trailing spaces.
> 
> > LayoutTests/fast/css/shadow-viewport-units.html:12
> > +    document.body.appendChild(output);	
> 
> Should not add trailing spaces.
> 
> > LayoutTests/fast/css/shadow-viewport-units.html:13
> > +    if (window.getComputedStyle(a).textShadow == "none")	    
> 
> Should not add trailing spaces.Should not add trailing spaces.
> 
> > LayoutTests/fast/css/shadow-viewport-units.html:15
> > +    else	
> 
> Should not add trailing spaces.

Thanks Darin for the review. I have added few more test cases. Now it covers all the code path.

[Build Bot]

Comment on attachment 212131 [details]
Patch

Attachment 212131 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1811217

New failing tests:
fast/css/shadow-viewport-units.html

[Build Bot]

Created attachment 212136 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212131 [details]
Patch

Attachment 212131 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1925333

New failing tests:
fast/css/shadow-viewport-units.html

[Build Bot]

Created attachment 212139 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Created attachment 212151 [details]
Patch

[Darin Adler]

Comment on attachment 212151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212151&action=review

> Source/WebCore/css/StyleResolver.cpp:2398
> +                if (item->x->isViewportPercentageHeight())
> +                    x = viewportPercentageHeight(x);
> +                else if (item->x->isViewportPercentageWidth())
> +                    x = viewportPercentageWidth(x);
> +                else if (item->x->isViewportPercentageMax())
> +                    x = viewportPercentageMax(x);
> +                else if (item->x->isViewportPercentageMin())
> +                    x = viewportPercentageMin(x);

Damn, I misread the earlier patch and gave you really bad advice. We should go back to more the style you had in that one. Really sorry!

(In reply to comment #20)
> (From update of attachment 212151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212151&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:2398
> > +                if (item->x->isViewportPercentageHeight())
> > +                    x = viewportPercentageHeight(x);
> > +                else if (item->x->isViewportPercentageWidth())
> > +                    x = viewportPercentageWidth(x);
> > +                else if (item->x->isViewportPercentageMax())
> > +                    x = viewportPercentageMax(x);
> > +                else if (item->x->isViewportPercentageMin())
> > +                    x = viewportPercentageMin(x);
> 
> Damn, I misread the earlier patch and gave you really bad advice. We should go back to more the style you had in that one. Really sorry!

So you mean shld rollback to the earlier patch?

[Darin Adler]

Comment on attachment 212151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212151&action=review

>>> Source/WebCore/css/StyleResolver.cpp:2398
>>> +                    x = viewportPercentageMin(x);
>> 
>> Damn, I misread the earlier patch and gave you really bad advice. We should go back to more the style you had in that one. Really sorry!
> 
> So you mean shld rollback to the earlier patch?

Roll back to the simple function that does all of these, but keep the other fixes I suggested. Let me go look at it again. Sorry for the runaround!

[Darin Adler]

Comment on attachment 212117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212117&action=review

> Source/WebCore/css/StyleResolver.cpp:4081
> +    return 0;

ASSERT_NOT_REACHED here, I think.

[Darin Adler]

Comment on attachment 212151 [details]
Patch

Again, really sorry to steer you wrong. I think your original code was good, with just a few style items to fix (name of function, name of argument, etc.)

Created attachment 212319 [details]
Patch

[Early Warning System Bot]

Comment on attachment 212319 [details]
Patch

Attachment 212319 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1981044

[Early Warning System Bot]

Comment on attachment 212319 [details]
Patch

Attachment 212319 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1999445

Created attachment 212320 [details]
Patch

[Darin Adler]

Comment on attachment 212320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212320&action=review

> Source/WebCore/css/StyleResolver.cpp:4068
> +int StyleResolver::viewportPercentageValue(CSSPrimitiveValue* unit, int percentage)

The unit argument could be CSSPrimitiveValue& instead of CSSPrimitiveValue*.

> Source/WebCore/css/StyleResolver.h:588
> +    // to get the viewport units value in terms of viewport height and width

This comment doesn’t seem to add much, and it’s also not in the right format. We use sentence style comments, not fragments like this one. I suggest just removing it.

Created attachment 212328 [details]
Patch

[Darin Adler]

Comment on attachment 212328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212328&action=review

> Source/WebCore/css/StyleResolver.cpp:2400
> +            if (item->x->isViewportPercentageLength())
> +                x = viewportPercentageValue(*item->x.get(), x);
>              int y = item->y->computeLength<int>(state.style(), state.rootElementStyle(), zoomFactor);
> +            if (item->y->isViewportPercentageLength())
> +                y = viewportPercentageValue(*item->y.get(), y);
>              int blur = item->blur ? item->blur->computeLength<int>(state.style(), state.rootElementStyle(), zoomFactor) : 0;
> +            if (item->blur && item->blur->isViewportPercentageLength())
> +                blur = viewportPercentageValue(*item->blur.get(), blur);
>              int spread = item->spread ? item->spread->computeLength<int>(state.style(), state.rootElementStyle(), zoomFactor) : 0;
> +            if (item->spread && item->spread->isViewportPercentageLength())
> +                spread = viewportPercentageValue(*item->spread.get(), spread);

No need to also use get() if we are using *.

[WebKit Commit Bot]

Comment on attachment 212328 [details]
Patch

Rejecting attachment 212328 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 212328, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
chanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 8] nodename nor servname provided, or not known>

Full output: http://webkit-queues.appspot.com/results/2125002

Created attachment 212420 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 212420 [details]
Patch

Clearing flags on attachment: 212420

Committed r156318: <http://trac.webkit.org/changeset/156318>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.