stylo: Google Inbox messages overlap each other after marking a message as done

REOPENED
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
REOPENED
17 days ago
3 days ago

People

(Reporter: mossop, Assigned: hiro)

Tracking

(Blocks: 1 bug)

Trunk
Unspecified
Windows 10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Reporter)

Description

17 days ago
Created attachment 8884325 [details]
Capture.JPG

I'm using Google Inbox with stylo enabled on Windows 10. With a long list of messages in my inbox if I open one near the bottom then mark it as done it closes and then a bunch of the messages at the bottom start to overlap each other.

Updated

17 days ago
Has STR: --- → yes
OS: Unspecified → Windows 10
Version: unspecified → Trunk
I can repro, and I see that flushing styles with the following bookmark:

javascript:(function() { var s = document.createElement('style'); s.innerHTML = '* { color: red }'; document.body.appendChild(s); document.body.offsetTop; s.parentNode.removeChild(s); }())

Makes the issue disappear. However, the same doesn't happen with just a recascade of the styles (resizing the window, for example).

So we end up with some element having invalid styles... Still no clue why off-hand. Maybe it's due to something going wrong with transitions/animations, or maybe we really miss a restyle and fail to invalidate a given element...
See Also: → bug 1379425
Created attachment 8884598 [details]
Restyle notification log

Hmm... So just disabling OMTA doesn't fix this, so there's something else going on...

Unfortunately there's a whole bunch of stuff going on, as the log shows, so it'll take me a bit to diagnose, I suppose.

In particular, they add and remove <script>'s, and <style> elements during that interaction, which is silly.

On a side note, that explains why FF, not only stylo, is slower than Chrome on Inbox, since we restyle the whole document when removing a <style> element... There's a pretty easy fix on stylo for that though, since we have the machinery I added in bug 1357583, and we just have to reuse it for removals, which is easy.

Anyway, will dig more...
Created attachment 8884602 [details] [diff] [review]
Patch to disable transitions and OMTA (which "fixes" the issue).

However, disabling transitions, I can no longer repro the issue.

Here's the patch I used (on top of current autoland).

Hiro, any chance you can take a look into this? Do you think it could be due to the issues re. snapshots + animation-only restyle outlined at bug 1379425 and related?
Flags: needinfo?(hikezoe)
See Also: → bug 1379433
(Assignee)

Comment 4

15 days ago
Yes, very likely. I also believe this happens on events (mouse movement, click, etc.), I did confirm it with dropping UpdateOnlyAnimationStyles() call in FlushThrottledStyles() using attachment 8884588 [details] in bug 1379425.  (Thanks for the great test cases)
Note that, as I commented in other bug, UpdateOnlyAnimationStyles() is slightly misleading, 
 
Honestly I don't still understand how snapshots interact with animation-only restyles, but yes, I think we don't need to clear snapshots for animation-only restyles. One thing I am concerned (don't quite understand) is SMIL. Class attribute SMIL seems to need snapshot [1].  Anyway I am going to care these snapshot and animation issues.  

[1] https://hg.mozilla.org/mozilla-central/file/a418121d4625/layout/base/ServoRestyleManager.cpp#l1012
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
(Assignee)

Comment 5

15 days ago
A solution I can think of is that we don't return the default computed values from Servo_ResolveStyle() when it's called from UpdateOnlyAnimationStyles (I will rename it to UpdateThrottledAnimationStyles in bug 1379534 though) even if the target element has snapshot.  This is what we talked about bug 1371450 in the last all hands, IIRC.
(Assignee)

Comment 6

14 days ago
I start thinking we might need animation-only *post* traversal. that's because when we kick animation-only restyle for event handling, we don't need to update any style contexts for normal restyle at all, we just need style contexts for animating elements.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> I start thinking we might need animation-only *post* traversal. that's
> because when we kick animation-only restyle for event handling, we don't
> need to update any style contexts for normal restyle at all, we just need
> style contexts for animating elements.

That makes sense, off-hand. Following both dirty bits on the post traversal is one of the things that doesn't make sense to me :)
Priority: -- → P1

Updated

12 days ago
See Also: → bug 1380289

Updated

12 days ago
Duplicate of this bug: 1380289
(Assignee)

Comment 9

6 days ago
As far as I can tell this has been fixed by bug 1371450. Please re-open if not fixed yet.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 days ago
Resolution: --- → FIXED
(Reporter)

Comment 10

3 days ago
Created attachment 8888836 [details]
Capture.PNG

I'm still seeing strange effects with stylo enabled on Inbox
(Reporter)

Updated

3 days ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

3 days ago
I can't reproduce at all.  Can someone provide a reduced test case?
You need to log in before you can comment on or make changes to this bug.