1365510 - stylo: Github's sticky header shadow is showing/hiding slowly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Shing Lyu [:shinglyu]]

Attachment: Screen recording video

Please check the screen recording video.

STR:
* Open a github PR file diff [0].
* Scroll all the way down so the sticky header appears
* The scroll back up so the sticky header is no longer sticky

Expected:

* The drop shadow for the sticky header should show up and disappear immediately

Actual:

* After the header become sticky, the shadow shows up after ~1 sec delay (opt build, even slower in debug build)
* After the header is no longer sticky, the shadow remains for ~1 sec before disappearing.

Notes:
The shadow is a <div class="pr-toolbar-shadow">. When the header becomes sticky, an "is-stuck" class is added to it's previous sibling, and the following CSS selector will match the shadow and set it as visible: ".pr-toolbar.is-stuck+.pr-toolbar-shadow".

[0] https://github.com/servo/servo/pull/16900/files

Comment 1

[Shing Lyu [:shinglyu]]

Attachment: Shadow appearing screenshot

Comment 2

[Shing Lyu [:shinglyu]]

Attachment: Shadow disappearing delayed screenshot

Comment 3

[Bobby Holley (:bholley)]

This sounds like an animation perf issue. Brian, are we tracking these yet? Should this be P1?

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

There's no animation on that page as far as I can see (and comment 0 seems to agree) so I don't think it's an animation perf issue.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

This is likely due to us invalidating a bunch of extra stuff due to the + combinator.

I looked at how other browsers did the restyle hint stuff, and they're much more precise than our currently existing machinery. In particular, instead of restyle hints that say: "Let's restyle the whole subtree", they actually go into the subtree and match the relevant selectors for all the children.

This is similar to the SomeDescendants optimization that Gecko has, but nicely split out so it doesn't pollute the styling code (it's done before-hand, in the equivalent to RestyleManager::AttributeChanged).

Comment 6

[Cameron McCormack (:heycam)]

Is this really just due to time spent restyling?


In bug 1366194 I was hoping to (or I was hoping to get Tommy to) do something like the current RestyleDepth tracking for later siblings.  That at least would let us avoid expanding out LaterSiblings to RestyleSubtree, if we only had zero (or more) descendant combinators after the later sibling combinator.

However we end up solving the SomeDescendants optimization -- like Gecko, with right-most selectors, or Emilio's suggestion of recording and propagating dependencies -- we should be able to generalize it to work with later siblings' descendants as well as with the current element's descendants.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

This seems to be fixed by bug 1368240, Shing, could you confirm it?

Comment 8

[Cameron McCormack (:heycam)]

I still see this.

Comment 9

[Cameron McCormack (:heycam)]

That was on a debug build.  On an opt build, it looks normal.  Let's say this is fixed then.