stylo: preshints (mapped attributes) not handled correctly for elements not in a document

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
P2
normal
4 months ago
4 months ago

People

(Reporter: bz, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

(Reporter)

Description

4 months ago
Testcase: 

  <!DOCTYPE html>
  <script>
  var f = document.createElement("font");
  f.setAttribute("color", "green");
  alert(getComputedStyle(f).color);
  </script>

Should alert "rgb(0, 128, 0)" but alerts "rgb(0, 0, 0)".

The problem, of course, is that mapped attributes only go in the nsHTMLStyleSheet hashtable when the node is bound to a document (so GetComposedDoc becomes non-null).  But nsMappedAttributeElement::WalkContentStyleRules walks the mapped attributes unconditionally.  So resolving style for a node that's not in the document works fine in Gecko.  But in Servo, ServoStyleSet::ResolveMappedAttrDeclarationBlocks only computes servo declarations for things in the nsHTMLStyleSheet.  And then Gecko_GetHTMLPresentationAttrDeclarationBlock will return null, because even though we have an nsMappedAttributes it has no servo declaration block.

One thing we _could_ try is using the owner, not composed, doc for the nsHTMLStyleSheet.  We'd then be able to remove the BindToTree code that currently resets the nsHTMLStyleSheet, and will instead need something in adoption to do so, when the owner doc changes.  This might cause performance regressions in situations where we currently set multiple mapped attrs on an element that is not in the document.  I _think_ that doesn't happen in the most common mapped attr case, when coming from the non-fragment parser, but would need to check.  And we'd need to measure the innerHTML setter impact...

The other option, of course, is to have elements registering "dirty" nsMappedAttributes with the nsHTMLStyleSheet in some way....
Priority: -- → P2
(Reporter)

Comment 1

4 months ago
I expect the fix for bug 1352898 fixed this.  Need to check.
Depends on: 1352898
(Reporter)

Comment 2

4 months ago
It helped, but did not fix.

The new behavior is pretty bizarre.  The testcase in comment 0 still shows the bug, but if I just add a <body> element the problem goes away (!).  That's because if there is no <body> we never call ServoStyleSet::PreTraverseSync, but if there is one then we do.  I think because the style flush from the getComputedStyle call triggers frame construction for the <body> and hence a servo traversal, which _happens_ to update stuff.

If I modify the testcase to flush layout at the start of the <script>, before setting the "color" attr on the <font>, then even with a <body> things don't work.

If I add a PreTraverseSync call to ServoStyleSet::ResolveTransientStyle, the problem goes away.  That would match the fact that nsComputedDOMStyle::DoGetStyleContextNoFlush just calls ResolveTransientStyle.=

But walking all the mapped attrs every time ResolveTransientStyle is called is horrible.  We should either walk only the ones that are actually dirty or do that walk off layout flushes and make sure that addition of a mapped attrs without a servo decl block marks us as needing a layout flush or something.
(Reporter)

Updated

4 months ago
Summary: stylo: preshints not handled correctly for elements not in a document → stylo: preshints (mapped attributes) not handled correctly for elements not in a document
You need to log in before you can comment on or make changes to this bug.