1435143 - regression: Nextcloud menu tooltip backgrounds are broken

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Darkspirit]

Attachment: testcase.html

Comment 1

[Darkspirit]

Attachment: Screenshot_20180202_042123.png

mozregression --good 2018-01-31 --bad 2018-02-01 --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8947704"
> 4:56.61 INFO: Last good revision: c9c4506e63009d9a5f4da3ef6d5e88fa529b4f75
> 4:56.61 INFO: First bad revision: 3940b5d8f13529b17acfba25fa8e1d1d179628d0
> 4:56.62 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c9c4506e63009d9a5f4da3ef6d5e88fa529b4f75&tochange=3940b5d8f13529b17acfba25fa8e1d1d179628d0

> 3940b5d8f135	Martin Roinson — Bug 1434288 - Update gecko for API changes in WR PR 2315. r=kats
> e99f6587c86e	Kartikaya Gupta — Bug 1434288 - Update webrender to commit e772c3cb8ea0a35e6477e9dc8dd2144e2de87b56. r=jrmuizel

-----

> WR @ b6e69a8efbcd8dc3e0c0a8a9925e6a9355635de3
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ea0e50f426552229ab5746ed3f5ab1c9ebf0561
mozregression --repo try --launch 9ea0e50f426552229ab5746ed3f5ab1c9ebf0561 --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8947704"
-> good

> WR @ 87c6a6ba7e75d8ed0b2ddeeb5fa4e363482f7f86
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7801945b00e7eac75e8bdf9d64869a9c284599de
mozregression --repo try --launch 7801945b00e7eac75e8bdf9d64869a9c284599de --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8947704"
-> goood

> WR @ e772c3cb8ea0a35e6477e9dc8dd2144e2de87b56, with the fixes
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=62ab4844535a8ae2194ee6b7e43c1f9f05bb1511
mozregression --repo try --launch 62ab4844535a8ae2194ee6b7e43c1f9f05bb1511 --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8947704"
-> bad

https://github.com/servo/webrender/compare/87c6a6ba7e75d8ed0b2ddeeb5fa4e363482f7f86...e772c3cb8ea0a35e6477e9dc8dd2144e2de87b56

Comment 2

[Glenn Watson [:gw]]

I reduced this down to the following test case, via a WR recording. I then rolled local wrench/WR back past the reported regression range above, and it still didn't look correct.

Looking closer at the test case below, the green rect (color 0, 128, 0) has a clip id of 5, which is the clip-id for a bullet point (a very small radius), which seems like the display list is wrong.

Looking at the regression range above, and since rolling back WR doesn't affect the output, I think this is most likely to be a gecko-side bug related to the external scroll ID changes.

Does that sound right? If so, who is best to re-assign this to?

---
root: 
    items: 
      - 
        type: "stacking-context"
        items: 
          - 
            bounds: [0, 0, 1920, 1450]
            "clip-rect": [0, 0, 1920, 1450]
            "clip-and-scroll": 0
            "backface-visible": true
            type: clip
            id: 1
            "content-size": [1920, 1450]
          - 
            "clip-rect": [0, 0, 1920, 1450]
            "clip-and-scroll": 1
            "backface-visible": true
            type: "scroll-frame"
            id: 2
            "content-size": [1920, 1450]
            bounds: [0, 0, 1920, 1450]
          - 
            bounds: [0, 0, 1920, 1450]
            "clip-rect": [0, 0, 1920, 1450]
            "clip-and-scroll": 2
            "backface-visible": true
            type: clip
            id: 3
            "content-size": [1920, 1450]
          - 
            bounds: [12, 12, 60, 77]
            "clip-rect": [12, 12, 60, 77]
            "clip-and-scroll": [0, 1]
            "backface-visible": true
            type: rect
            color: 250.00002 164 1 1.0000
          - 
            bounds: [51, 47, 9, 9]
            "clip-rect": [51, 47, 9, 9]
            "clip-and-scroll": 0
            "backface-visible": true
            type: clip
            id: 4
            "content-size": [9, 9]
            complex: 
              - 
                rect: [50.8, 46.925, 9.2, 9.2]
                radius: [4.5, 4.5]
                "clip-mode": clip
          - 
            bounds: [51, 47, 9, 9]
            "clip-rect": [51, 47, 9, 9]
            "clip-and-scroll": 4
            "backface-visible": true
            type: rect
            color: black
          - 
            bounds: [72, 36, 0, 0]
            "clip-rect": [72, 36, 0, 0]
            "clip-and-scroll": [0, 1]
            "backface-visible": true
            type: "stacking-context"
            "scroll-policy": scrollable
            transform: [1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, -25, 0, 0, 1]
            "transform-style": flat
            filters: []
            items: 
              - 
                bounds: [0, 0, 50, 29]
                "clip-rect": [0, 0, 50, 29]
                "clip-and-scroll": 1
                "backface-visible": true
                type: clip
                id: 5
                "content-size": [50, 29]
                complex: 
                  - 
                    rect: [0, 0, 50, 29]
                    radius: [1.5, 1.5]
                    "clip-mode": clip
              - 
                bounds: [0, 0, 50, 29]
                "clip-rect": [0, 0, 50, 29]
                "clip-and-scroll": [0, 5]
                "backface-visible": true
                type: rect
                color: 0 128 0 1.0000
              - 
                bounds: [-1, -1, 52, 31]
                "clip-rect": [-2, -2, 54, 33]
                "clip-and-scroll": [0, 1]
                "backface-visible": true
                glyphs: [55, 72, 86, 87]
                offsets: [0, 23, 14, 23, 28, 23, 40, 23]
                size: 18
                color: black
                font: /usr/share/fonts/truetype/dejavu/DejaVuSerif.ttf

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(Moving comment over from the github issue to keep discussion in one place)

