798157 - awesome bar dropdown background color is missing and looks wrong

Status: VERIFIED FIXED

Description

[Timothy Nikkel (:tnikkel)]

I can't provide a screenshot easily because the dropdown disappears when I hit print screen.

Confirmed to be caused by one of the changesets in bug 408284.

Comment 1

[Chris Coulson]

You can probably get a delayed screenshot. If you're using GNOME, you do this by running something like "gnome-screenshot -d 5", then open the dropdown and wait for the delay to expire.

Comment 2

[Timothy Nikkel (:tnikkel)]

Created attachment 668259 [details]
screenshot

Thanks for the tip. Here is a screenshot.

Comment 3

[Timothy Nikkel (:tnikkel)]

The first bad revision is:
changeset:   109106:4aac63aa19dc
user:        Chris Coulson <[e-mail]>
date:        Wed Oct 03 19:53:53 2012 +1300
summary:     b=408284 use ARGB visuals for popup windows when window manager is compositing r=karlt

Comment 4

[Chris Coulson]

I'm not too sure what the background colour is meant to be for your theme, but I think the issue is that there is no shadow. I noticed this earlier too, but it's less obvious in my theme.

Is this using compiz btw? I'd actually just pinged our compiz maintainer to ask if it's possible to make it draw a shadow for argb windows, but I'm still waiting for him to answer.

Comment 5

[Timothy Nikkel (:tnikkel)]

Created attachment 668263 [details]
what it should look like

This is what is should look like.

Yes, I think it is with compiz.

Comment 6

[Chris Coulson]

Thanks. This is on Ubuntu, isn't it? Do you know which version it is?

Comment 7

[Timothy Nikkel (:tnikkel)]

Yes, it is Ubuntu 11.04.

Comment 8

[Karl Tomlinson (back Jan 30 :karlt)]

Sounds similar to bug 781332.
Guessing 4aac63aa19dc is just bringing ARGB formats/visuals into the equation, as would have already been there for shaped panels such as the downloads panel.
(Doesn't explain why the issue wasn't seen on identity or bookmarks panels.)

Comment 9

[Timothy Nikkel (:tnikkel)]

It does seem similar to bug 781332 but it does feel and look different from bug 781332 though.

Comment 10

[Timothy Nikkel (:tnikkel)]

The text looks awful in the awesome bar dropdown. The text looked fine in the downloads panel.

Comment 11

[Chris Coulson]

So I guess there's actually more than one issue here (the lack of shadows for opaque, rectangular windows with an argb visual being one of them) :(

Comment 12

[Timothy Nikkel (:tnikkel)]

I've now seen the same problem on a select drop down on a content web page, which should make this easier to debug.

Comment 13

[Timothy Nikkel (:tnikkel)]

Created attachment 668535 [details]
testcase

Comment 14

[Karl Tomlinson (back Jan 30 :karlt)]

I wonder what theme is used here?
Perhaps that is in ~/.gtkrc-2.0, or /etc/gtk-2.0/gtkrc.

Comment 15

[Timothy Nikkel (:tnikkel)]

In my Appearance Preferences the theme is called Human.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Karl Tomlinson (:karlt) from comment #14)
> I wonder what theme is used here?
> Perhaps that is in ~/.gtkrc-2.0, or /etc/gtk-2.0/gtkrc.

Neither file exists here.

Comment 17

[Timothy Nikkel (:tnikkel)]

From a NSPR_LOG_MODULES=Layers:9 log
-3442880[7f0efea62260]: BasicLayerManager (0x7f0ecd95f200)
-3442880[7f0efea62260]:   BasicContainerLayer (0x7f0ed56b1000) [visible=< (x=0, y=0, w=444, h=275); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=444.000000, h=275.000000) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollId=1 }]
-3442880[7f0efea62260]:     BasicThebesLayer (0x7f0ed737ec00) [visible=< (x=0, y=0, w=444, h=275); >] [opaqueContent] [isFixedPosition]

So we think the content is opaque at least.

Comment 18

[Timothy Nikkel (:tnikkel)]

And mIsTransparent is false and mTransparencyBitmap is null on the window for the problem popup.

Comment 19

[Chris Coulson]

I tried this with the same theme today, and wasn't able to reproduce it :(

Does it happen if you set a background color with an inline style on the PopupAutoCompleteRichResult panel (in browser.xul), rather than using the native styled background? Eg, something like "-moz-appearance:none;background-color:lightgray;"

