Bug 42799 – pixelWidth/posWidth are processed incorrectly

• Component: CSS
• Status: RESOLVED WONTFIX

[Yuxiang Luo]

[Environment: Google Chrome 6.0.466.0 (Official Build 52279) dev, WebKit 534.3]

When setting an element's 'width' with em unit, 100em for example, the 'pixelWidth' is also 100, i.e. without transforming from 'em' to 'px'. It's also the same for 'height', 'top', 'bottom', 'left', 'right'.

[Simon Fraser (smfr)]

What is "pixelWidth"? I don't see it in the source anywhere.

[Yuxiang Luo]

(In reply to comment #1)
> What is "pixelWidth"? I don't see it in the source anywhere.

pixelWidth and posWidth (and others) properties are calculated when they are accessed. You can look at 'JSCSSStyleDeclaration.cpp', and search 'pixel'.
I'm currently working on a patch to fix 'pixel/pos' problem.

[Yuxiang Luo]

The existence of CSS properties that start with prefix 'pixel' and 'pos' is for compatibility with IE, but the currently the behavior differs a lot from that of IE. Take pixelWidth/posWidth as example:
1. Getting pixelWidth: if style.width has unit type 'em', 'ex' or 'rem', then pixelWidth's value is not returned without transformation
2. Getting posWidth: it should return parseFloat(style.width), but not the same with pixelWidth
3. Setting pixelWidth: style.width's unit type should keep unchanged, while the value should be changed with proper transformation.
4. Setting posWidth: style.width's unit type should keep unchanged, while the value is exactly the value assign to posWidth

[Simon Fraser (smfr)]

Please attach a testcase.

[Yuxiang Luo]

Created attachment 66845 [details]
testcase for css properties with pixel/pos prefix

The patch for this bug is almost ready

[Yuxiang Luo]

Created attachment 67148 [details]
patch

[WebKit Review Bot]

Attachment 67148 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSPrimitiveValue.cpp:352:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/css/CSSPrimitiveValue.cpp:380:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/css/CSSPrimitiveValue.cpp:840:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuxiang Luo]

Created attachment 67151 [details]
patch

There are style warning about 'case label indent', but the file already has too many such kind of style warnings. To avoid distracting reviewer's attention, I temporarily keep them unchanged.

[WebKit Review Bot]

Attachment 67151 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSPrimitiveValue.cpp:312:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/css/CSSPrimitiveValue.cpp:842:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuxiang Luo]

Can someone take a look at this bug and help review? thanks

[Darin Adler]

Comment on attachment 67151 [details]
patch

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

This code really ought to be factored so that the rules about pixel and pos are in the DOM; the code inside the bindings is getting way too complex and is doing things that should not be part of the bindings.

It’s OK to have the JavaScript bindings parse out the pixel and pos prefix, but then the work of actually computing an appropriate value for both getting and setting should be done in the DOM code, not the bindings. And we should consider moving almost all of the code into the DOM class. The bindings can just pass in the property name and an enum to indicate what the prefix is, and the rest can be handled in the DOM classes. The rule of thumb is that the bindings code should be about translating from the engine data structures to the DOM, but most of the logic should be in the DOM class itself.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:85
> -static String cssPropertyName(const Identifier& propertyName, bool* hadPixelOrPosPrefix = 0)
> +static String cssPropertyName(const Identifier& propertyName, bool* hadPixelPrefix = 0, bool* hadPosPrefix = 0)

Since we support only one prefix, this should be a single enum, not a pair of booleans.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:153
> +    // posTop returns "CSS top" as number value in original unit _if_ its a

Should be "it's" not "its".

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:156
> +    bool pixel, pos;

We normally declare multiple variables like this on multiple lines.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:162
> +            RefPtr<CSSPrimitiveValue> pv = static_pointer_cast<CSSPrimitiveValue>(v);

This seems to do extra work that is not needed for the common case where we have neither the pixel nor the pos prefix.

We normally prefer to use words rather than letters for local variables, and this is something we try to stick with for new code.

I suggest primitiveValue rather than pv.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:164
> +                // pixel* like pixelTop returns integer instead of float

