Bug 45778 – CSSStyleDeclaration.prototype.setProperty does not set priority correctly

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: DOM

[Nikita Vasilyev]

Created attachment 67597 [details]
CSSStyleDeclaration.prototype.setProperty

The reduction fails in WebKit and Opera.

3 FAIL 
Expected: '' 
Actual: 'important'

4 FAIL 
Expected: '' 
Actual: 'important'

5 FAIL 
Expected: '' 
Actual: 'important'

Works well in Firefox 4.0b5.

[Xianzhu Wang]

Created attachment 73169 [details]
test case of ignored setProperty()

For the failing cases, setProperty() sets neither priority nor property value.
A setProperty() call will be ignored if
a) the property has 'important' priority before the call;
and
b) the third parameter of the call is omitted, null or empty string

[Jason Liu]

Created attachment 95873 [details]
Patch

Root cause: setProperty will fail when the style is !important.

[Alexey Proskuryakov]

Comment on attachment 95873 [details]
Patch

+        Fixed bug https://bugs.webkit.org/show_bug.cgi?id=45778.

Please see existing entries in ChangeLog for preferred format. There should be bug title and bug URL.

+        No new tests. (OOPS!)

A test is definitely needed for a fix like this, please add it.

[Jason Liu]

Created attachment 96207 [details]
patch

patch

[Simon Fraser (smfr)]

Comment on attachment 96207 [details]
patch

Looks like your patch is backwards. It's removing code.

[Jason Liu]

Created attachment 96211 [details]
Patch

Patch

[Ryosuke Niwa]

Comment on attachment 96211 [details]
Patch

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

> LayoutTests/css1/font_properties/font_style_setProperty.html:4
> +<TITLE>CSS1 Test Suite: change value of an important property by setting JS property</TITLE>

Is this really a part of CSS1 test suite?  Could you tell us where you copied it from?

[Jason Liu]

(In reply to comment #7)
> (From update of attachment 96211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96211&action=review
> 
> > LayoutTests/css1/font_properties/font_style_setProperty.html:4
> > +<TITLE>CSS1 Test Suite: change value of an important property by setting JS property</TITLE>
> 
> Is this really a part of CSS1 test suite?  Could you tell us where you copied it from?

I wrote it by myself.
Is there anything wrong?
Thanks for your help.

[Alexey Proskuryakov]

Comment on attachment 96211 [details]
Patch

LayoutTests/css1 is a location for official CSS 1 test suite. This new test should be added to fast/dom/CSSStyleDeclaration directory.

It should also be a text-only test, so that all platforms could share results without trouble. You can read back resulting style with getComputedStyle(). I recommend that you re-create the test using make-new-script-test script, which will add boilerplate for this style of test:

make-new-script-test fast/dom/CSSStyleDeclaration/setProperty-important.html

Sorry about slow review turnaround.

[Marc Hoyois]

> A setProperty() call will be ignored if
> a) the property has 'important' priority before the call;
> and
> b) the third parameter of the call is omitted, null or empty string

This is still an issue in the latest WebKit…
The current behavior does not respect the algorithm for setProperty described here: http://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-setproperty

[Ahmad Saleem]

Created attachment 460468 [details]
Safari 15.5 matches other browsers

I took a test case from the patch and changed it into following JSFiddle:

Link - https://jsfiddle.net/vsx91qgy/show

As seen in the attached screenshot, all browsers have same output and only difference from expected result is on d5 <div>.

I am not sure whether the above behavior is aligned with the web-spec or not (as mentioned in Comment 10) but since it is matching all behavior, should we consider it as "RESOLVED INVALID" or "RESOLVED CONFIGURATION CHANGED"? If web-spec have been updated and it is intended output then can we consider it - "RESOLVED INVALID"? In case, if web-spec need clarity, can it be raised by someone separately? Thanks!