Bug 83026 - REGRESSION(r112177): listStyleType CSS property gets converted into listStyle
Summary: REGRESSION(r112177): listStyleType CSS property gets converted into listStyle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
URL: http://tinymce.moxiecode.com/webkit/l...
Keywords:
: 84618 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-04-03 06:37 PDT by Johan "Spocke" Sörlin
Modified: 2012-04-26 19:37 PDT (History)
5 users (show)

See Also:


Attachments
Fixes the regression (18.95 KB, patch)
2012-04-24 14:53 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (19.63 KB, patch)
2012-04-24 16:56 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johan "Spocke" Sörlin 2012-04-03 06:37:30 PDT
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
Comment 1 Alexis Menard (darktears) 2012-04-03 06:47:33 PDT
(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.
Comment 2 Ryosuke Niwa 2012-04-03 12:04:53 PDT
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.
Comment 3 Johan "Spocke" Sörlin 2012-04-03 13:34:12 PDT
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.
Comment 4 Ryosuke Niwa 2012-04-03 13:41:09 PDT
(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.
Comment 5 Ryosuke Niwa 2012-04-24 14:06:58 PDT
*** Bug 84618 has been marked as a duplicate of this bug. ***
Comment 6 Ryosuke Niwa 2012-04-24 14:07:51 PDT
Okay, we do have to fix this bug.
Comment 7 Ryosuke Niwa 2012-04-24 14:53:45 PDT
Created attachment 138654 [details]
Fixes the regression
Comment 8 Alexis Menard (darktears) 2012-04-24 15:00:48 PDT
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.
Comment 9 Ryosuke Niwa 2012-04-24 15:01:56 PDT
(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.
Comment 10 Alexis Menard (darktears) 2012-04-24 15:16:42 PDT
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.
Comment 11 Ryosuke Niwa 2012-04-24 16:16:50 PDT
(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.
Comment 12 Early Warning System Bot 2012-04-24 16:28:47 PDT
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
Comment 13 Early Warning System Bot 2012-04-24 16:43:43 PDT
Comment on attachment 138654 [details]
Fixes the regression

Attachment 138654 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12528378
Comment 14 Ryosuke Niwa 2012-04-24 16:56:56 PDT
Created attachment 138692 [details]
Patch
Comment 15 Darin Adler 2012-04-24 17:29:44 PDT
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?
Comment 16 Ryosuke Niwa 2012-04-24 17:48:11 PDT
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 :-(
Comment 17 Ryosuke Niwa 2012-04-25 10:43:35 PDT
Committed r115227: <http://trac.webkit.org/changeset/115227>
Comment 18 Ryosuke Niwa 2012-04-25 14:36:16 PDT
Build fix landed in http://trac.webkit.org/changeset/115244.
Comment 19 Rafael Brandao 2012-04-25 16:24:25 PDT
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).
Comment 20 Ryosuke Niwa 2012-04-25 18:12:57 PDT
(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.
Comment 21 Ryosuke Niwa 2012-04-26 04:44:34 PDT
Another build fix landed in http://trac.webkit.org/changeset/115302.
Comment 22 Ryosuke Niwa 2012-04-26 19:37:18 PDT
Committed r115399: <http://trac.webkit.org/changeset/115399>