The wording of this comment is confusing. One way to write this in plain language is:

    // Properties with names with a pixel prefix, such as pixelTop, return integer values, not floating point.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:166
> +                    RefPtr<Node> node = thisObj->impl()->getNode();

This should be a raw pointer, not a RefPtr.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:172
> +                    if (style && rootStyle)
> +                        return jsNumber(exec, pv->computeLengthInt(style, rootStyle));
> +                    return jsNumber(exec, 0);

In WebKit we normally use a style called “early return”. In this coding style you would write it like this:

    if (!style || !rootStyle)
        return jsNumber(exec, 0);
    return jsNumber(exec, primitiveValue->computeLengthInt(style, rootStyle));

[Darin Adler]

Thanks very much for taking this on. I think you’re heading in the right direction with this patch. Sorry it took so long for me to get around to reviewing.

[Yuxiang Luo]

(In reply to comment #11)
> (From update of attachment 67151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67151&action=review
> 
> This code really ought to be factored so that the rules about pixel and pos are in the DOM; the code inside the bindings is getting way too complex and is doing things that should not be part of the bindings.

Hi,darin

Thanks very much for the review. Do you mean the handling of pixel/pos should be put in CSSStyleDeclaration class? However, I don't think it should be put there, because pixel/pos-prefix properties are not CSS property, but CSSOM property accessed by script. And, if we put the logic in there, when setting 'pixelWidth', the CSSStyleDeclaration::setProperty will be invoked as following:
  setProperty('width', PREFIX_PIXEL, 100, ...);
The interface then looks very weird, people don't know what the interface intends to do.

Isn't it better to let CSSStyleDeclaration do only what it should do, that is to get/set primitive css properties? It might be better to put transformation code between primitive css properties and cssom properties in bindings.

> 
> It’s OK to have the JavaScript bindings parse out the pixel and pos prefix, but then the work of actually computing an appropriate value for both getting and setting should be done in the DOM code, not the bindings. And we should consider moving almost all of the code into the DOM class. The bindings can just pass in the property name and an enum to indicate what the prefix is, and the rest can be handled in the DOM classes. The rule of thumb is that the bindings code should be about translating from the engine data structures to the DOM, but most of the logic should be in the DOM class itself.
> 
> > WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:85
> > -static String cssPropertyName(const Identifier& propertyName, bool* hadPixelOrPosPrefix = 0)
> > +static String cssPropertyName(const Identifier& propertyName, bool* hadPixelPrefix = 0, bool* hadPosPrefix = 0)
> 
> Since we support only one prefix, this should be a single enum, not a pair of booleans.
> 
> > WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:153
> > +    // posTop returns "CSS top" as number value in original unit _if_ its a
> 
> Should be "it's" not "its".
> 
> > WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:156
> > +    bool pixel, pos;
> 
> We normally declare multiple variables like this on multiple lines.
> 
> > WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:162
> > +            RefPtr<CSSPrimitiveValue> pv = static_pointer_cast<CSSPrimitiveValue>(v);
> 
> This seems to do extra work that is not needed for the common case where we have neither the pixel nor the pos prefix.
> 
> We normally prefer to use words rather than letters for local variables, and this is something we try to stick with for new code.
> 
> I suggest primitiveValue rather than pv.
> 
> > WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:164
> > +                // pixel* like pixelTop returns integer instead of float
> 
> The wording of this comment is confusing. One way to write this in plain language is:
> 
>     // Properties with names with a pixel prefix, such as pixelTop, return integer values, not floating point.
> 
> > WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:166
> > +                    RefPtr<Node> node = thisObj->impl()->getNode();
> 
> This should be a raw pointer, not a RefPtr.
> 
> > WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:172
> > +                    if (style && rootStyle)
> > +                        return jsNumber(exec, pv->computeLengthInt(style, rootStyle));
> > +                    return jsNumber(exec, 0);
> 
> In WebKit we normally use a style called “early return”. In this coding style you would write it like this:
> 
>     if (!style || !rootStyle)
>         return jsNumber(exec, 0);
>     return jsNumber(exec, primitiveValue->computeLengthInt(style, rootStyle));

[Darin Adler]

(In reply to comment #13)
> Thanks very much for the review. Do you mean the handling of pixel/pos should be put in CSSStyleDeclaration class? However, I don't think it should be put there, because pixel/pos-prefix properties are not CSS property, but CSSOM property accessed by script. And, if we put the logic in there, when setting 'pixelWidth', the CSSStyleDeclaration::setProperty will be invoked as following:
>   setProperty('width', PREFIX_PIXEL, 100, ...);
> The interface then looks very weird, people don't know what the interface intends to do.
> 
> Isn't it better to let CSSStyleDeclaration do only what it should do, that is to get/set primitive css properties? It might be better to put transformation code between primitive css properties and cssom properties in bindings.

Yes, we should put it on CSSStyleDeclaration. If you don’t like having that code in CSSStyleDeclaration we can just call over to Element and put the bulk of the work in that class.

[Yuxiang Luo]

(In reply to comment #14)
> (In reply to comment #13)
> > Thanks very much for the review. Do you mean the handling of pixel/pos should be put in CSSStyleDeclaration class? However, I don't think it should be put there, because pixel/pos-prefix properties are not CSS property, but CSSOM property accessed by script. And, if we put the logic in there, when setting 'pixelWidth', the CSSStyleDeclaration::setProperty will be invoked as following:
> >   setProperty('width', PREFIX_PIXEL, 100, ...);
> > The interface then looks very weird, people don't know what the interface intends to do.
> > 
> > Isn't it better to let CSSStyleDeclaration do only what it should do, that is to get/set primitive css properties? It might be better to put transformation code between primitive css properties and cssom properties in bindings.
> 
> Yes, we should put it on CSSStyleDeclaration. If you don’t like having that code in CSSStyleDeclaration we can just call over to Element and put the bulk of the work in that class.

But such properties belong to Style object, not Element/Node. Logically, such CSSOM properties should belong to bindings object (i.e. JSCSSStyleDeclaration), although it makes the logic in it more complex.

[Johnny(Jianning) Ding]

(In reply to comment #15)
> But such properties belong to Style object, not Element/Node. Logically, such CSSOM properties should belong to bindings object (i.e. JSCSSStyleDeclaration), although it makes the logic in it more complex.

It looks like those properties which have pixel/pos prefix are designed to be compatible with IE. So far those properties are not part of CSSOM spec, but the question is do we consider treating them as part of CSSOM?

If yes, according to DOM spec, the access interface of those properties ought to be language independent, the logic can be inside of the CSSStyleDeclaration's native implementation, and all language bindings can access those properties via the unified implementation. But currently the logic is only implemented in js binding. So I agree with Darin's comment.

[Yuxiang Luo]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Thanks very much for the review. Do you mean the handling of pixel/pos should be put in CSSStyleDeclaration class? However, I don't think it should be put there, because pixel/pos-prefix properties are not CSS property, but CSSOM property accessed by script. And, if we put the logic in there, when setting 'pixelWidth', the CSSStyleDeclaration::setProperty will be invoked as following:
> > >   setProperty('width', PREFIX_PIXEL, 100, ...);
> > > The interface then looks very weird, people don't know what the interface intends to do.
> > > 
> > > Isn't it better to let CSSStyleDeclaration do only what it should do, that is to get/set primitive css properties? It might be better to put transformation code between primitive css properties and cssom properties in bindings.
> > 
> > Yes, we should put it on CSSStyleDeclaration. If you don’t like having that code in CSSStyleDeclaration we can just call over to Element and put the bulk of the work in that class.
> 
> But such properties belong to Style object, not Element/Node. Logically, such CSSOM properties should belong to bindings object (i.e. JSCSSStyleDeclaration), although it makes the logic in it more complex.

After discussing with my teammate, I think you may be right, that it's better to put them in CSSStyleDeclaration, so other language bindings can also use the same logic. I'll refactor the code, thanks.

[Darin Adler]

The principle here: Language bindings should provide only the binding to the language. Features needed for the language bindings go into the core DOM code. Whether such features are in the DOM or CSS standard is irrelevant to this basic factoring decision. Code inside the language binding should be kept to a minimum. The only things that should go there are things that require binding-specific logic for speed or the parts of the binding that have to do something that we can’t express in a language-independent IDL file.

[Anne van Kesteren]

Can these attributes not be dropped? Minefield does not have them.

[Darin Adler]

(In reply to comment #19)
> Can these attributes not be dropped? Minefield does not have them.

Maybe we can drop them. To find out we have to test websites, and also WebKit-only content such as Dashboard widgets and Mac OS X applications that has never been tested with another web engine.

[Yuxiang Luo]

(In reply to comment #20)
> (In reply to comment #19)
> > Can these attributes not be dropped? Minefield does not have them.
> 
> Maybe we can drop them. To find out we have to test websites, and also WebKit-only content such as Dashboard widgets and Mac OS X applications that has never been tested with another web engine.

I'm not sure whether there is any special cause for adding these properties at the beginning. I'll take a look at how many layout tests this affects.

[Simon Fraser (smfr)]

Doesn't Google have a way to look at its corpus of pages to see how many use these properties?

[Yuxiang Luo]

(In reply to comment #22)
> Doesn't Google have a way to look at its corpus of pages to see how many use these properties?

There's about 1/5 web pages have such properties in its JavaScript source code. If a library has any of these properties in its source code, all pages that uses the library will be counted in.

[Johnny(Jianning) Ding]

> There's about 1/5 web pages have such properties in its JavaScript source code. If a library has any of these properties in its source code, all pages that uses the library will be counted in.
If your calculation is right, there are pretty many pages dependent on those pxiel/pos properties, we may need to keep them.

[Simon Fraser (smfr)]

(In reply to comment #23)
> (In reply to comment #22)
> > Doesn't Google have a way to look at its corpus of pages to see how many use these properties?
> 
> There's about 1/5 web pages have such properties in its JavaScript source code. 

One in five? Are you saying that 20% of web pages use this?

[Xianzhu Wang]

I high(In reply to comment #23)
> There's about 1/5 web pages have such properties in its JavaScript source code. If a library has any of these properties in its source code, all pages that uses the library will be counted in.

I highly suspect your number. Can you provide more details, for example, the set of pages tested, the test method, name of the library using the properties and how the properties are used in the library, etc.?

[Yuxiang Luo]

(In reply to comment #26)
> I high(In reply to comment #23)
> > There's about 1/5 web pages have such properties in its JavaScript source code. If a library has any of these properties in its source code, all pages that uses the library will be counted in.
> 
> I highly suspect your number. Can you provide more details, for example, the set of pages tested, the test method, name of the library using the properties and how the properties are used in the library, etc.?

The test is done by Mark Huang for me. I gave him an chrome extension which just scan all JavaScript Sourcecode on a page. I ever want to test on the top 100K URL, but finally he fount most pages returns 404 (from Mark, running over Google Internal service, there may be a bug somewhere which makes so many pages are unaccessible). So, he helped run the extension on a 1000-URL set (which all can be accessed), and got the number. You can turn to him if you want to get more details. I have also checked randomly some of pages, they indeed use these properties. But two points: 1. The set may be a little small. 2. These 1000-URLs are almost Chinese websites.

[Xianzhu Wang]

I think that 1/5 pages contain pixelXXX is because of such code (maybe in some libraries such as jquery):

  if (....pixelXXX)
    use pixelXXX
  else
    use XXX

The above code is because we have bug 29084 (getComputedStyle returns percentage values for left / right / top / bottom). After we fix bug 29084 and drop the pixelXXX properties, the above code will still work.

[Mike Sherov]

Regarding the pixelWidth values being used in jQuery, that only happens if getComputedStyle doesn't exist. So although it appears in jQuery, it isn't used for webkit.

[Mike Sherov]

It looks pixelXXXX and posXXXX can be dropped as far as I can tell. jQuery is the only library that seems to still be using it, and it's only for the case I mentioned. Time as well be time to drop these properties.

[Mike Sherov]

Just dropping by to report that pixelWidth and posWidth are no longer present in jQuery 2.0, proving it wasn't being used for Webkit :-)

[Simon Fraser (smfr)]

Marking WONTFIX since we don't plan to add support for pixelWidth/posWidth, and jQuery no longer needs them.