Closed Bug 1281670 Opened 5 years ago Closed 4 years ago

Style attribute is not parsed into style object under certain conditions

Categories

(Core :: DOM: Core & HTML, defect)

47 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: neil, Assigned: edgar)

References

Details

Attachments

(1 file, 6 obsolete files)

Attached file testcase.html (obsolete) —
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36

Steps to reproduce:

When using the DOMParser to parse and sanitise a document outside of the "live" document in the page:
1. Read properties on the style object of an element
2. Remove the style property of the element
3. Add back a new style property to the element
4. Try to read a property on the style object of the element


Actual results:

The style object is empty (the style attribute has not been parsed). Worse, if you modify a property on the style object, it overwrites the whole style attribute currently set.


Expected results:

The style object contains a parsed version of the attribute. Setting a property on the style object does not modify independent style properties.

Minimal test case attached. Tested in latest stable release (Firefox 47.0).

We use this access pattern as part of our rendering process for emails at https://www.fastmail.com and this bug is resulting in Firefox users seeing degraded fidelity in rendering emails compared to users of other browsers.
Component: Untriaged → DOM
Product: Firefox → Core
Hi Edgar, could you please take a look? Thanks!
Flags: needinfo?(echen)
Assignee: nobody → echen
Flags: needinfo?(echen)
I did some tests, 

1. If you modify a property on the style object without removing and calling "setAttribute",
   it works good.
   e.g. http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4329

2. If you remove attribute and add back by modifying the property on the style object
   (instead of calling "setAttribute"), it works good.
   e.g. http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4330

3. If you call "setAttribute" (even doesn't remove attribute first), issue happens.
   e.g. http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4332

It looks like the issue is related to "setAttribute".
(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> It looks like the issue is related to "setAttribute".

Another clue: issue happens only when the style object has been accessed.

e.g.

works good:
  var doc = (new DOMParser()).parseFromString('<div style="color:red">Foo</div>', 'text/html');
  var div = doc.querySelector('div');
  div.setAttribute('style', 'color:green');

issue happens:
  var doc = (new DOMParser()).parseFromString('<div style="color:red">Foo</div>', 'text/html');
  var div = doc.querySelector('div');
  var foo = div.style; // just access style object, do nothing
  div.setAttribute('style', 'color:green');
See Also: → 418214
Hmm.  Since we do in fact keep around the string value nowadays, it might just be possible to back the patch for bug 418214 out entirely...
(In reply to Boris Zbarsky [:bz] from comment #4)
> Hmm.  Since we do in fact keep around the string value nowadays, it might
> just be possible to back the patch for bug 418214 out entirely...

Thanks for the comment. I will try it.
On the other hand, we'd probably not want to parse it on initial attr set, for performance reasons.  We'd just want to keep reparsing it once we'd parsed it once...
(In reply to Boris Zbarsky [:bz] from comment #6)
> On the other hand, we'd probably not want to parse it on initial attr set,
> for performance reasons.  We'd just want to keep reparsing it once we'd
> parsed it once...

Got it.
Attached patch Patch, v1 (obsolete) — Splinter Review
Attached patch Patch, v2 (obsolete) — Splinter Review
Attachment #8775920 - Attachment is obsolete: true
Comment on attachment 8776433 [details] [diff] [review]
Patch, v2

Review of attachment 8776433 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Boris Zbarsky [:bz] from comment #6)
> On the other hand, we'd probably not want to parse it on initial attr set,
> for performance reasons.  We'd just want to keep reparsing it once we'd
> parsed it once...

I keep the changes of bug 418214 since we don't want to parse style on initial attr set.
But add a condition in ParseStyleAttribute() to keep reparsing style once we had parsed it.

Hi Boris, may I have your review? Thank you.
Attachment #8776433 - Flags: review?(bzbarsky)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8776433 [details] [diff] [review]
Patch, v2

>+      DOMSlots()->mStyle ||

This will force slots allocation, which is not needed here.  What you want is to add a helper that calls GetExistingDOMSlots() and if that returns non-null returns its mStyle, otherwise returns null.  Or just returns a boolean, really...

The test should probably be a web platform test, since this is all covered by specs.

r=me with those fixed.
Attachment #8776433 - Flags: review?(bzbarsky) → review+
Attached patch Patch, v3, r=bz (obsolete) — Splinter Review
Address review comment #12.
Attachment #8776433 - Attachment is obsolete: true
Attachment #8778755 - Flags: review+
Attached patch Add web-platform-test, v1 (obsolete) — Splinter Review
The web platform test  needs a title.  And do you still need the mochitest?
(In reply to Boris Zbarsky [:bz] from comment #15)
> The web platform test  needs a title.  And do you still need the mochitest?

Thanks for reminding me. No, we don't need the mochitest.
Attached patch Patch, v4 (obsolete) — Splinter Review
Address comment#15,
1) squash patches
2) add title in wpt 
3) remove mochitest test

Hi Boris, may I have your review again, in case I missed something?
Thank you.
Attachment #8778755 - Attachment is obsolete: true
Attachment #8778756 - Attachment is obsolete: true
Attachment #8779207 - Flags: review?(bzbarsky)
Comment on attachment 8779207 [details] [diff] [review]
Patch, v4

>+    assert_equals(div.getAttribute("style"), 'color: red',
>+                  'Shouldn not affect style attribute');
>+}, 'Parsing of initial style attribute');

"Should not affect"?  Same elsewhere.  And it might be better to label this assert_equals as "Value of style attribute should match the string value that was set" or something.

r=me
Attachment #8779207 - Flags: review?(bzbarsky) → review+
Attached patch Patch, v5, r=bzSplinter Review
Address review comment #18.
Attachment #8764443 - Attachment is obsolete: true
Attachment #8779207 - Attachment is obsolete: true
Attachment #8779651 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1148ab6819
Keep reparsing style attribute if we had parsed it once. r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff1148ab6819
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.