511323 - Painting weirdness involving a <select> with a transparent background

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcase

If I have a <select style="background: transparent"> then I get weird painting problems when I mouse over select elements and (on my Linux box only) weird painting artifacts when I simply open the select.

Karl wasn't sure if this was layout or gfx. and mstange suggested this might be related to bug 509876.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: screenshot

This is what I see (screenshot taken in a VM).

Comment 2

[Karl Tomlinson (:karlt)]

Sounds like it was just fortunate that this seemed to work before bug 502424 landed, but tn says he has some ideas here.  (Bug 408284 is related but won't be able to provide transparent widgets on all Linux systems.)

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: draw an opaque color on dropdowns

This is a regression from bug 502424, but only because bug 502424 did the right thing and we have been doing the wrong thing since display lists landed. The dropdown has an opaque widget and if the dropdown doesn't have an opaque background then we end up with garbage because we don't draw an opaque color everywhere. Before bug 502424 landed we would draw the viewport color behind the dropdown list whenever its widget was painted. For colors with alpha=255 you never saw the viewport color. But bug 502424 stopped painting the viewport color when the dropdown widget was painted (because it is incorrect).

If we keep the widget opaque then we need to draw something so just look up the style context tree composing background colors underneath until we have alpha = 255 and draw that over the whole widget. This of course is a horrible substitute for a translucent dropdown, but its the best we can do.

If we wanted to support actually translucent dropdowns we could set the dropdown widget to be transparent in nsCSSFrameConstructor::InitializeSelectFrame after creating it. This worked fine for me in Windows. We'd probably need to hook it up so that background and opacity style changes caused changes to the widget's transparency. When using a transparency widget there is a slight flicker the first time you use the drop down after a style change that changes the background. We'd probably also need to add something to nsIWidget to determine if the configuration we're on supports alpha transparency so that we can degrade gracefully.

This is not testable because drawWindow can't "see" the dropdown because it's in another widget.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: make the anonymous block frame child of combobox frame not inherit background-color [Checkin: Comment 22]

The anonymous block frame that a combobox control frame creates to display the selected option when not dropped down shouldn't inherit the background color of the combobox because it leads to duplicate backgrounds. And if that background color is translucent than the final color will be wrong. This declaration was added in 2000 in http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/style/html.css&rev=3.64 and hasn't changed since. The linked bug doesn't mention anything about this. It might have been put in because the drop down list does need to inherit the background color from the combobox. Or it might have been needed then, but I can't see any reason why it is still needed now.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With the background color patch, dynamic changes to the background color of an element that's an ancestor of a <select> might need to dynamically repaint the dropdown, but do they? I suspect not.

The behaviour of using the viewport background color behind the dropdown was sort of intentional as a way of dodging the problem here. I still think it's not bad behaviour, although I'm happy to take your patch if the dynamic change issue is easy to resolve.

Comment 6

[Timothy Nikkel (:tnikkel)]

We still have the dynamic change problem when using the viewport background color.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess that's true. However, depending on how we fix that, your change might make the fix harder.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What I mean is, we could special-case changes to the root element or body background to invalidate popups (those changes are already special-cased to invalidate the entire viewport), but doing something special for every element that might contain a <select> might be harder.

Comment 9

[Timothy Nikkel (:tnikkel)]

If we're going to go to that length to make sure that translucent dropdowns get updated correctly when dynamic changes happen to the rest of the document while they are dropped down, shouldn't we just make the widget transparent and do it properly instead? Does anybody actually want translucent dropdowns for <select>s?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know.

The main thing I want to avoid is bugs where you open a dropdown, the page does something, and then as you select and deselect options they're redrawn using a different background color than the first draw --- i.e. unpredictable visual artifacts.

So one thing we could do is precompute the background color when the dropdown is opened, and use that consistently thereafter.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: draw an opaque color on dropdowns v2

Compute and cache the backstop color to draw on dropdown, not on draw.

I don't really care whether we use the viewport color or this way.

Comment 12

[Timothy Nikkel (:tnikkel)]

I'm making this bug just about drawing something reasonable instead of garbage. If we actually want to do translucent dropdowns properly with a transparent widget then that will be followup.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

@@ -1733,16 +1743,37 @@ nsListControlFrame::AboutToDropDown()
 {
   if (mIsAllContentHere && mIsAllFramesHere && mHasBeenInitialized) {
     ScrollToIndex(GetSelectedIndex());
 #ifdef ACCESSIBILITY
     FireMenuItemActiveEvent(); // Inform assistive tech what got focus
 #endif
   }
   mItemSelectionStarted = PR_FALSE;
+
+  if (IsInDropDownMode()) {

Surely we wouldn't call AboutToDropDown on a listbox that's not in a dropdown?

+    // We start looking for backgrounds above the combobox frame to avoid
+    // duplicating the combobox frame's background

Why do you want to avoid duplicating the combobox frame's background? It's not rendered in the popup.

+    while (NS_GET_A(mLastDropdownBackstopColor) < 255 && context) {

The NS_GET_A short-circuit is probably not worth doing. In common cases (when no ancestor has a background color) it won't help.

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to comment #13)
> Surely we wouldn't call AboutToDropDown on a listbox that's not in a dropdown?

Ok.

> +    // We start looking for backgrounds above the combobox frame to avoid
> +    // duplicating the combobox frame's background
> 
> Why do you want to avoid duplicating the combobox frame's background? It's not
> rendered in the popup.

Perhaps I'm using the wrong terminology. What would you call the background that gets rendered on the popup? The list dropdown inherits the background-color from the <select> style (because of the rule *|*::-moz-dropdown-list { background-color: inherit; } in forms.css), and the comboboxframe is the main frame for the <select> content

> +    while (NS_GET_A(mLastDropdownBackstopColor) < 255 && context) {
> 
> The NS_GET_A short-circuit is probably not worth doing. In common cases (when
> no ancestor has a background color) it won't help.

I think it makes it clear what we are doing logically. Besides, its only a couple of shifts and logical ops, sure to be overshadowed by fetching from memory these pointers.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> > +    // We start looking for backgrounds above the combobox frame to avoid
> > +    // duplicating the combobox frame's background
> > 
> > Why do you want to avoid duplicating the combobox frame's background? It's not
> > rendered in the popup.
> 
> Perhaps I'm using the wrong terminology. What would you call the background
> that gets rendered on the popup? The list dropdown inherits the
> background-color from the <select> style (because of the rule
> *|*::-moz-dropdown-list { background-color: inherit; } in forms.css), and the
> comboboxframe is the main frame for the <select> content

OK, if you start searching at the combobox frame's background, then we could remove that forms.css rule.

> > +    while (NS_GET_A(mLastDropdownBackstopColor) < 255 && context) {
> > 
> > The NS_GET_A short-circuit is probably not worth doing. In common cases (when
> > no ancestor has a background color) it won't help.
> 
> I think it makes it clear what we are doing logically. Besides, its only a
> couple of shifts and logical ops, sure to be overshadowed by fetching from
> memory these pointers.

OK.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to comment #15)
> OK, if you start searching at the combobox frame's background, then we could
> remove that forms.css rule.

I think I prefer it how it is because it separates the hack to draw an opaque backstop color from the drawing of the real background color. If we ever wanted to go the transparent widget route we'd need to do this (so we can support configurations that don't have alpha transparency for widgets).

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 396175 [details] [diff] [review]
draw an opaque color on dropdowns v2

OK, r+ if you get rid of the IsInDropDownMode check (or turn it into an assertion)

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: draw an opaque color on dropdowns v3

Turned IsInDropDownMode check into an assertion.

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: draw an opaque color on dropdowns v4 [Checkin: Comment 21]

Previous patch re-declared mLastDropdownBackstopColor in AboutToDropDown, hiding the member variable, leading to bad results. I thought there was a compiler warning about this? I guess we don't have it turned on.

Comment 20

[Timothy Nikkel (:tnikkel)]

Just a quick note to the kind-hearted person who ends up fulfilling my checkin-needed request: there are two separate patches in this bug that need checkin. Sorry to not make that clear. (I happened to see a try-server push with just one of the patches while looking at something else.)

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I checked in the second patch. The first patch still needs checkin.
http://hg.mozilla.org/mozilla-central/rev/d023a0300d0f

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in first patch.

http://hg.mozilla.org/mozilla-central/rev/0c0883e236d5

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

With both patches applied, on a non-reduced testcase, I'm still seeing painting weirdness the first time I expand a <select>. The second and subsequent times work, though. Should I file a new bug?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On every select or just selects with background:transparent? Either way it's a new bug.

Comment 25

[Timothy Nikkel (:tnikkel)]

Filed bug 513185 for this issue.

Comment 26

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d4a76980bee4