Bug 112415 - CSS Variables are not properly set via Web Inspector
Summary: CSS Variables are not properly set via Web Inspector
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alan Cutter
URL:
Keywords:
Depends on:
Blocks: 85580
  Show dependency treegraph
 
Reported: 2013-03-15 00:28 PDT by Alan Cutter
Modified: 2014-02-08 01:41 PST (History)
17 users (show)

See Also:


Attachments
Patch (9.70 KB, patch)
2013-03-25 21:58 PDT, Alan Cutter
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (491.43 KB, application/zip)
2013-03-26 01:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-future (946.84 KB, application/zip)
2013-03-26 11:55 PDT, Build Bot
no flags Details
Patch (8.39 KB, patch)
2013-03-26 18:28 PDT, Alan Cutter
no flags Details | Formatted Diff | Diff
Patch (8.15 KB, patch)
2013-03-27 03:40 PDT, Alan Cutter
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Cutter 2013-03-15 00:28:50 PDT
When setting CSS variables in the Web Inspector only the last variable takes effect. This is most likely due to CSS variables all sharing the same CSSPropertyID.

Example:
#test {
    padding-left: -webkit-var(a);
    padding-top: -webkit-var(b);
    -webkit-var-a: 100px;
    -webkit-var-b: 200px;
}

If these values are entered via the Web Inspector only padding-top will have the correct value. padding-left will remain unset.
Comment 1 Alan Cutter 2013-03-25 21:58:07 PDT
Created attachment 194999 [details]
Patch
Comment 2 Build Bot 2013-03-26 01:58:55 PDT
Comment on attachment 194999 [details]
Patch

Attachment 194999 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17295382

New failing tests:
inspector/styles/add-multiple-inline-css-variables.html
Comment 3 Build Bot 2013-03-26 01:58:57 PDT
Created attachment 195033 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 4 Build Bot 2013-03-26 11:55:07 PDT
Comment on attachment 194999 [details]
Patch

Attachment 194999 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17289514

New failing tests:
inspector/styles/add-multiple-inline-css-variables.html
Comment 5 Build Bot 2013-03-26 11:55:10 PDT
Created attachment 195130 [details]
Archive of layout-test-results from webkit-ews-01 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-future  Platform: Mac OS X 10.8.2
Comment 6 Alan Cutter 2013-03-26 18:28:28 PDT
Created attachment 195205 [details]
Patch
Comment 7 Alan Cutter 2013-03-26 18:29:45 PDT
(In reply to comment #6)
> Created an attachment (id=195205) [details]
> Patch

Removed test from Mac port (CSS variables not enabled there).
Comment 8 Alexander Pavlov (apavlov) 2013-03-27 03:00:43 PDT
Comment on attachment 195205 [details]
Patch

The Web Inspector test looks good.

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

> Source/WebCore/css/StylePropertySet.cpp:759
> +    ASSERT(value->isVariableValue());

toHTMLElement() and friends check for !value as well. Do you want to do the same here?

> Source/WebCore/css/StylePropertySet.cpp:765
> +    ASSERT(value->isVariableValue());

Ditto

> Source/WebCore/css/StylePropertySet.cpp:771
> +    for (int i = propertyCount() - 1; i >= 0; i--) {

Most for-loops in this file seem to use the prefix-decrement.

> LayoutTests/inspector/styles/variables/add-inline-css-variable.html:14
> +        var idToDOMNode = WebInspector.domAgent._idToDOMNode;

You can use InspectorTest.expandedNodeWithId(idValue) for brevity (since you have selected it in the tree) from elements-test.js (and then use node.id)
Comment 9 Alan Cutter 2013-03-27 03:40:40 PDT
Created attachment 195261 [details]
Patch
Comment 10 Alan Cutter 2013-03-27 03:43:46 PDT
(In reply to comment #8)
> (From update of attachment 195205 [details])
> The Web Inspector test looks good.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=195205&action=review
> 

Thanks for the review.

> > Source/WebCore/css/StylePropertySet.cpp:759
> > +    ASSERT(value->isVariableValue());
> 
> toHTMLElement() and friends check for !value as well. Do you want to do the same here?
> 
> > Source/WebCore/css/StylePropertySet.cpp:765
> > +    ASSERT(value->isVariableValue());
> 
> Ditto
> 

Added check for !value.

> > Source/WebCore/css/StylePropertySet.cpp:771
> > +    for (int i = propertyCount() - 1; i >= 0; i--) {
> 
> Most for-loops in this file seem to use the prefix-decrement.
> 

Changed to prefix.

> > LayoutTests/inspector/styles/variables/add-inline-css-variable.html:14
> > +        var idToDOMNode = WebInspector.domAgent._idToDOMNode;
> 
> You can use InspectorTest.expandedNodeWithId(idValue) for brevity (since you have selected it in the tree) from elements-test.js (and then use node.id)

I'm glad there was an easier way, I'm not so familiar with the Web Inspector to know much better, just copied what another test was doing. Changed to use expandedNodeWithId.
Comment 11 Andreas Kling 2014-02-07 03:41:09 PST
CSS variables got yanked.