It was my understanding that the external scroll ID change would be a no-op from the gecko point of view (i.e. it was just shifting some code around, no functional changes intended). @mrobinson can you confirm? If so then it shouldn't have exposed any latent gecko bugs assuming that's what happened.

Comment 4

[Martin Robinson (email:[e-mail])]

Yes, my change should not have changed the way that scroll ids were assigned in Gecko at all. Nevertheless, it is possible that it introduced a subtle change. I'm currently trying to track down what is going with the display lists generated here.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Thanks, reassigning to you for now.

Comment 6

[Martin Robinson (email:[e-mail])]

Attachment: Properly unwrap Maybe<WrScrollId> for the root scroll frame

When comparing a Maybe<WrScrollId> to another WrScrollId we need to properly
handle the case where Nothing() signifies the root scroll frame. This is
equivalent to calling scrollId.valueOr(FrameMetrics::NULL_SCROLL_ID), as was
done before WrScrolLId replaced ViewId in the WebRender ScrollingLayersHelper.
We also have DisplayListBuilder::TopmostScrollId always return a value instead
of a Maybe, since an empty clip stack means that the current scroll id is that
of the root scroll frame.

Previously Nothing() was not equivalent to WrScrollId { 0 }, which caused the
ScrollingLayersHelper to fill the mClipAndScroll value and push another
set of clip and scroll nodes onto the WebRender display list builder.

Comment 7

[Martin Robinson (email:[e-mail])]

The try job for this change is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7745f7eb6cfa1dbdca73b77e96625e0e1d013e3

Comment 8

[Darkspirit]

Debian Testing (KDE, Radeon RX480)

mozregression --repo try --launch b7745f7eb6cfa1dbdca73b77e96625e0e1d013e3 --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8947704|https://www.youtube.com/user/failarmy/videos|https://www.coursera.org/?authMode=login|https://bugzilla.mozilla.org/attachment.cgi?id=8949097"
* about:support webrender is active
* bug 1435143 Nextcloud: good
* bug 1435415 failarmy: good (checked with all themes + quantum lights dynamic)
* bug 1435675 login widget: good (Other bug, not new: The website had many colorful dots for a short moment when I switched to that tab.)
* bug 1435129 tab loading: good
* bug 1436420 activity stream: good

mozregression --launch 2018-02-07 --pref gfx.webrender.all:true startup.homepage_welcome_url:"https://bugzilla.mozilla.org/attachment.cgi?id=8947704|https://www.youtube.com/user/failarmy/videos|https://www.coursera.org/?authMode=login|https://bugzilla.mozilla.org/attachment.cgi?id=8949097"
* about:support webrender is active
* bug 1435143 Nextcloud: bad
* bug 1435415 failarmy: bad
* bug 1435675 login widget: bad 
* bug 1435129 tab loading: bad
* bug 1436420 activity stream: bad

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8949075 [details] [diff] [review]
Properly unwrap Maybe<WrScrollId> for the root scroll frame

Review of attachment 8949075 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3dbee5cf682
Properly unwrap Maybe<WrScrollId> for the root scroll frame. r=kats

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d3dbee5cf682

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

This being fixed with a PR - is there a Gecko side reftest that we can add that catches this problem?

Comment 18

[Darkspirit]

comment 15 includes a reftest?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Yeah it does. So this bug is good :)