1688899 - Leftover lines under menu items after hovering in the new AppMenu.

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

Attachment: Screen Shot 2021-01-26 at 10.27.26.png

As shown in the screenshot…

Comment 1

[Mike Conley (:mconley) (:⚙️)]

This looks like an invalidation bug to me. Hey jrmuizel, do you know what the best way of approaching this problem would be?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

What platform is this on? Is this a regression?

Comment 3

[Blake Winton (:bwinton) (:☕️)]

MacOS 11.1 Big Sur, 2018 MacBook Pro.

I don't know whether it's a regression or not, because the highlights are new. (Added in https://bugzilla.mozilla.org/show_bug.cgi?id=1688744 I think. Mike would know better.)

Mossop (cc-ed) also saw it, perhaps on a different platform.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Yeah, I'd say bug 1688744 is where we added the CSS where we first started noticing this. Frustratingly, I can't seem to reproduce this reliably on a new profile.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

I'm also noticing it on my macOS 10.15.7 Catalina MBP. I think I've only seen it when the window is on the Retina display, but I can't say for certain.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Is dark mode needed to reproduce the problem?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I have also only noticed this with dark mode, so... potentially? But we're not sure.

To get the CSS applied from the bug I referred to, create and set: browser.proton.appmenu.enabled to true, then restart. Note that just a few days ago, "Library" was removed from that menu, but both "Bookmarks" and "History" are there, and I've been able to see the lines within those submenus as well.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

I just reproduced it on the Light theme with 87.0a1 (2021-01-28) (64-bit), so I don't think it's dark-mode only.

Comment 9

[Dave Townsend [:mossop]]

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

Is dark mode needed to reproduce the problem?

I do not run with dark mode.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

I can't reproduce it on 10.15.6

Comment 11

[Markus Stange [:mstange]]

I can reproduce this. Needinfo'ing myself to debug it.

Comment 13

[Markus Stange [:mstange]]

This happens when the popup opens at a fractional position in terms of "logical coordinates". I can only get it to reproduce when I have the Proton tab bar enabled, which causes the toolbar to be located at a different fractional position than before.

We try to open the popup widget at a Y position of, say, 120.5. For window positions and sizes, macOS only allows integer "logical" coordinates. So this code makes us round the widget position to an integer (121) and we end up with a mViewToWidgetOffset of (0, 0.5).

From now on we have a confusion between these three spaces: LayerManager space, view space, and widget space:

• Invalidation thinks LayerManager space == view space.
• Some places during painting think LayerManager space == widget DrawTarget space == widget space.

So the invalidated rect is shifted by 0.5 logical pixels (== 1 device pixel) compared to the painted rect. The shifting happens between the InvalidateViewNoSuppression call and the nsChildView::Invalidate call, in InvalidateWidgetArea.

Questions:

• Do we want to have a non-zero mViewToWidgetOffset in this case, or should we snap the view position together with the widget position? Would that have other bad effects?
• Should LayerManager space match widget space or view space?

In an ideal world, nsViews would not exist and there would be no "view to widget offset".

Comment 14

[Markus Stange [:mstange]]

Attachment: Bug 1688899 - Avoid painting artifacts in fractionally-positioned menupopups by treating the invalid region as being relative to the widget. r=tnikkel

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Markus Stange [:mstange] from comment #13)

• Do we want to have a non-zero mViewToWidgetOffset in this case, or should we snap the view position together with the widget position? Would that have other bad effects?
• Should LayerManager space match widget space or view space?

Without paging all of this back into my head my initial answers would be mViewToWidgetOffset should be non-zero in this case and having the view dims different from the frame dims is not what we want.

We do snapping in layer manager space, so it has to be widget space, no?

In an ideal world, nsViews would not exist and there would be no "view to widget offset".

I think we might need a 'frame to widget offset' or something similar to solve the same problem as the view to widget offset, although I'm not 100% sure.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecdc9c2a8a20
Avoid painting artifacts in fractionally-positioned menupopups by treating the invalid region as being relative to the widget. r=tnikkel

Comment 17

[Sandor Molnar[:smolnar]]

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

Comment 19

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I was able to reproduce the issue on Firefox 87.0a1 (2021-01-28) under macOS 11.2.3.

The issue is no longer reproducible on Firefox 88.0a1 (2021-03-21). Tests were performed on macOS 11.2.3, Ubuntu 20.04, and Windows 10.