Closed Bug 1366964 Opened 8 years ago Closed 8 years ago

Update style context generation after style resolved

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
52 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: wcpan, Assigned: wcpan)

Details

Attachments

(1 file)

Bug 1366964 - Update style context generation after style resolved.
59 bytes, text/x-review-board-request
heycam
: review+
Details
It was accidentally removed in bug 1203766 during patch fixing.
Comment on attachment 8870291 [details] Bug 1366964 - Update style context generation after style resolved. https://reviewboard.mozilla.org/r/141758/#review145366 ::: layout/style/nsComputedDOMStyle.cpp:918 (Diff revision 1) > nsStyleSet* styleSet = mPresShell->StyleSet()->AsGecko(); > RefPtr<nsStyleContext> unanimatedStyleContext = > styleSet->ResolveStyleByRemovingAnimation( > mContent->AsElement(), mStyleContext, eRestyle_AllHintsWithAnimations); > SetResolvedStyleContext(Move(unanimatedStyleContext)); > + mStyleContextGeneration = currentGeneration; Do we also need to set it after the SetFrameStyleContext call? r=me with that, or alternatively, just set once right at the end of the function.
Attachment #8870291 - Flags: review?(cam) → review+
Updated patch, waiting for try build.
Test case failed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c7ec3990b149750e8ac0f6f602af1b5131b6de0&selectedJob=101150113 Looks like I got something wrong when the element attribute changes after a style property computed.
So out-of-tree attribute modifications do not increase restyle generation. We probably need to examine pending restyle flags of itself, and its children and ancestors.
> So out-of-tree attribute modifications do not increase restyle generation. Is that add a pending restyle for elements out of the document, but never processed them? Or do we not add a pending restyle for elements out of the document at all? Can you back to when the mStyleContextGeneration stuff was not broken, and see how we dealt with this case?
(In reply to Cameron McCormack (:heycam) from comment #7) > > So out-of-tree attribute modifications do not increase restyle generation. > > Is that add a pending restyle for elements out of the document, but never > processed them? Or do we not add a pending restyle for elements out of the > document at all? > > Can you back to when the mStyleContextGeneration stuff was not broken, and > see how we dealt with this case? This will trigger mozilla::RestyleTracker::AddPendingRestyle e = document.createElement("div"); document.body.appendChild(e); e.className = "g"; while this don't: e = document.createElement("div"); e.className = "g";
I think we can skip the restyle generation check for out-of-tree element, because we cannot predict how deep is the tree to be. Since computing out-of-tree element style should be an edge case, simply checking node->IsInComposedDoc() should enough.
But this optimization does not work with Stylo, because ServoRestyleManager::AttributeChanged does not call PostRestyleEvent in some cases, but GeckoRestyleManager::AttributeChanged does if it needs. For example, in testcase layout/style/test/test_bug437915.html, changing attribute "title" does not trigger ServoRestyleManager::PostRestyleEvent, so the restyle generation does not change. Is this intended for Stylo to optimize in this way, which is different from Gecko?
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(xidorn+moz) → needinfo?(cam)
Yes, Gecko handles attribute changes by checking immediately whether it needs to restyle the element (or some descendant) at the time the attribute change is made -- and calls PostRestyleEvent if it does. With stylo, changing the attribute will cause a snapshot to be taken so that it can compare the before- and after-values of the attributes later, when flushing style. In ServoRestyleManager::SnapshotFor we call nsIPresShell::EnsureStyleFlush, which is what will cause us to flush style later. Really, the restyle generation should not change until *after* we flush the styles. That's why we check the restyle generation after the style flush in UpdateCurrentStyleSources.
Flags: needinfo?(cam)
I had a trace through that test case, and I think I found the problem. It is related to the fact that we are restyling a display:none element. Here we get the frame for the element that has just been restyled: http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/layout/base/ServoRestyleManager.cpp#357-358 Because the <div> here is display:none, it has no frame. Which means we decide not to create a new nsStyleContext, because there's no frame to set it on. Which means the |recreatedAnyContext| return value is false, which is what we use to determine whether to bump the generation.
I see. When I remove display: none in the test case, all tests were passed. Can we reasonably find out when should we rely on restyle generation counter or not? e.g. check if it is not visible Or we can just check if we are using Servo, just like MustReresolveStyle?
The call to Servo_TakeChangeHint at the top of ServoRestyleManager::ProcessPostTraversal takes the nsRestyleHint that is stored in the ServoElementData's RestyleData, then drops the RestyleData. Also in the RestyleData (which actually just landed today) is a flag that indicates whether the styles changed: https://hg.mozilla.org/integration/autoland/file/7e97fe54970f/servo/components/style/data.rs#l354 https://hg.mozilla.org/integration/autoland/file/7e97fe54970f/servo/components/style/data.rs#l416 So maybe we can make Servo_TakeChangeHint also return this flag. Then we could OR it in the ProcessPostTraversal return value.
That should make the restyle generation counter accurate.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b392ae936f43bd9432ad6f4b2041e06b2d02b8b2 A WIP that exposes RestyleData.is_restyle() from Servo_takeChangeHint back to C++. At least this passes layout/style/test/test_bug437915.html. Waiting for try result.
The failed test case is layout/style/test/test_transitions_computed_values.html. I think the problem is the parent div does not set the dirty-descendants bit when we alter the style of its child, so ProcessPostTraversal does not traverse into the child. Maybe it's because we don't have style::data::ElementData for a hidden element.
Assignee: nobody → wpan
Attachment #8870291 - Flags: review+ → review?(cam)
Should have passed all tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7c0f50b380416b1e98388f87def78c21a3ffafa Because Servo_StyleSet_MightHaveAttributeDependency does not aware non-styled elements, so I cannot use the optimization. Probably not as good as the original intention (for invisible elements), but this should still help.
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #19) > Because Servo_StyleSet_MightHaveAttributeDependency does not aware > non-styled elements, so I cannot use the optimization. I don't understand this. Stylo_StyleSet_MightHaveAttributeDependency doesn't take an element as an argument. It just returns whether a given attribute name has appeared anywhere in a style sheet. Why can't we call IncrementMaybeRestyleGeneration() after the StyleSet()->MightHaveAttributeDependency call?
Flags: needinfo?(wpan)
(In reply to Cameron McCormack (:heycam) from comment #20) > I don't understand this. Stylo_StyleSet_MightHaveAttributeDependency > doesn't take an element as an argument. It just returns whether a given > attribute name has appeared anywhere in a style sheet. Why can't we call > IncrementMaybeRestyleGeneration() after the > StyleSet()->MightHaveAttributeDependency call? So I misunderstood that function. It returns false because the attribute is "style". I tried to add the generation counter in PostRestyleEvent, but it does not always call if we are changing "class" for a hidden element, or if it does not actually change anything. http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/layout/base/ServoRestyleManager.cpp#853 So I think I can add the increment in AttributeWillChange before taking the snapshot, then we can cover all cases (hopefully).
Flags: needinfo?(wpan)
Comment on attachment 8870291 [details] Bug 1366964 - Update style context generation after style resolved. Not sure why MozReview taken down the review flag, instead of setting r?.
Attachment #8870291 - Flags: review?(cam)
Comment on attachment 8870291 [details] Bug 1366964 - Update style context generation after style resolved. https://reviewboard.mozilla.org/r/141758/#review157442 ::: layout/base/RestyleManager.h:42 (Diff revision 4) > NS_INLINE_DECL_REFCOUNTING(mozilla::RestyleManager) > > // Get an integer that increments every time we process pending restyles. > // The value is never 0. > uint32_t GetRestyleGeneration() const { return mRestyleGeneration; } > + uint32_t GetMaybeRestyleGeneration() const { return mMaybeRestyleGeneration; } Nit: I'm not sure about the name "maybe restyle generation" now. Since this is just for handling undisplayed elements, what do you think about calling it "GetUndisplayedRestyleGeneration"? (And similarly for the other functions/variables.) ::: layout/base/RestyleManager.h:244 (Diff revision 4) > + // Ensure mMaybeRestyleGeneration > 0, because the same reason of > + // IncrementRestyleGeneration. Nit: s/because the same reason of/for the same reason as/ ::: layout/base/RestyleManager.h:262 (Diff revision 4) > + // Unlike mRestyleGeneration, which means the actual restyling count, > + // mMaybeRestyleGeneration represents any possible style changes that can > + // cause restyling. This is needed for getComputedStyle to work with > + // non-styled (e.g. display: none) elements. > + uint32_t mMaybeRestyleGeneration; s/style changes/DOM changes/ Can you also please move this comment up to the GetMaybeRestyleGeneration() declaration above. ::: layout/base/ServoRestyleManager.cpp:139 (Diff revision 4) > > if (aRestyleHint == 0 && !aMinChangeHint) { > return; // Nothing to do. > } > > + IncrementMaybeRestyleGeneration(); If aMinChangeHint has some change hints, but aRestyleHint is 0, then we're not going to actually restyle anything, and we're just using this PostRestyleEvent call to process the change hint. So let's wrap this with if (aRestyleHint) { ... } Can you please also add a comment here saying that we assume that if we are posting a restyle hint, then it's likely we will have some style changes. And that if we have any nsComputedDOMStyle objects for undisplayed elements, then we must invalidate their styles, since we don't know if any of the restyling that we do would affect undisplayed elements. ::: layout/base/ServoRestyleManager.cpp:967 (Diff revision 4) > (aAttribute == nsGkAtoms::border && > aElement->IsHTMLElement(nsGkAtoms::table)); > } > > void > ServoRestyleManager::AttributeWillChange(Element* aElement, Why don't we need to call IncrementMaybeRestyleGeneration in ContentStateChanged too? ::: layout/base/ServoRestyleManager.cpp:990 (Diff revision 4) > aAttribute == nsGkAtoms::lang || > StyleSet()->MightHaveAttributeDependency(*aElement, aAttribute))) { > return; > } > > + IncrementMaybeRestyleGeneration(); Similarly here, please add a comment saying that we can't tell if an attribute change will affect the styles of undisplayed elements, since we don't actually restyle undisplayed elements during the restyle traversal. So we just assume that any attribute change could cause an undisplayed element's style to change. ::: layout/style/nsComputedDOMStyle.cpp:825 (Diff revision 4) > uint64_t currentGeneration = > - mPresShell->GetPresContext()->GetRestyleGeneration(); > + mPresShell->GetPresContext()->GetMaybeRestyleGeneration(); Please add a comment here saying that we need to use GetMaybeRestyleGeneration rather than GetRestyleGeneration, because the caching of mStyleContext is an optimization that's useful only for undisplayed elements, and for undisplayed elements we need to take into account any DOM changes that might cause a restyle. And point out that for a Gecko style backend, using GetRestyleGeneration() would be sufficient, since it is incremented whenever we process restyles, whereas for Servo that counter is only incremented when we have resolved new styles on at least one element after processing restyles. ::: layout/style/nsComputedDOMStyle.cpp:829 (Diff revision 4) > - if (mStyleContextGeneration == currentGeneration) { > + // If the mContent is out-of-document, because it does not restyle at all, > + // we always need to update the style context to ensure it is up-to-date. I think the reason we need this is not just that elements outside of the document aren't restyled (since elements in display:none subtrees that are in the document are also not restyled), but that we can't easily detect DOM changes to elements not in the document, and so the generation won't have increased. Still, it seems a shame that we can't make something like this: <script> var e = document.createElement("span"); var cs = getComputedStyle(e); console.log(cs.color); console.log(cs.fontSize); console.log(cs.marginTop); </script> avoid re-resolving style each time we inspect a property on the computed style object. ::: layout/style/nsComputedDOMStyle.cpp:831 (Diff revision 4) > - mPresShell->GetPresContext()->GetRestyleGeneration(); > + mPresShell->GetPresContext()->GetMaybeRestyleGeneration(); > > if (mStyleContext) { > - if (mStyleContextGeneration == currentGeneration) { > + // If the mContent is out-of-document, because it does not restyle at all, > + // we always need to update the style context to ensure it is up-to-date. > + nsCOMPtr<nsINode> node = do_QueryInterface(mContent); mContent is an nsCOMPtr<nsIContent>, and nsIContent inherits from nsINode, so no need for this. You can just call IsInComposedDoc() on mContent directly. ::: layout/style/nsComputedDOMStyle.cpp:921 (Diff revision 4) > SetResolvedStyleContext(Move(resolvedStyleContext)); > + mStyleContextGeneration = currentGeneration; How about we pass currentGeneration as an argument to SetResolvedStyleContext, and have it update mStyleContextGeneration.
Boris, do you have any ideas on how we can handle elements not in the document here? Because we don't get DOM mutation notifications for disconnected elements, it seems hard to know whether we've had any changes that might invalidate our cached styles of disconnected elements. There's not some kind of DOM generation counter we could use, is there? I see nsMutationGuard::sGeneration, but I guess that considers all documents.
Flags: needinfo?(bzbarsky)
nsMutationGuard is just for noticing reentrant mutation via mutation events. There's not concept of DOM generation that I know of. One simple option is to restrict the caching bug 1203766 added to elements that are in the DOM, no?
Flags: needinfo?(bzbarsky)
Addressed all issues. Also added IncreamentUndisplayedRestyleGeneration() to several places because the refactor in bug 1378190.
Comment on attachment 8870291 [details] Bug 1366964 - Update style context generation after style resolved. https://reviewboard.mozilla.org/r/141758/#review161510 r=me with these comments addressed. Thanks for fixing this! ::: layout/base/ServoRestyleManager.cpp:133 (Diff revision 5) > > if (aRestyleHint == 0 && !aMinChangeHint) { > return; // Nothing to do. > } > > + // Assuming the change hints will invalidate cached style for s/change hints/restyle hints/ ::: layout/base/ServoRestyleManager.cpp:949 (Diff revision 5) > + // Assuming we need to invalidate cached style in getComputedStyle for > + // undisplayed elements, since we don't know if it is needed. > + IncrementUndisplayedRestyleGeneration(); I think we need to do this even if restyleHint or changeHint is non-zero, and the reason is that for stylo, it's the snapshot that takes into account normal content state changes. (The restyleHint and changeHint return values from ContentStateChangedInternal are for some special handling, e.g. forcing a frame reconstruction when an image has finished loading, or repainting frames when some state changes that affect themed widget rendering.) So just move this out of the if statement. ::: layout/base/ServoRestyleManager.cpp:1107 (Diff revision 5) > + // Assuming we need to invalidate cached style in getComputedStyle for > + // undisplayed elements, since we don't know if it is needed. > + IncrementUndisplayedRestyleGeneration(); This case is a little different from ContentStateChanged, since for attribute changes that affect the result of selector matching, the undisplayed restyle generation will be incremented in TakeSnapshotForAttributeChange (as called from AttributeWillChange) above. So here, we only have to care about restyle hints generated from just above. We don't need to increment the restyle generation if we have a change hint but no restyle hint, so let's move this out of the if statement, and do it just |if (restyleHint)|. ::: layout/style/nsComputedDOMStyle.cpp:842 (Diff revision 5) > - if (mStyleContextGeneration == currentGeneration) { > + // If the mContent is out-of-document, because it does not restyle at all, > + // we always need to update the style context to ensure it is up-to-date. I would reword this to say about how we can't use the undisplayed restyle generation to handle out of document elements. So maybe something like: // We can't rely on the undisplayed restyle generation if // mContent is out-of-document, since that generation is not // incremented for DOM changes on out-of-document elements. // So we always need to update the style context to ensure it // it up-to-date.
Attachment #8870291 - Flags: review?(cam) → review+
Pushed by wpan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b46807aa128f Update style context generation after style resolved. r=heycam
https://hg.mozilla.org/mozilla-central/rev/b46807aa128f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: