If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Painting weirdness involving a <select> with a transparent background

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mrbkap, Assigned: tnikkel)

Tracking

({regression})

Trunk
mozilla1.9.3a1
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 395228 [details]
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.
(Reporter)

Comment 1

8 years ago
Created attachment 395230 [details]
screenshot

This is what I see (screenshot taken in a VM).
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.)
Assignee: nobody → tnikkel
Blocks: 502424
Depends on: 408284
(Assignee)

Comment 3

8 years ago
Created attachment 395779 [details] [diff] [review]
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.
Attachment #395779 - Flags: review?(roc)
(Assignee)

Comment 4

8 years ago
Created attachment 395781 [details] [diff] [review]
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.
Attachment #395781 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Blocks: 317375
Flags: blocking1.9.2?
Keywords: regression
OS: Linux → All
Hardware: x86 → All
Attachment #395781 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 6

8 years ago
We still have the dynamic change problem when using the viewport background color.
No longer blocks: 317375
I guess that's true. However, depending on how we fix that, your change might make the fix harder.
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.
(Assignee)

Comment 9

8 years ago
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?
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.
(Assignee)

Comment 11

8 years ago
Created attachment 396175 [details] [diff] [review]
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.
Attachment #395779 - Attachment is obsolete: true
Attachment #396175 - Flags: review?(roc)
Attachment #395779 - Flags: review?(roc)
(Assignee)

Comment 12

8 years ago
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.
No longer depends on: 408284
@@ -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.
(Assignee)

Comment 14

8 years ago
(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.
> > +    // 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.
(Assignee)

Comment 16

8 years ago
(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 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)
Attachment #396175 - Flags: review?(roc) → review+
(Assignee)

Comment 18

8 years ago
Created attachment 396264 [details] [diff] [review]
draw an opaque color on dropdowns v3

Turned IsInDropDownMode check into an assertion.
Attachment #396175 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

8 years ago
Created attachment 396291 [details] [diff] [review]
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.
Attachment #396264 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

8 years ago
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.)
I checked in the second patch. The first patch still needs checkin.
http://hg.mozilla.org/mozilla-central/rev/d023a0300d0f
(Assignee)

Updated

8 years ago
Attachment #396291 - Flags: approval1.9.2?
Checked in first patch.

http://hg.mozilla.org/mozilla-central/rev/0c0883e236d5
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Reporter)

Comment 23

8 years ago
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?
On every select or just selects with background:transparent? Either way it's a new bug.
(Assignee)

Updated

8 years ago
Depends on: 513185
(Assignee)

Comment 25

8 years ago
Filed bug 513185 for this issue.
Attachment #396291 - Flags: approval1.9.2? → approval1.9.2+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Attachment #396291 - Attachment description: draw an opaque color on dropdowns v4 → draw an opaque color on dropdowns v4 [Checkin: Comment 21]
Attachment #395781 - Attachment description: make the anonymous block frame child of combobox frame not inherit background-color → make the anonymous block frame child of combobox frame not inherit background-color [Checkin: Comment 22]
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Comment 26

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d4a76980bee4
status1.9.2: --- → beta1-fixed
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
You need to log in before you can comment on or make changes to this bug.