Comment 20

[Chris Coulson]

For the other issue (missing shadows), I wonder if we could turn off argb visuals for popups that don't need it? It looks like that information is available before we create the widget for popup windows:

http://hg.mozilla.org/mozilla-central/file/acd25563db2f/layout/xul/base/src/nsMenuPopupFrame.cpp#l265

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Coulson from comment #19)
> I tried this with the same theme today, and wasn't able to reproduce it :(

Thanks for trying.

> Does it happen if you set a background color with an inline style on the
> PopupAutoCompleteRichResult panel (in browser.xul), rather than using the
> native styled background? Eg, something like
> "-moz-appearance:none;background-color:lightgray;"

That seemed too hard so I just made nsIFrame::IsThemed always return false and I also added the rule  * {-moz-appearance:none !important;background-color:lightgray !important; } to the testcase here and I was still able to reproduce the problem in the testcase.

Comment 22

[Karl Tomlinson (back Jan 30 :karlt)]

(In reply to Chris Coulson from comment #20)
> For the other issue (missing shadows), I wonder if we could turn off argb
> visuals for popups that don't need it?

If mPopupHint is useful, it could be used from nsWindow::Create.
If not, then adding another field to nsWidgetInitData sounds appropriate.

Comment 23

[Chris Coulson]

(In reply to Karl Tomlinson (:karlt) from comment #22)
> (In reply to Chris Coulson from comment #20)
> > For the other issue (missing shadows), I wonder if we could turn off argb
> > visuals for popups that don't need it?
> 
> If mPopupHint is useful, it could be used from nsWindow::Create.
> If not, then adding another field to nsWidgetInitData sounds appropriate.

Hi,

The problem with mPopupHint is that the awesomebar dropdown is implemented as a panel. I did try a quick test last night using mDropShadow (which gets set on popups without transparent content). That seemed to work ok - the only exceptions being that popups for HTML select elements still get an argb visual when it's not needed, and tooltips no longer get one. Although, tooltips don't really need it yet anyway.

Comment 24

[Chris Coulson]

(In reply to Timothy Nikkel (:tn) from comment #21)
> (In reply to Chris Coulson from comment #19)
> > I tried this with the same theme today, and wasn't able to reproduce it :(
> 
> Thanks for trying.
> 
> > Does it happen if you set a background color with an inline style on the
> > PopupAutoCompleteRichResult panel (in browser.xul), rather than using the
> > native styled background? Eg, something like
> > "-moz-appearance:none;background-color:lightgray;"
> 
> That seemed too hard so I just made nsIFrame::IsThemed always return false
> and I also added the rule  * {-moz-appearance:none
> !important;background-color:lightgray !important; } to the testcase here and
> I was still able to reproduce the problem in the testcase.

I don't know if that changes the background style of the popup. I wanted to see if the problem went away with gfxXlibNativeRenderer eliminated from the render path

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Coulson from comment #24)
> > That seemed too hard so I just made nsIFrame::IsThemed always return false
> > and I also added the rule  * {-moz-appearance:none
> > !important;background-color:lightgray !important; } to the testcase here and
> > I was still able to reproduce the problem in the testcase.
> 
> I don't know if that changes the background style of the popup. I wanted to
> see if the problem went away with gfxXlibNativeRenderer eliminated from the
> render path

I think it should. nsIFrame::IsThemed is what we use to decide if we should use native theme drawing based on a -moz-appearance style rule.

Comment 26

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #25)
> > I don't know if that changes the background style of the popup. I wanted to
> > see if the problem went away with gfxXlibNativeRenderer eliminated from the
> > render path
> 
> I think it should. nsIFrame::IsThemed is what we use to decide if we should
> use native theme drawing based on a -moz-appearance style rule.

There were some cases that bypassed IsThemed and just used ThemeSupportsWidget. After I fixed that there were no calls to any NativeRenderer Draw* function and the problem still happens.

Comment 27

[Karl Tomlinson (back Jan 30 :karlt)]

(In reply to Chris Coulson from comment #23)
> I did try a quick test last night using mDropShadow (which gets
> set on popups without transparent content). That seemed to work ok - the
> only exceptions being that popups for HTML select elements still get an argb
> visual when it's not needed, and tooltips no longer get one. Although,
> tooltips don't really need it yet anyway.

mDropShadow sounds a pretty close match for what we want, because we are choosing the XRGB visual to get a drop shadow.

I didn't expect select elements to have drop shadows because I think of them as extending from the button, and the compositor doesn't know whether these extend up or down from the button.  (Perhaps others might imagine or different themes present these differently, though.)

I wonder why tooltips no longer get one.
Is that because mIsContentShell is set or because nsLayoutUtils::GetFrameTransparency doesn't think the frame is styled with transparent background?
I wonder why the mIsContentShell test is even there.  Perhaps GetFrameTransparency cannot be relied on to make content-controlled backgrounds opaque.  (I don't know which popups can be styled by content.)

Comment 28

[Chris Coulson]

Tooltips currently don't get one because of http://hg.mozilla.org/mozilla-central/file/05e1bb4d7cd8/widget/gtk2/nsNativeThemeGTK.cpp#l1428.

I'm not sure what to do with the background colour issue at this point. I wish I could reproduce it here. I assume this is happening with the same setup as in bug 781332 (ie, with the fglrx driver)?

Comment 29

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Coulson from comment #28)
> I'm not sure what to do with the background colour issue at this point. I
> wish I could reproduce it here. I assume this is happening with the same
> setup as in bug 781332 (ie, with the fglrx driver)?

Yes, that is correct, same machine.

Comment 30

[Timothy Nikkel (:tnikkel)]

Created attachment 670217 [details]
gfx info dump

Comment 31

[Karl Tomlinson (back Jan 30 :karlt)]

I looked into this with tn.  When _cairo_xlib_surface_composite is copying a depth-24 opaque pixmap to the depth-32 window, the pixmap looks as expected.
The bug occurs with CAIRO_OPERATOR_SOURCE or CAIRO_OPERATOR_OVER.  There is no mask.

This looks like a bug in the RENDER driver implementation not filling in the alpha components of the destination surface.  The bug did not occur with GL layers.

Comment 32

[Karl Tomlinson (back Jan 30 :karlt)]

The bug occurs with compiz or metacity (with compositing).

Comment 33

[Karl Tomlinson (back Jan 30 :karlt)]

Created attachment 681297 [details] [diff] [review]
use ARGB visuals only for popups that will be translucent

Using an XRGB visual where possible should be more efficient and allows the
window compositor to know where to draw a drop shadow.

This should also work around the fglrx bug in at least most cases.
This bug at least isn't common on dropdowns that we expect to be transparent,
probably because we use an ARGB Pixmap and so don't usually hit the same
driver bug.  Perhaps there may still be a bug when copying from an XRGB Pixmap
to an ARGB Pixmap (bug 781332) but that shows much less frequently.

Reusing mDropShadow for this wouldn't workaround the fglrx bug for menulist
popups.

I removed the comment about the composited-changed signal because it is not
really correct: even windows that are only shown for a short time tend to be
kept after hidden and reshown at a much later time.  If the window manager is
no longer compositing at a later time, shapes will be added in OnExposeEvent.

Comment 34

[Karl Tomlinson (back Jan 30 :karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f605e71a350

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f605e71a350

Comment 37

[Karl Tomlinson (back Jan 30 :karlt)]

Comment on attachment 681297 [details] [diff] [review]
use ARGB visuals only for popups that will be translucent

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 408284
User impact if declined: missing shadows on menus/dropdowns for Ubuntu users + translucent menus for those using fglrx driver
Testing completed (on m-c, etc.): on m-c 
Risk to taking this patch (and alternatives if risky): low - this reduces the number of situations where the changes in bug 408284 take effect
String or UUID changes made by this patch: none.

Comment 38

[bhavana bajaj [:bajaj]]

Comment on attachment 681297 [details] [diff] [review]
use ARGB visuals only for popups that will be translucent

low risk patch fixing 807176 tracking 18. Approving on aurora.

Comment 39

[bhavana bajaj [:bajaj]]

Adding QA to help verify issue in this bug is fixed and as well and a quick verification on Bug 807176 being fixed by the patch uplifted here.

Comment 40

[Karl Tomlinson (back Jan 30 :karlt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0fa256f3c12f

Comment 42

[Virgil Dicu [:virgil] [QA]]

Couldn't reproduce the issue from comment 0 with fglrx.

Could anyone who could reproduce set this to verified?

Comment 43

[Timothy Nikkel (:tnikkel)]

This is fixed for me. Yay.