613449 - DWM fails to composite plugin windows correctly when they're over glass margins

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

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.

Comment 1

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

Ah, I see now that with only 1px of margin some of the DWM's border does indeed leak out.

Comment 2

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

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.

Comment 3

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

Attachment: D3DTest showing how to patch up alpha values

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.

Comment 4

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

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.

Comment 5

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

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.

Comment 6

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

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.

Comment 7

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

Attachment: Part 1: Move border-side opacity checking to nsStyleBorder

This also moves NS_OPPOSITE_SIDE to nsStyleConst so other code can use it.

Comment 8

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

Attachment: Part 2: treat single-side borders as opaque areas when possible

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.

Comment 9

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

Attachment: Part 3: Add aContainingRect parameter to GetLargestRectangle

Very simple extension of GetLargestRectangle. We'll need this for the next patch.

Comment 10

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

Attachment: Part 4: Ensure we cover all plugins when we choose a "largest rectangle" for Aero Glass margins

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.

Comment 11

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

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

[Rob Arnold [:robarnold]]

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.

Comment 13

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

Actually, patches 3 and 4 are a bit too hacky, I can do better.

Comment 14

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

Attachment: Part 3 v2

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.

Comment 15

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

Attachment: Part 4 v2

We should try to find a "largest rectangle" that will include the plugin bounds *after* we've shrunk it by kGlassMarginAdjustment.

Comment 16

[Jim Mathies [:jimm]]

(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?

Comment 17

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

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

[Jim Mathies [:jimm]]

(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

[Jim Mathies [:jimm]]

..and if it does, what happens if we change kGlassMarginAdjustment to 1px?

Comment 20

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

(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

[Jim Mathies [:jimm]]

Comment on attachment 492436 [details] [diff] [review]
Part 4 v2

ok!

Comment 22

[Rob Arnold [:robarnold]]

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.

Comment 23

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

Which prose comment needs to be updated? I did update the comment for nsRegion::GetLargestRectangle.

Comment 24

[Rob Arnold [:robarnold]]

The big comment block above the code that describes the algorithm.

Comment 25

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

Attachment: Part 3 v3

Per comments

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 492282 [details] [diff] [review]
Part 1: Move border-side opacity checking to nsStyleBorder

r=dbaron

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 28

[Jem]

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.

Comment 29

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

A screenshot would be great.

Comment 30

[Jem]

(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

[Jem]

Sorry that last link doesn't work. Try this one:

http://img716.imageshack.us/img716/6695/09122010143316.jpg

Comment 32

[Jem]

Still present as above in 20101210...

Comment 33

[Giorgio]

http://img709.imageshack.us/img709/3400/bugfn.png

is this the bug you are talking about?

Comment 34

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

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

[Giorgio]

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

Comment 36

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

Then it's not this bug

Comment 37

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

This should block, it's a regression.

Comment 38

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

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.