Closed
Bug 467459
Opened 16 years ago
Closed 16 years ago
Painting issues (smearing) with <select> dropdown, with background image, in iframe
Categories
(Core :: Layout, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b3
People
(Reporter: jruderman, Assigned: zwol)
References
()
Details
(Keywords: regression, testcase, verified1.9.1)
Attachments
(3 files, 2 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081201 Minefield/3.2a1pre
Scrolling the dropdown causes the items to smear, making it difficult to select the correct item from the dropdown. This is a regression from Firefox 3.0.x.
I noticed this on a site that isn't public yet. I'll upload a simple testcase today, and tomorrow I can add a comment with the URL of the affected site.
Flags: blocking1.9.1?
Blocking on this gross regression!
Flags: blocking1.9.1? → blocking1.9.1+
|
||
Comment 3•16 years ago
|
||
Any chance of a regression range?
|
||
Updated•16 years ago
|
Keywords: regressionwindow-wanted
|
Reporter | |
Comment 4•16 years ago
|
||
As Joe Drew has probably guessed, I noticed this bug on a pre-launch version of http://open-government.us/.
Oh, huh, this is different than the bug I thought it was trying to show. I'm guessing what's happening here is that because there's an explicit background set, the code that paints the native background isn't being triggered -- correctly for the widget itself, incorrectly for the popup.
Well maybe not, it's drawn correctly the first time the popup is opened.
Assignee: nobody → vladimir
This is broken all the way back to 2008-11-08 -- I have a hunch that it might be related to/caused by bug 438987.
Nope, it's not.. works in 2008-08-08.
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-12&enddate=2008-09-13
Keywords: regressionwindow-wanted
|
|
||
Comment 10•16 years ago
|
||
Zack, could your changes in the regression range cause this?
It's not bug 453566 (did a local backout), but still could be bug 453916; unfortunately that patch doesn't back out cleanly and it's fairly large to fix by hand.
What I think is happening is that the broken popup has a transparent background, and before it was getting the DefaultBackgroundColor set on it (solid white). In the testcase, in a working build the first popup outside of the iframe has a slightly translucent background whereas the second one (that smears in a broken build) has a solid white background. So I'm guessing that some case where a transparent background was set to the background color isn't being done now.
|
Assignee | |
Comment 12•16 years ago
|
||
Will investigate.
|
|
Assignee | |
Comment 13•16 years ago
|
||
I can reproduce the problem with any partially or fully transparent background color, as well as with the transparent background image being used before, as long as the platform-native dropdown is suppressed. (The new test case uses a semitransparent color.)
Because of this, I'm baffled as to how this manages to work correctly in 3.0; in particular, if the bug was exposed by my patches in in bug 453916, then the semitransparent-color case should have been visibly buggy in 3.0.
However, I do have a hypothesis for the cause; the new test case has its colors set to illustrate it. First, note that when either control is inactive, the label for the currently selected choice is drawn with the control's background color composed with the parent element's background color (gray on top of yellow). When the outer control is active, though, the dropdown is drawn with the control's background color composed with the *body*'s background color (gray on top of green).
Now, the <iframe> has an opaque background color (white), but the implicit <body> element for the document inside the <iframe> has a semitransparent blue background color. Normally, that color gets painted on top of the parent <iframe>'s background and all is well. In the dropdown, though, we seem to be composing the semitransparent gray with the semitransparent blue and not going any farther than that.
I have not yet found the code that handles this color composition, so I'm not sure how invasive the fix will need to be.
|
|
Assignee | |
Comment 14•16 years ago
|
||
With the simpler test case, if you scrub the mouse back and forth enough it will show in 3.0.x. So the only sense in which this is a regression is that (due to the changes in bug 453916) it's now visible for the default 'transparent'.
|
|
Assignee | |
Comment 15•16 years ago
|
||
I spent an awful long time looking for the code doing the color composition in the various frames involved and the rendering code they call, but it turns out not to be there at all -- it's nsPresShell::Paint() that sets an ultimate background color for this kind of floating frame. That color is the current view manager's DefaultBackgroundColor, and if that color is (semi-)transparent, we lose.
(Before the patches in bug 453916 that color would in fact have been white by default, I believe.)
This patch walks up the parent chain of the view managers (awkwardly - the direct way to do that is private to view/, and the indirect way involves nsIView::GetViewManager(), which has a "this may become expensive in the future" warning attached to it) and composites all their default background colors. If what we get out of that is still not fully opaque, we throw the pres context's default background color (which is guaranteed to be opaque) underneath that.
I'm calling this a candidate patch because I'm not sure it wouldn't be better to skip walking the view manager hierarchy and just go straight to the pres context's default background color, or somehow find out the color associated with the *immediate* parent of the root view and use that. Also I have no idea how to write a test case for this one.
Assignee: vladimir → zweinberg
Status: NEW → ASSIGNED
Attachment #352438 -
Flags: superreview?(roc)
Attachment #352438 -
Flags: review?(roc)
Component: GFX: Thebes → Layout
QA Contact: thebes → layout
+ NS_ASSERTION(PR_FALSE, "painting a destroyed PresShell");
Make this NS_ERROR while you're here.
+ PRBool needTransparency = PR_FALSE;
for (nsIView *view = aView; view; view = view->GetParent()) {
if (view->HasWidget()) {
// Both glass and transparent windows need the transparent bg color
if (eTransparencyOpaque != view->GetWidget()->GetTransparencyMode()) {
- backgroundColor = NS_RGBA(0,0,0,0);
+ needTransparency = PR_TRUE;
break;
}
}
}
Actually the best thing to do is probably to find the backstop here in this loop.
For each view, get its view manager and check whether the view manager's default background is opaque; if it is, set backgroundColor to that.
|
|
||
Comment 17•16 years ago
|
||
So that will work for now because we don't tie content into the chrome viewmanager. But when we do, that will walk into the chrome and get its background color in some cases, no? That doesn't sound good.
The topmost content document's "default background color" (whether in nsViewManager or stored elsewhere, when view managers have gone away), will always have to be opaque. That's needed not just here but for background painting in general.
|
|
||
Comment 19•16 years ago
|
||
Ah, ok. As long as we have tests for that, life's good.
|
||
Updated•16 years ago
|
Priority: -- → P2
|
|
Assignee | |
Comment 20•16 years ago
|
||
Here's a revised patch incorporating roc's suggestions. Also, I tried to write a test using mochitest EventUtils and WindowSnapshot libraries. It doesn't work (the snapshots do not include the dropped-down list, and it fails on inexplicable graphical differences even with the patch) but I don't have time today to beat my head on it anymore.
Attachment #352438 -
Attachment is obsolete: true
Attachment #352438 -
Flags: superreview?(roc)
Attachment #352438 -
Flags: review?(roc)
Comment on attachment 352620 [details] [diff] [review]
(rev 2) roc's changes + attempt at test case
We don't have a way to test dropdown rendering, unfortunately. Take the test out and let's get the fix landed.
Attachment #352620 -
Flags: superreview+
Attachment #352620 -
Flags: review+
|
|
Assignee | |
Comment 22•16 years ago
|
||
Ok, here is a version with no test.
Attachment #352620 -
Attachment is obsolete: true
Attachment #352625 -
Flags: superreview?(roc)
Attachment #352625 -
Flags: review?(roc)
|
|
Assignee | |
Updated•16 years ago
|
Flags: in-litmus?
Attachment #352625 -
Flags: superreview?(roc)
Attachment #352625 -
Flags: superreview+
Attachment #352625 -
Flags: review?(roc)
Attachment #352625 -
Flags: review+
|
|
Assignee | |
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
|
||
Comment 23•16 years ago
|
||
Comment on attachment 352625 [details] [diff] [review]
(rev 3) test dropped
[Checkin: Comment 23 & 24]
http://hg.mozilla.org/mozilla-central/rev/e3c43873e9b2
Attachment #352625 -
Attachment description: (rev 3) test dropped → (rev 3) test dropped
[Checkin: Comment 23]
|
|
||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
|
|
||
Comment 24•16 years ago
|
||
Comment on attachment 352625 [details] [diff] [review]
(rev 3) test dropped
[Checkin: Comment 23 & 24]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9bc278078aee
Attachment #352625 -
Attachment description: (rev 3) test dropped
[Checkin: Comment 23] → (rev 3) test dropped
[Checkin: Comment 23 & 24]
|
|
||
Updated•16 years ago
|
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
|
||
Comment 25•15 years ago
|
||
verified FIXED (no smears on both testcases) on debug builds:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
|
|
Assignee | |
Updated•15 years ago
|
Flags: in-litmus?
You need to log in
before you can comment on or make changes to this bug.
Description
•