Bug 83026 – REGRESSION(r112177): listStyleType CSS property gets converted into listStyle

• Component: CSS
• Status: RESOLVED FIXED

[Johan "Spocke" Sörlin]

When you set the listStyleType property on an OL element style it gets converted into a listStyle property when you retrieve it using element.style.cssText. This is a change in behavior from previous versions and it's also not producing the same results as other browsers.

Expected results:
It should return "list-style-type: value".

Actual results:
It now returns "list-style: value" in other words an combined/shorthand style.

Steps to reproduce:
1. Open the attached test URL.
2. Observe that it fails on latest WebKit but not on FF, IE 9, Opera or older WebKit versions.

Seems to be related to this change:
http://trac.webkit.org/changeset/112880

[Alexis Menard (darktears)]

(In reply to comment #0)
> When you set the listStyleType property on an OL element style it gets converted into a listStyle property when you retrieve it using element.style.cssText. This is a change in behavior from previous versions and it's also not producing the same results as other browsers.
> 
> Expected results:
> It should return "list-style-type: value".
> 
> Actual results:
> It now returns "list-style: value" in other words an combined/shorthand style.
> 
> Steps to reproduce:
> 1. Open the attached test URL.
> 2. Observe that it fails on latest WebKit but not on FF, IE 9, Opera or older WebKit versions.
> 
> Seems to be related to this change:
> http://trac.webkit.org/changeset/112880

I don't think it is related to this change. The bug seems to be in StylePropertySet::asText() because of http://trac.webkit.org/changeset/112177

I'm adding Ryosuke as CC.

[Ryosuke Niwa]

I could certainly fix this particular case by using longhand notations when not all longhand properties are set.

But in general, WebKit cannot preserve the list of properties set by the script because we don't store that information anywhere.

[Johan "Spocke" Sörlin]

Hmm, this was breaking some unit test we had. We could add a workaround in the unit tets for this specific case though it might break compatibility for other code out there so it's not important for our needs now when I know why it happens.

Having the auto combine feature in there is a great thing to reduce the number of styles needed that is a feature that other browsers don't have since we needed to add similar functionality in our serialization logic.

[Ryosuke Niwa]

(In reply to comment #3)
> Hmm, this was breaking some unit test we had. We could add a workaround in the unit tets for this specific case though it might break compatibility for other code out there so it's not important for our needs now when I know why it happens.

Yeah, we need a better spec for shorthand/longhand notations and serialization of them :(

> Having the auto combine feature in there is a great thing to reduce the number of styles needed that is a feature that other browsers don't have since we needed to add similar functionality in our serialization logic.

Right, that was the point of the change.

[Ryosuke Niwa]

*** Bug 84618 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Okay, we do have to fix this bug.

[Ryosuke Niwa]

Created attachment 138654 [details]
Fixes the regression

[Alexis Menard (darktears)]

Comment on attachment 138654 [details]
Fixes the regression

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

> Source/WebCore/css/StylePropertySet.cpp:635
> +                value = getPropertyValue(CSSPropertyBorder, ReturnNullOnUncommonValues);

the name uncommon is a bit confusing here. In fact what you want is the "expanded" version.

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 138654 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138654&action=review
> 
> > Source/WebCore/css/StylePropertySet.cpp:635
> > +                value = getPropertyValue(CSSPropertyBorder, ReturnNullOnUncommonValues);
> 
> the name uncommon is a bit confusing here. In fact what you want is the "expanded" version.

I know... but getPropertyValue can't return multiple properties. I'm open for a better name though.

[Alexis Menard (darktears)]

Comment on attachment 138654 [details]
Fixes the regression

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

> Source/WebCore/css/StylePropertySet.cpp:97
>  {

Only border use this valueMode, what about extracting the code in the "case" into a separate function that you can reuse in asText() (you already special case there anyway). Then you can make that enum name less generic.

[Ryosuke Niwa]

(In reply to comment #10)
> (From update of attachment 138654 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138654&action=review
> 
> > Source/WebCore/css/StylePropertySet.cpp:97
> >  {
> 
> Only border use this valueMode, what about extracting the code in the "case" into a separate function that you can reuse in asText() (you already special case there anyway). Then you can make that enum name less generic.

Good idea. That'll make the change local to StylePropertySet.cpp.

[Early Warning System Bot]

Comment on attachment 138654 [details]
Fixes the regression

Attachment 138654 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12525369

[Early Warning System Bot]

Comment on attachment 138654 [details]
Fixes the regression

Attachment 138654 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12528378

[Ryosuke Niwa]

Created attachment 138692 [details]
Patch

[Darin Adler]

Comment on attachment 138692 [details]
Patch

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

> Source/WebCore/css/StylePropertySet.cpp:258
> +    if (!top || !top->value() || !right || !right->value() || !bottom || !bottom->value() || !left || !left->value()
> +        || top->value()->isInitialValue() || right->value()->isInitialValue() || bottom->value()->isInitialValue() || bottom->value()->isInitialValue()
> +        || top->isImportant() != right->isImportant() || right->isImportant() != bottom->isImportant()
> +        || bottom->isImportant() != left->isImportant())
>          return String();

This if statement is hard to read.

How about breaking it up into three separate if statements?

> Source/WebCore/css/StylePropertySet.cpp:398
> +    bool isImportant = false;

Maybe name this lastPropertyWasImportant?

> Source/WebCore/css/StylePropertySet.cpp:420
> +enum CommonValueMode { OmitShorthandWithUncommonValues, ReturnNullOnUncommonValues };

This enum should not be here. We defined it in the header.

> Source/WebCore/css/StylePropertySet.cpp:425
> +    String result;

String is not a good way to build up a result; we’ll have to re-allocate 5 times. Instead we should use StringBuilder. I know the code you moved here was using String.

> Source/WebCore/css/StylePropertySet.h:126
> +    enum CommonValueMode { OmitShorthandWithUncommonValues, ReturnNullOnUncommonValues };

Wouldn’t the name of the first just be OmitUncommonValues? What is a “common value” anyway?

[Ryosuke Niwa]

Common value means the value of properties when they all match. I'm all ears if you know of a better name. I'm particularly unhappy with uncommon value :-(

[Ryosuke Niwa]

Committed r115227: <http://trac.webkit.org/changeset/115227>

[Ryosuke Niwa]

Build fix landed in http://trac.webkit.org/changeset/115244.

[Rafael Brandao]

Comment on attachment 138692 [details]
Patch

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

> Source/WebCore/css/StylePropertySet.cpp:255
> +        || top->value()->isInitialValue() || right->value()->isInitialValue() || bottom->value()->isInitialValue() || bottom->value()->isInitialValue()

I believe you forgot to check for "left->value()->isInitialValue()" here (you repeat bottom twice).

[Ryosuke Niwa]

(In reply to comment #19)
> (From update of attachment 138692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138692&action=review
> 
> > Source/WebCore/css/StylePropertySet.cpp:255
> > +        || top->value()->isInitialValue() || right->value()->isInitialValue() || bottom->value()->isInitialValue() || bottom->value()->isInitialValue()
> 
> I believe you forgot to check for "left->value()->isInitialValue()" here (you repeat bottom twice).

:( Thanks for pointing that out. Fixing it in a minute.

[Ryosuke Niwa]

Another build fix landed in http://trac.webkit.org/changeset/115302.

[Ryosuke Niwa]

Committed r115399: <http://trac.webkit.org/changeset/115399>