Closed
Bug 511323
Opened 15 years ago
Closed 15 years ago
Painting weirdness involving a <select> with a transparent background
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: mrbkap, Assigned: tnikkel)
References
Details
(Keywords: regression)
Attachments
(4 files, 3 obsolete files)
167 bytes,
text/html
|
Details | |
58.19 KB,
image/png
|
Details | |
1.01 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
4.40 KB,
patch
|
roc
:
approval1.9.2+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
This is what I see (screenshot taken in a VM).
Comment 2•15 years ago
|
||
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 | ||
Comment 3•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
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•15 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•15 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•15 years ago
|
||
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•15 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•15 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•15 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•15 years ago
|
||
Turned IsInDropDownMode check into an assertion.
Attachment #396175 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 19•15 years ago
|
||
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•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•15 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•15 years ago
|
Attachment #396291 -
Flags: approval1.9.2?
Checked in first patch.
http://hg.mozilla.org/mozilla-central/rev/0c0883e236d5
Reporter | ||
Comment 23•15 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 | ||
Comment 25•15 years ago
|
||
Filed bug 513185 for this issue.
Attachment #396291 -
Flags: approval1.9.2? → approval1.9.2+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [needs 192 landing]
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Updated•15 years ago
|
Attachment #396291 -
Attachment description: draw an opaque color on dropdowns v4 → draw an opaque color on dropdowns v4
[Checkin: Comment 21]
Updated•15 years ago
|
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]
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.3a1
Assignee | ||
Comment 26•15 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•