1286445 - stylo: Make hover partially work for non pseudo-elements.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1286445: Stub out HasStateDependentStyle.

Review commit: https://reviewboard.mozilla.org/r/63872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63872/

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1286445: stylo: Support restyles of non-pseudo content on state change.

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

[Cameron McCormack (:heycam)]

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.)

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

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/

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8770397 [details]
Bug 1286445: Stub out HasStateDependentStyle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63872/diff/1-2/

Comment 7

[Emilio Cobos Álvarez [:emilio]]

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?

Comment 8

[Emilio Cobos Álvarez [:emilio]]

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!

Comment 9

[Emilio Cobos Álvarez [:emilio]]

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.

Comment 10

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

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).

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1286445: stylo: Unset restyle frames appropriately after regenerating style contexts.

Review commit: https://reviewboard.mozilla.org/r/64626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64626/

Comment 14

[Emilio Cobos Álvarez [:emilio]]

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/

Comment 15

[Emilio Cobos Álvarez [:emilio]]

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

[Cameron McCormack (:heycam)]

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())|?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ba3d1fe1de9
https://hg.mozilla.org/mozilla-central/rev/9abd6392de0d
https://hg.mozilla.org/mozilla-central/rev/e816e9199940