stylo: Only flush styles on nsComputedDOMStyle::UpdateCurrentStyleSources if we know that the child or one of its ancestors need a restyle.

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
18 days ago
9 days ago

People

(Reporter: emilio, Assigned: wcpan)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

18 days ago
Blink does this, and could be a nice perf win.
A few things that come to mind:

* There might be tests that call getComputedStyle on some element, assuming that's sufficient to flush all styles, invalidating the test (but not causing it to fail).  Not sure what the best way to check for that would be -- we maybe want to know whether a test did a getComputedStyle call and threw away the result (which would be the most common pattern).

* Can we do anything for properties that need layout flushed too?  Probably not.

* We'll still need to do the parent document layout flush in nsDocument::FlushPendingNotifications.
Boris, what's the reason here for flushing styles on the document from the pres shell we got passed in rather than on mContent's document?

http://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/layout/style/nsComputedDOMStyle.cpp#761

I guess that means that with:

  <iframe src="somewhere.html"></iframe>
  <script>
    getComputedStyle(document.querySelector("iframe").contentDocument.querySelector("div")).color
  </script>

we'll flush in the other document and not in the inner document.  Why is that what we want to do?
Flags: needinfo?(bzbarsky)
Or, with the actual test I meant to paste:

<!DOCTYPE html>
<script>
function run() {
  var p = document.querySelector("iframe").contentDocument.querySelector("p");
  p.style.color = "green";
  alert(getComputedStyle(p).color);
}
</script>
<iframe srcdoc="<p>Hello</p>" onload="run()"></iframe>
> There might be tests that call getComputedStyle on some element, assuming that's sufficient to flush all styles

True.  Ideally people would be computing the style the actually care to make sure is flushed, but they might be pretty bad at it...

We could try to audit for this or something.  Not sure how feasible that would be.

> * Can we do anything for properties that need layout flushed too?  Probably not.

Probably not...

> what's the reason here for flushing styles on the document from the pres shell we got passed in rather than on mContent's document

Ah, the fun of CSSOM.

So per spec, when you somewindow.getComputedStyle(someelement), that means resolve style for someelement using the CSS rules from _somewindow_'s document.  How inline style interacts with all this is not very clear.

What UAs _actually_ do is also unclear.  For elements which do not have a box, I believe they more or less follow the spec as written.  For elements which do have a box, I think UAs tend to return the styles of that box, which has nothing to do with the spec.  Certainly that's what we do.  Note that for the properties where the resolved value is the used value it's really not clear what the spec's language would even mean; are we supposed to do layout on the one document with CSS rules from the other document or something?

I was pretty sure these issues had all been raised on the spec, but I can't locate them right now.  It's possible they didn't get migrated from the multiple previous issue trackers the spec had (including the www-style mailing list).  Someone should probably get these filed; it would be ideal if this were someone who has the bandwidth to follow up on them, which is not me right now.

In any case, we could flush _both_ documents in cases when they differ, I guess, to fix testcases like comment 3.  But in practice, using a document other than the ownerDocument of the element for getComputedStyle will mostly lead people to grief on the web.  :(
Flags: needinfo?(bzbarsky)
OK.  So perhaps for the purposes of making getComputedStyle flushing skippable, we don't need to try to optimize the case where the window getComputedStyle was called on is different from the window the element came from.  But we should probably file a bug to sort out what we really should be doing in those cases.

Per discussion with wcpan just now, it sounds like this improvement to getComputedStyle is good enough for the use cases that bug 1358495 had in mind for getComputedStyleWithoutFlushing, and then we don't have to find a solution for the "pres shell has disappeared so we really ought to flush to get it back" issue from that bug.  Wei-Cheng, do you want to take this bug?
Flags: needinfo?(wpan)
I'll take a look, as this bug could probably solve bug 1358495 in a saner way.
Flags: needinfo?(wpan)
Assignee: nobody → wpan
Priority: -- → P2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=653e17d2080fa85116762c49c4c0de225fc34ea3

So I got some oranges. Need to verify the usage for those testcases.
You need to log in before you can comment on or make changes to this bug.