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
(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.
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.
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.
(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.
*** Bug 84618 has been marked as a duplicate of this bug. ***
Okay, we do have to fix this bug.
Created attachment 138654 [details] Fixes the regression
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.
(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 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.
(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 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 on attachment 138654 [details] Fixes the regression Attachment 138654 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12528378
Created attachment 138692 [details] Patch
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?
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 :-(
Committed r115227: <http://trac.webkit.org/changeset/115227>
Build fix landed in http://trac.webkit.org/changeset/115244.
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).
(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.
Another build fix landed in http://trac.webkit.org/changeset/115302.
Committed r115399: <http://trac.webkit.org/changeset/115399>