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

RESOLVED FIXED 83026

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

https://bugs.webkit.org/show_bug.cgi?id=83026

Summary REGRESSION(r112177): listStyleType CSS property gets converted into listStyle

Johan "Spocke" Sörlin

Reported 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

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexis Menard (darktears)

Comment 1 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.

Ryosuke Niwa

Comment 2 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.

Johan "Spocke" Sörlin

Comment 3 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.

Ryosuke Niwa

Comment 4 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.

Ryosuke Niwa

Comment 5 2012-04-24 14:06:58 PDT

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

Ryosuke Niwa

Comment 6 2012-04-24 14:07:51 PDT

Okay, we do have to fix this bug.

Ryosuke Niwa

Comment 7 2012-04-24 14:53:45 PDT

Created attachment 138654 [details] Fixes the regression

Alexis Menard (darktears)

Comment 8 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.

Ryosuke Niwa

Comment 9 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.

Alexis Menard (darktears)

Comment 10 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.

Ryosuke Niwa

Comment 11 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.

Early Warning System Bot

Comment 12 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

Early Warning System Bot

Comment 13 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

Ryosuke Niwa

Comment 14 2012-04-24 16:56:56 PDT

Created attachment 138692 [details] Patch

Darin Adler

Comment 15 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?

Ryosuke Niwa

Comment 16 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 :-(

Ryosuke Niwa

Comment 17 2012-04-25 10:43:35 PDT

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

Ryosuke Niwa

Comment 18 2012-04-25 14:36:16 PDT

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

Rafael Brandao

Comment 19 2012-04-25 16:24:25 PDT

Ryosuke Niwa

Comment 20 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.

Ryosuke Niwa

Comment 21 2012-04-26 04:44:34 PDT

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

Ryosuke Niwa

Comment 22 2012-04-26 19:37:18 PDT

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component CSS

Assignee

Ryosuke Niwa

Reported

2012-04-03 06:37 PDT

Modified

2012-04-26 19:37 PDT History

CC List

5 users Show

URL

http://tinymce.moxiecode.com/webkit/list_style_type.html

Keywords

Duplicates (1)

84618 View as bug list

Depends on

Blocks