1611710 - Checkmark icons disappearing in the One-Click Search Engines section

Status: RESOLVED FIXED

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

Description

[Itiel]

STR:

1. Open about:preferences#search
2. Ctrl+Shift+C
3. Scroll down to the One-Click Search Engines section -- all the search engines except for one (presumably the default one?) have no checkmark next to them
4. Hover the search engines tree -- checkmarks are back
5. Scroll back to the top and then down -- same results as in steps 3 & 4

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fd2d5ef280f4501b8fe79c66f9011d6a73a10115&tochange=a65aef85ad0df16d756283fb1975cdd90ea61843
Suspect: bug 1588142

Comment 1

[:Gijs (out for now; he/him)]

(In reply to Itiel from comment #0)

STR:

1. Open about:preferences#search
2. Ctrl+Shift+C

What does this do for you? On mac, it shows the inspector with the "inspect" tool selected for me.

3. Scroll down to the One-Click Search Engines section -- all the search engines except for one (presumably the default one?) have no checkmark next to them

Can't reproduce this on macOS. :-(

4. Hover the search engines tree -- checkmarks are back
5. Scroll back to the top and then down -- same results as in steps 3 & 4

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fd2d5ef280f4501b8fe79c66f9011d6a73a10115&tochange=a65aef85ad0df16d756283fb1975cdd90ea61843
Suspect: bug 1588142

Can you reproduce in safe mode?

Comment 2

[Itiel]

(In reply to :Gijs (he/him) from comment #1)

What does this do for you? On mac, it shows the inspector with the "inspect" tool selected for me.

Yep, that's what I meant.

Can't reproduce this on macOS. :-(

I'm on Windows 10 btw.

Can you reproduce in safe mode?

This was tested on a clean profile, using mozregression.
I can still test in safe mode if you'd like.

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Itiel from comment #2)

(In reply to :Gijs (he/him) from comment #1)

Can you reproduce in safe mode?

This was tested on a clean profile, using mozregression.
I can still test in safe mode if you'd like.

Yes please; I somewhat suspect a graphics issue, in which case I'd expect safe mode to resolve the problem, so trying to confirm that. :-)

Comment 4

[Itiel]

(In reply to :Gijs (he/him) from comment #3)

Yes please; I somewhat suspect a graphics issue, in which case I'd expect safe mode to resolve the problem, so trying to confirm that. :-)

Yep, that's probably the case. This works okay in safe mode.

Comment 5

[Itiel]

Attachment: about:support contents

Comment 6

[:Gijs (out for now; he/him)]

Punting this to graphics based on comment #4...

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment 9123189 [details] says that WebRender was used. And I could reproduce the problem on Linux and on Windows. And when I disabled WebRender, I did not see the problem.

Comment 8

[Sotaro Ikeda [:sotaro]]

We could not know the regression of WebRender because of comment 0 :(

Comment 9

[Alexandra Martin]

Managed to reproduce the issue on Nightly 74.0a1 (2020-02-03) using Windows 10 x64 and Ubuntu 18.04.3 LTS. On Release 72.0.2 and Beta 73.0 the issue couldn't be reproduced. Will add the according flags.

Comment 10

[Sotaro Ikeda [:sotaro]]

:gw, can you comment to this bug? Is it caused by WebRender side or gecko side?

Comment 11

[Itiel]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)

We could not know the regression of WebRender because of comment 0 :(

I'd be happy to check for the regression range if a reduced test case can be made.

Comment 12

[Glenn Watson [:gw]]

I can't seem to repro this on Linux with current m-c. Sotaro, is it still occurring for you? Could you attach a screenshot, maybe I'm looking at the wrong thing?

Comment 13

[Itiel]

I can also reproduce this without having to open the inspector, by zooming in the about:preferences page:
https://gfycat.com/darkearlyfireant

Comment 14

[Itiel]

Screencast with opening the inspector:
https://gfycat.com/mediumblindgermanspitz

Comment 15

[Itiel]

(In reply to Itiel from comment #13)

I can also reproduce this without having to open the inspector, by zooming in the about:preferences page:
https://gfycat.com/darkearlyfireant

For whatever reason I could reproduce this only when the zoom-in is >200%.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: video when problem happened

Comment 17

[Sotaro Ikeda [:sotaro]]

I confirmed that the problem happened with latest nightly and latest m-c on Ubuntu. Attachment 9124626 [details] is a video when problem happened.
In the STR in comment 0, if we did not follow "2. Ctrl+Shift+C", the problem did not happen. Then it might be related to WR bug.

Comment 18

[Glenn Watson [:gw]]

OK, I am able to reproduce now. I'll try a mozregression to see if I can identify when it broke.

Comment 19

[Glenn Watson [:gw]]

Attachment: bug.png

I was able to get a WR capture when the bug occurs.

By tweaking the scene.ron file, I can confirm that the portion of the output in the red dashed box is a single image that has been sent to WR by Gecko.

From that, I assume that this is a blob image and that there is a bug in the blob handling code.

The number of checkmarks that get drawn seems somewhat dependent on size of the viewport and/or inspector panel. So I suspect some kind of culling / visibility issue.

Jeff, Nical, does that sound right? Could you take a look at this?

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Ok, yeah the "One-Click Search Engines" table use a xul tree element which will cause fallback to blob rendering, given the complexity of what can go in a xul tree it's not that surprising that it would break.

Gijs, how hard would it be to replace the xul tree with a non-xul widget?

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #20)

Ok, yeah the "One-Click Search Engines" table use a xul tree element which will cause fallback to blob rendering, given the complexity of what can go in a xul tree it's not that surprising that it would break.

Gijs, how hard would it be to replace the xul tree with a non-xul widget?

I don't know the exact details of this UI, but from a quick look converting this tree is doable but not easy. The good news is that we aren't dealing with an infinite number of rows so performance isn't a concern, and we aren't extending places-tree or reusing this same tree in other places in the chrome. But the things that look like they would take a fair amount of time here are:

• the UI logic is coupled with the tree API which is quite different from how one usually interacts with the DOM (https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/browser/components/preferences/in-content/search.js#48)
• the 'keyword' field on the right with disappearing textboxes is implemented within the tree custom element (https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/toolkit/content/widgets/tree.js#1302) along with extra handling in prefs (like https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/browser/components/preferences/in-content/search.js#339-345).
• there are shared styles with #blocklistsTree (like https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/browser/themes/shared/incontentprefs/preferences.inc.css#420) - so we'd want to check if that should be converted at the same time.

So if we wanted to port this and maintain the exact same functionality it would take some time. Certainly not anything that could make it into 74.

More generally, is this something that could happen to any tree and WR or is it somehow constrained with a particular feature this is using? I'm wondering if we need to kick off a project to completely remove XUL trees (things like the Places tree will be a lot more work) or to better support them with WR. The current plan is to not focus on general removal for them beyond the XBL->Custom Element because of the cost to do so, unless if it becomes necessary for other reasons (see https://bugzilla.mozilla.org/show_bug.cgi?id=1446341#c27). If you think this may be necessary, let's have a chat to discuss the options.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

(In reply to Brian Grinstead [:bgrins] from comment #21)

More generally, is this something that could happen to any tree and WR or is it somehow constrained with a particular feature this is using? I'm wondering if we need to kick off a project to completely remove XUL trees (things like the Places tree will be a lot more work) or to better support them with WR. The current plan is to not focus on general removal for them beyond the XBL->Custom Element because of the cost to do so, unless if it becomes necessary for other reasons (see https://bugzilla.mozilla.org/show_bug.cgi?id=1446341#c27). If you think this may be necessary, let's have a chat to discuss the options.

I was hoping it would be relatively easy so that we could just avoid investigating what's going wrong. It sounds like it's hard enough that it's probably still worth taking a look at what's going wrong.

Regardless of this particular instance it's still true that XUL trees will have bad performance with WebRender (any changes of content will cause a full repaint) so it's still good to move away from them in places where performance maters.

Comment 23

[:Gijs (out for now; he/him)]

I think Brian's answered my ni here. :-)

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 25

[Jessie [:jbonisteel] pls NI]

Jeff - per our chat, can you update here what next steps/a potential fix might be?

We will try and get to this for 74, but not sure we will be able to.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

I looked at this some more. We're calling RenderDocument on each svg icon but not calling Fill() for the ones that don't show up. Something must be going wrong inbetween.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

Attachment: A possible fix

Comment 28

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1611710. Don't restrict the draw target to the visible area.

Sometimes the painting code will look at the clip which is derived
from the intial size of the surface and not draw if things if they
are outside of it. We want to draw the entire item so use dtRect
instead of visibleRect.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd13b92ec265
Don't restrict the draw target to the visible area.

Comment 30

[Pascal Chevrel:pascalc]

Jeff, is that something you want to uplift to 74? (last beta being built in a few hours)

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9129018 [details]
Bug 1611710. Don't restrict the draw target to the visible area.

Beta/Release Uplift Approval Request

• User impact if declined: - Checkmark icons disappearing in the One-Click Search Engines section

• Blobs are clipped incorrectly (bug 1617426)
• Text does not appear until mouseover when using WebRender (bug 1617627)

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The code should be pretty well covered by tests. This adds a new tests which passes and it fixes a number of different bugs. That being said it hasn't had much time to bake on Nightly. It is, however, a one line change so easy to revert if needed.
• String changes made/needed:

Comment 32

[Pascal Chevrel:pascalc]

Comment on attachment 9129018 [details]
Bug 1611710. Don't restrict the draw target to the visible area.

Taking in our last 74 beta as it fixes several bugs caused by webrender and the patch is small and would be backed out easily in RC if we encounter a problem.

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Can you review the patch, please? It already landed.

Comment 34

[Markus Stange [:mstange]]

Done.

Comment 35

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/91e7614fd190

Comment 36

[Arthur Iakab [arthur_iakab]]

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