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

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
ASSIGNED
3 months ago
14 hours ago

People

(Reporter: emilio, Assigned: wcpan)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 months 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>

Comment 4

3 months ago
> 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.
Nominate because this is the proper fix for bug 1358495, which is a [qf:p1]
Whiteboard: [qf]
Inherited from QF bug 1358495.
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [qf] → [qf:p3]
Boris is this bug only applicable if stylo happens or is it a fix for with and without stylo? If it is dependent on stylo and only if stylo is enabled we think it may not be a QF p1.
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 11

a month ago
(In reply to Naveed Ihsanullah [:naveed] from comment #10)
> Boris is this bug only applicable if stylo happens or is it a fix for with
> and without stylo? If it is dependent on stylo and only if stylo is enabled
> we think it may not be a QF p1.

It should be independent of stylo, though I initially filed it with stylo in mind.
Flags: needinfo?(bzbarsky)
Summary: stylo: Only flush styles on nsComputedDOMStyle::UpdateCurrentStyleSources if we know that the child or one of its ancestors need a restyle. → Only flush styles on nsComputedDOMStyle::UpdateCurrentStyleSources if we know that the child or one of its ancestors need a restyle.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> (In reply to Naveed Ihsanullah [:naveed] from comment #10)
> > Boris is this bug only applicable if stylo happens or is it a fix for with
> > and without stylo? If it is dependent on stylo and only if stylo is enabled
> > we think it may not be a QF p1.
> 
> It should be independent of stylo, though I initially filed it with stylo in
> mind.

I disagree with the triage decision based on this.  Putting it back in the queue.  :-)
Whiteboard: [qf:p3] → [qf]
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #12)

> I disagree with the triage decision based on this.  Putting it back in the
> queue.  :-)

Especially as bug 1358495 has been wontfixed due to this being expected to happen soon.
Given independence of this bug and stylo, and given that bug 1358495 was [qf:p1], I'm going to qf:p1 this bug.
Whiteboard: [qf] → [qf:p1]
WIP: some stylo tests failed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f51687d180faa4e67cc4cff103e174a67aeeb1a
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
All tests passed, but I'm not sure why I got some failure in bug 1379267.
I think it should not related to these patches.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cecba1334a1c17bb5a4a78469e4e73c41e0e1a6

Comment 19

14 hours ago
mozreview-review
Comment on attachment 8889295 [details]
Bug 1363805 - Part 1: Add a flag to nsIDocument::FlushPendingNotification.

https://reviewboard.mozilla.org/r/160346/#review166126

r=me with this.

::: commit-message-5928d:1
(Diff revision 1)
> +Bug 1363805 - Part 1: Add a flag to nsIDocument::FlushPendingNotification. r?heycam

"FlushPendingNotifications"

::: dom/base/FlushType.h:41
(Diff revision 1)
> +enum class FlushTarget : uint8_t {
> +  Normal = 0,
> +  ParentOnly = 1,

Please document the purpose of the class and the Normal and ParentOnly values.

::: dom/base/FlushType.h:44
(Diff revision 1)
>  };
>  
> +enum class FlushTarget : uint8_t {
> +  Normal = 0,
> +  ParentOnly = 1,
> +  Count

If the Count enum is not used anyway, please remove it.

::: dom/base/nsIDocument.h:1582
(Diff revision 1)
> -  virtual void FlushPendingNotifications(mozilla::FlushType aType) = 0;
> +  virtual void FlushPendingNotifications(mozilla::FlushType aType,
> +                                         mozilla::FlushTarget aTarget) = 0;

I think the second argument should take FlushTarget::Normal as a default value, since that's the case for 99% of FlushPendingNotification calls.  Then this patch can shrink a fair bit.
Attachment #8889295 - Flags: review?(cam) → review+
You need to log in before you can comment on or make changes to this bug.