Closed
Bug 613449
Opened 14 years ago
Closed 14 years ago
DWM fails to composite plugin windows correctly when they're over glass margins
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: roc, Assigned: roc)
References
Details
(Keywords: regression)
Attachments
(5 files, 3 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
robarnold
:
review+
|
Details | Diff | Splinter Review |
See bug 590568 comments 93 to 104.
The standalone test app in comment #104 clearly shows that the DWM fails hard where a child window is over the glass margins. This causes at least two visible bugs in Firefox:
a) https://bugzilla.mozilla.org/attachment.cgi?id=491440
When a windowed plugin is at the edge of our window, you get a couple of rows or columns of weird pixels. I'm pretty sure this is due to the plugin being over the two-pixel margin we add for borderless glass:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#2526
b) https://bug590568.bugzilla.mozilla.org/attachment.cgi?id=491442
When you make the window very short, the entire plugin becomes subject to "the effect". I think here our "find the largest rectangle in the opaque region" analysis is choosing the URL bar as the largest rectangle and extending the glass margin over the entire content area. Probably because the 1px bottom-border of the URL bar is not marked as opaque by display list analysis, so our opaque region is split into two separate pieces.
Problem (b) is fixable by enhancing display list analysis a bit more so we know that the URL bar and the content area form a contiguous opaque rect. Or, we could modify the way we find the largest opaque rectangle to ensure it always includes all windowed plugins (if possible). I'll probably go with the display list enhancement since it should also improve performance (smaller glass margins).
Problem (a) may not be completely fixable. That extra margin was added in bug 554982. I'm not sure why 2px was chosen ... can we make it 1px? I assume we know of no other way to hide the DWM border? It's not the end of the world if we can't fix it.
Assignee | ||
Comment 1•14 years ago
|
||
Ah, I see now that with only 1px of margin some of the DWM's border does indeed leak out.
Assignee | ||
Comment 2•14 years ago
|
||
OK, so as suggested obliquely by some DWM documentation, the problem is that GDI drawing into the child window is not setting the alpha values of the child window pixels correctly. If I modify D3DTest.cpp to use AlphaBlend to draw into the child window, the problem goes away.
Assignee | ||
Comment 3•14 years ago
|
||
It seems to be possible to "fix up" window alpha values after they've been painted. This code shows how, see ChildMsgProc. But it's horrible, we have to copy the contents of the window to a DIB, set all the alpha values in the DIB to 255, then AlphaBlend the contents of the DIB back to the window. Blech.
It's probably not worth doing this just to fix part (a) of this bug.
Assignee | ||
Comment 4•14 years ago
|
||
Embedding the window inside (on top of) another child window that we paint with AlphaBlend doesn't seem to work. It seems like the DWM uses something like OPERATOR_SOURCE to composite the child windows into the parent. It seems crazy.
Assignee | ||
Comment 5•14 years ago
|
||
The approach in comment #3 could perform OK since we only have to do the fixup for the part of the child window that overlaps the glass borders, and typically those borders will only be 2 pixels wide.
But trapping WM_PAINT for all the child windows for the plugin would be really painful. Maybe we could use Windows hooks or something, but it would fall apart if more processes than the browse and the plugin-container are involved. I don't want to go down this road.
Assignee | ||
Comment 6•14 years ago
|
||
Especially since so far I'm not aware of any user-filed bugs on this issue. Flash is probably almost always drawing its window with correct alpha values.
Assignee | ||
Comment 7•14 years ago
|
||
This also moves NS_OPPOSITE_SIDE to nsStyleConst so other code can use it.
Attachment #492282 -
Flags: review?(dbaron)
Assignee | ||
Comment 8•14 years ago
|
||
When a border is just painting a single filled opaque rectangle, let GetOpaqueRegion return that rectangle.
Currently, in the Firefox chrome, the row of pixels between the URL bar and the content area is a bottom-only border. This lets us know it's opaque, so we can know the whole URL bar + content area is opaque, which lets us use smaller Aero Glass margins for the chrome.
Attachment #492283 -
Flags: review?(dbaron)
Assignee | ||
Comment 9•14 years ago
|
||
Very simple extension of GetLargestRectangle. We'll need this for the next patch.
Attachment #492284 -
Flags: review?(tellrob)
Assignee | ||
Comment 10•14 years ago
|
||
This bug makes it important that the rectangle chosen as the basis for computing Aero Glass margins contains all plugins, so we minimize the area of plugins over the glass margins. This patch ensures that.
Attachment #492286 -
Flags: review?(jmathies)
Assignee | ||
Comment 11•14 years ago
|
||
These patches fix problem (b) in comment #0. Problem (a) is partially fixed; with tabs-on-top and the URL bar showing, the top Aero Glass margin only reaches down to the top of the URL bar, so plugins in the content area are nowhere near it so "the effect" is not visible at the top edge of plugins. It's still visible on the other sides if the plugin is at the edge of the window. Hopefully it won't really show up.
Now that I've done this, I wonder if these problems are visible on any plugin except for our test plugin ... I find it hard to believe that all other plugins set alpha values in their windows, though.
Comment 12•14 years ago
|
||
Comment on attachment 492284 [details] [diff] [review]
Part 3: Add aContainingRect parameter to GetLargestRectangle
You should probably update the prose description of the algorithm as well. It's also not clear to me why Parts 1 & 2 don't fix this, based on comment 0.
Attachment #492284 -
Flags: review?(tellrob) → review+
Updated•14 years ago
|
Attachment #492286 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 13•14 years ago
|
||
Actually, patches 3 and 4 are a bit too hacky, I can do better.
Assignee | ||
Comment 14•14 years ago
|
||
Sorry about the double review Rob. We can make SetLargestRectangle more general; there is no need to restrict the rectangle to being contained in the region (I'll use this in a revised part 4). Also, folding the in-rectangle bonus value into the area PRInt64 was a hack. Instead, use a proper pair of PRInt64s.
Attachment #492432 -
Flags: review?(tellrob)
Assignee | ||
Comment 15•14 years ago
|
||
We should try to find a "largest rectangle" that will include the plugin bounds *after* we've shrunk it by kGlassMarginAdjustment.
Attachment #492436 -
Flags: review?(jmathies)
Comment 16•14 years ago
|
||
(In reply to comment #15)
> Created attachment 492436 [details] [diff] [review]
> Part 4 v2
>
> We should try to find a "largest rectangle" that will include the plugin bounds
> *after* we've shrunk it by kGlassMarginAdjustment.
With this, wouldn't we end up with plugins that have borders where they overlap the side of the window? Or were those borders removed with 130078? In which case, do we still need the adjustment?
Assignee | ||
Comment 17•14 years ago
|
||
I'm not sure what you mean. We definitely still need to add kGlassMarginAdjustment to the glass margins to push the DWM's border under our opaque content.
Comment 18•14 years ago
|
||
(In reply to comment #17)
> I'm not sure what you mean. We definitely still need to add
> kGlassMarginAdjustment to the glass margins to push the DWM's border under our
> opaque content.
Hrm, Part 3 doesn't apply cleanly, so I can't test this.
I'm trying to understand if there's a negative side effect from this. The 2px adjustment hides a window border on the client area of the browser. (Which, AFAICT is actually only 1px, but that's a different discussion.) We inflate the client bounds by 2px and calculate the largest opaque region including the added margin. then we subtract the 2px from the result. Which would seem to leave us with the client area border on any side of the window where a plugin is clipped by the window frame.
If you apply these and load a plugin, then scrunch the lower frame border up so that the plugin is clipped by the frame, does the client area border suddenly show up?
Comment 19•14 years ago
|
||
..and if it does, what happens if we change kGlassMarginAdjustment to 1px?
Assignee | ||
Comment 20•14 years ago
|
||
(In reply to comment #18)
> (In reply to comment #17)
> > I'm not sure what you mean. We definitely still need to add
> > kGlassMarginAdjustment to the glass margins to push the DWM's border under our
> > opaque content.
>
> Hrm, Part 3 doesn't apply cleanly, so I can't test this.
>
> I'm trying to understand if there's a negative side effect from this. The 2px
> adjustment hides a window border on the client area of the browser. (Which,
> AFAICT is actually only 1px, but that's a different discussion.)
Yeah, but I tried changing it to 1px and you can see blurred border leaking out. Or something like that. It definitely looked bad. See comment #1.
> We inflate the
> client bounds by 2px and calculate the largest opaque region including the
> added margin. then we subtract the 2px from the result. Which would seem to
> leave us with the client area border on any side of the window where a plugin
> is clipped by the window frame.
No. GetLargestRectangle always returns a rectangle that's contained in the window opaque region. So if the plugin bounds + 2px margin extends outside the window opaque region, the part outside the window opaque region does not affect the result of GetLargestRectangle.
> If you apply these and load a plugin, then scrunch the lower frame border up so
> that the plugin is clipped by the frame, does the client area border suddenly
> show up?
No.
Comment 21•14 years ago
|
||
Comment on attachment 492436 [details] [diff] [review]
Part 4 v2
ok!
Attachment #492436 -
Flags: review?(jmathies) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #492284 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #492286 -
Attachment is obsolete: true
Comment 22•14 years ago
|
||
Comment on attachment 492432 [details] [diff] [review]
Part 3 v2
The prose comment needs to be updated and this really ought to have tests. Otherwise looks good.
Attachment #492432 -
Flags: review?(tellrob) → review-
Assignee | ||
Comment 23•14 years ago
|
||
Which prose comment needs to be updated? I did update the comment for nsRegion::GetLargestRectangle.
Comment 24•14 years ago
|
||
The big comment block above the code that describes the algorithm.
Updated•14 years ago
|
Attachment #493158 -
Flags: review?(tellrob) → review+
Comment 26•14 years ago
|
||
Comment on attachment 492282 [details] [diff] [review]
Part 1: Move border-side opacity checking to nsStyleBorder
r=dbaron
Attachment #492282 -
Flags: review?(dbaron) → review+
Updated•14 years ago
|
Attachment #492432 -
Attachment is obsolete: true
Comment 27•14 years ago
|
||
Comment on attachment 492283 [details] [diff] [review]
Part 2: treat single-side borders as opaque areas when possible
>+ switch (i) {
>+ case eSideTop: aResult->height = m; break;
>+ case eSideBottom: aResult->y = aResult->YMost() - m; aResult->height = m; break;
>+ case eSideLeft: aResult->width += m; break;
This should be = rather than +=.
>+ case eSideRight: aResult->x = aResult->XMost() - m; aResult->width = m; break;
>+ default:
>+ NS_ERROR("Bad side value");
>+ break;
Given that the other cases are on one line, put default: ... break; all on one line too.
>+ }
>+ *aSide = i;
>+ return PR_TRUE;
>+ }
>+ }
>+ return PR_FALSE;
>+}
>+
>+nsRegion
>+nsDisplayBorder::GetOpaqueRegion(nsDisplayListBuilder* aBuilder,
>+ PRBool* aOutTransparentBackground)
The parameter name here disagrees with aForceTransparentSurface in part 3 of bug 602757. (Not sure which way you want to converge.)
r=dbaron with those things fixed
Attachment #492283 -
Flags: review?(dbaron) → review+
Comment 28•14 years ago
|
||
Build 20101208. D3D9. Hardware acceleration back on. Flash plugin no longer overlays as such when scrolling up, however with multiple tabs open at the time, the tabs in the immediate area above the plugin are 'wiped / erased'. Hope that's clear - if not I'll clarify.
Assignee | ||
Comment 29•14 years ago
|
||
A screenshot would be great.
Comment 30•14 years ago
|
||
(In reply to comment #29)
> A screenshot would be great.
http://img535.imageshack.us/img535/4316/09122010142821.jpg
I also get this:
http://img716.imageshack.us/img71
Comment 31•14 years ago
|
||
Sorry that last link doesn't work. Try this one:
http://img716.imageshack.us/img716/6695/09122010143316.jpg
Comment 32•14 years ago
|
||
Still present as above in 20101210...
Comment 33•14 years ago
|
||
http://img709.imageshack.us/img709/3400/bugfn.png
is this the bug you are talking about?
Assignee | ||
Comment 34•14 years ago
|
||
I'm not sure.
There's no need to keep reminding us that the bug exists. The patches have not been checked in.
Comment 35•14 years ago
|
||
i don't want to remind you that the bug exists, i don't know if i need to file a new bug, in my shot no plugins were used, just xul
Assignee | ||
Comment 36•14 years ago
|
||
Then it's not this bug
Assignee | ||
Comment 37•14 years ago
|
||
This should block, it's a regression.
Assignee | ||
Comment 38•14 years ago
|
||
I checked in parts 3 and 4:
http://hg.mozilla.org/mozilla-central/rev/cf82d60b3f1c
http://hg.mozilla.org/mozilla-central/rev/9713b198fe3d
Currently, the Firefox chrome has a transparent row of pixels directly above the content area, so parts 1 and 2 don't help at all. So I'm not checking those in.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•