Closed
Bug 1286445
Opened 8 years ago
Closed 8 years ago
stylo: Make hover partially work for non pseudo-elements.
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(3 files)
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63872/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63872/
Attachment #8770397 -
Flags: review?(cam)
Attachment #8770398 -
Flags: review?(cam)
Assignee | ||
Comment 2•8 years ago
|
||
This includes, for example :hover. Review commit: https://reviewboard.mozilla.org/r/63874/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63874/
Comment 3•8 years ago
|
||
Comment on attachment 8770397 [details] Bug 1286445: Stub out HasStateDependentStyle. https://reviewboard.mozilla.org/r/63872/#review60936 r=me with these changes. ::: layout/base/ServoRestyleManager.cpp:25 (Diff revision 1) > +static > +void DirtyTree(nsIContent* aContent) Nit: "static void" one line, rest on the next. ::: layout/base/ServoRestyleManager.cpp:28 (Diff revision 1) > + if (aContent->IsDirtyForServo()) > + return; Nit: prefer {}s around single line statements. ::: layout/base/ServoRestyleManager.cpp:34 (Diff revision 1) > + uint32_t childrenCount; > + nsIContent* const* children = aContent->GetChildArray(&childrenCount); > + > + MOZ_ASSERT(children); > + > + for (uint32_t i = 0; i < childrenCount; ++i) > + DirtyTree(children[i]); Let's use a FlattenedChildIterator to iterate over the descendants. That will traverse into shadow trees. Like this: http://searchfox.org/mozilla-central/rev/c44d0b1673fef5e0e2e19fa82d6780a74c186151/layout/base/RestyleManager.cpp#3042 ::: layout/base/ServoRestyleManager.cpp:64 (Diff revision 1) > - aElement->SetIsDirtyForServo(); > + // Propagate the IS_DIRTY flag down the tree. > + DirtyTree(aElement); > + > + // Propagate the HAS_DIRTY_DESCENDANTS flag to the root. > nsINode* cur = aElement; > while ((cur = cur->GetParentNode())) { (This might need to be GetParentElementCrossingShadowRoot to break out of shadow trees.)
Attachment #8770397 -
Flags: review?(cam) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8770398 [details] Bug 1286445: stylo: Support restyles of non-pseudo content on state change. https://reviewboard.mozilla.org/r/63874/#review60942 r=me with the following addressed. ::: layout/base/ServoRestyleManager.h:76 (Diff revision 1) > + * Traverses the frame tree of dirty content fixing up the style contexts of > + * the elements. I think we're traversing the content tree, rather than the frame tree here. Also "fixing up the style contexts" makes me think of nsStyleContext::ApplyStyleFixups, so how about something like this instead: Traverses a tree of content that Servo has just restyled, recreating style contexts for their frames with the new style data. ::: layout/base/ServoRestyleManager.h:83 (Diff revision 1) > + * > + * This is just static so ServoStyleSet can mark this class as friend, so we > + * can access to the GetContext method without making it available to everyone > + * else. > + */ > + static void CorrectFrameStyleContexts(nsIContent* aContent, Just a naming nit: "Correct" in the name makes me think of nsFrame::CorrectStyleParentFrame, which isn't related to this. Can we call it something like: "RecreateStyleContexts"? ::: layout/base/ServoRestyleManager.cpp:96 (Diff revision 1) > + if (!(aContent->IsDirtyForServo() || aContent->HasDirtyDescendantsForServo())) > + return; Nit: prefer {}s around single line if statements. ::: layout/base/ServoRestyleManager.cpp:109 (Diff revision 1) > + // TODO: Figure out what pseudos does this content have, and do the proper > + // thing with them. We could probably call ProbePseudoElementStyle for every > + // pseudo, though that's probably overkill. (I think that only works with ::before and ::after, so that wouldn't help find e.g. anonymous table frames.) ::: layout/base/ServoRestyleManager.cpp:119 (Diff revision 1) > + aParentContext, > + nullptr, > + CSSPseudoElementType::NotPseudo); > + > + primaryFrame->SetStyleContext(context.get()); > + Put a TODO comment in here mentioning that we need to compare old and new style to generate change hints, which then need to be processed? ::: layout/base/ServoRestyleManager.cpp:134 (Diff revision 1) > + // FIXME(emilio): Is there any root node of a document that isn't content? > + nsIDocument* doc = PresContext()->Document(); > + CorrectFrameStyleContexts(doc->RootNode()->AsContent(), nullptr, styleSet); nsINode::RootNode returns the root of the subtree the node is in. So calling it on an nsIDocument will return itself. nsIDocument isn't content, so I guess we're lucky that this works. I think you want to use GetRootElement(). (And then null check it.) ::: layout/style/ServoStyleSet.h:36 (Diff revision 1) > class nsPresContext; > struct TreeMatchContext; > > namespace mozilla { > > +class ServoRestyleManager; This can go up in the bunch of other forward declarations.
Attachment #8770398 -
Flags: review?(cam) → review+
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8770398 [details] Bug 1286445: stylo: Support restyles of non-pseudo content on state change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63874/diff/1-2/
Attachment #8770397 -
Attachment description: Bug 1286445: stylo: Propagate the IS_DIRTY flag down the tree. → Bug 1286445: Stub out HasStateDependentStyle
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8770397 [details] Bug 1286445: Stub out HasStateDependentStyle. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63872/diff/1-2/
Assignee | ||
Comment 7•8 years ago
|
||
https://reviewboard.mozilla.org/r/63872/#review60936 > (This might need to be GetParentElementCrossingShadowRoot to break out of shadow trees.) This is still pending, can any node have a parent which is not an element?
Assignee | ||
Comment 8•8 years ago
|
||
https://reviewboard.mozilla.org/r/63874/#review60942 > nsINode::RootNode returns the root of the subtree the node is in. So calling it on an nsIDocument will return itself. nsIDocument isn't content, so I guess we're lucky that this works. I think you want to use GetRootElement(). (And then null check it.) In fact it didn't work, it just *worked* because I was setting the dirty flags on the document, calling UpdateStyleBackend implicitely. This is fixed now, and also fixed other XUL/XBL issues. > This can go up in the bunch of other forward declarations. Thanks, missed them!
Assignee | ||
Comment 9•8 years ago
|
||
I changed some things to fix XBL/XUL, but other than that it's the same idea. I'll probably land this tomorrow, but feel free to take a look for another review if I missed something.
Flags: needinfo?(cam)
Comment 10•8 years ago
|
||
https://reviewboard.mozilla.org/r/63872/#review60936 > This is still pending, can any node have a parent which is not an element? Yes, a node can be a child of a Document, DocumentFragment, ShadowRoot, at least (may have forgotten another).
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/63874/#review61154 r=me with these changes. ::: layout/base/ServoRestyleManager.cpp:20 (Diff revision 2) > : RestyleManagerBase(aPresContext) > { > } > > -void > -ServoRestyleManager::Disconnect() > +/* static */ void > +ServoRestyleManager::DirtyTree(nsIContent* aContent) (Ideally the removal of Disconnect would be done in a separate patch.) ::: layout/base/ServoRestyleManager.cpp:96 (Diff revision 2) > + if (!(aContent->IsDirtyForServo() || aContent->HasDirtyDescendantsForServo())) > + return; Nit: {}s around single statement if body. ::: layout/base/ServoRestyleManager.cpp:124 (Diff revision 2) > + uint32_t childrenCount; > + nsIContent* const* children = aContent->GetChildArray(&childrenCount); > + > + for (uint32_t i = 0; i < childrenCount; ++i) { > + RecreateStyleContexts(children[i], context.get(), aStyleSet); > + } We should use the FlattenedChildIterator here, too. ::: layout/base/nsRefreshDriver.cpp:1749 (Diff revision 2) > > NS_ADDREF(shell); > mStyleFlushObservers.RemoveElement(shell); > RestyleManagerHandle restyleManager = > shell->GetPresContext()->RestyleManager(); > + Don't think this needs to be in this patch. ::: layout/style/ServoStyleSet.h:28 (Diff revision 2) > namespace dom { > class Element; > } // namespace dom > class CSSStyleSheet; > class ServoStyleSheet; > +class ServoRestyleManager; Nit: one line before, to keep the list (almost) sorted. ::: layout/style/ServoStyleSet.h:121 (Diff revision 2) > nsRestyleHint HasStateDependentStyle(dom::Element* aElement, > mozilla::CSSPseudoElementType aPseudoType, > dom::Element* aPseudoElement, > EventStates aStateMask); > > - void RestyleSubtree(nsINode* aNode); > + void RestyleSubtree(nsINode* aNode, bool aForce); Can you add a comment above here that says what aForce does, including that aNode must be an nsIContent if aForce is true? And maybe briefly describes what RestyleSubtree does in general, too. ::: layout/style/ServoStyleSet.cpp:73 (Diff revision 2) > +void > +ServoStyleSet::ForceRestyle(nsPresContext* aPresContext) > +{ > + PRTime t1 = PR_Now(); > + nsIDocument* doc = aPresContext->Document(); > + Element* root = doc->GetRootElement(); > + if (root) { > + RestyleSubtree(root, /* aForce = */ true); > + } > + PRTime t2 = PR_Now(); > + mRestyleTime = t2 - t1; > +} > + This hunk probably remained after rebasing from servo/gecko-dev to mozilla/gecko-dev, so I think it needs to be dropped.
Comment 12•8 years ago
|
||
I guess MozReview doesn't really handle different completely different patches in subsequent pushes very well, it's still carried over an r=me on the second patch even though it is now something else. But, an explicit r=me on that second patch (the stubbing out).
Flags: needinfo?(cam)
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64626/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64626/
Attachment #8770397 -
Attachment description: Bug 1286445: Stub out HasStateDependentStyle → Bug 1286445: Stub out HasStateDependentStyle.
Attachment #8771501 -
Flags: review?(cam)
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8770398 [details] Bug 1286445: stylo: Support restyles of non-pseudo content on state change. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63874/diff/2-3/
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8770397 [details] Bug 1286445: Stub out HasStateDependentStyle. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63872/diff/2-3/
Comment 16•8 years ago
|
||
Comment on attachment 8771501 [details] Bug 1286445: stylo: Unset restyle frames appropriately after regenerating style contexts. https://reviewboard.mozilla.org/r/64626/#review61732 r=me with that. ::: layout/base/ServoRestyleManager.cpp:131 (Diff revision 1) > > // TODO: Compare old and new styles to generate restyle change hints, and > // process them. > primaryFrame->SetStyleContext(context.get()); > > + if (!aContent->HasDirtyDescendantsForServo()) { Rather than checking HasDirtyForDescendantsForServo twice, how about we wrap the first half of this function in an |if (aContent->IsDirtyForServo())| check and the second half in |if (aContent->HasDirtyDescendantsForServo())|?
Attachment #8771501 -
Flags: review?(cam) → review+
Comment 17•8 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba3d1fe1de9 stylo: Support restyles of non-pseudo content on state change. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/9abd6392de0d Stub out HasStateDependentStyle. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/e816e9199940 stylo: Unset restyle frames appropriately after regenerating style contexts. r=heycam
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ba3d1fe1de9 https://hg.mozilla.org/mozilla-central/rev/9abd6392de0d https://hg.mozilla.org/mozilla-central/rev/e816e9199940
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•