622328 - on window re-size part of Adobe's Acrobat Reader X plugin becomes transparent

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Ahmed Harthi]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b9pre) Gecko/20101231 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b9pre) Gecko/20101231 Firefox/4.0b9pre

When I re-size the window of FireFox with Acrobat X reader plugin viewed, the repainted area of the plugin becomes transparent.

I experienced this bug only with Adobe Acrobat Reader X.

Reproducible: Always

Steps to Reproduce:
1. Make sure that D2D is enabled
2. Open a new window
3. go to https://wiki.mozilla.org/images/e/ed/Analyst_report_Q1_2010.pdf
4. Re-size the window, make it smaller
5. Maximize or enlarge the window
Actual Results:  
The new repainted area will be transparent.

Expected Results:  
Normal behavior, no transparent area.

Graphics
        Adapter Description ATI Mobility Radeon HD 3400 Series
        Vendor ID 1002
        Device ID 95c4
        Adapter RAM 256
        Adapter Drivers aticfx32 aticfx32 atiumdag atidxx32 atiumdva
        Driver Version 8.801.0.0
        Driver Date 11/25/2010
        Direct2D Enabled true
        DirectWrite Enabled true
        WebGL Renderer TransGaming Inc. -- ANGLE -- OpenGL ES 2.0 (git-devel Dec 31 2010 03:42:46)
      GPU Accelerated Windows 2/2 Direct3D 10

Comment 1

[kolubinowicki]

Attachment: Screencast of glitches Minefield Gecko/20101231 Firefox/4.0b9pre

I can confirm the glitches when resizing a browser window. Check the attached screencast for more info.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Glitch (no D2D/D3D)

I can reproduce this issue with both D3D9 layers and no accelerated layers at all. See screenshot, this was taken without any hardware acceleration whatsoever.

Comment 3

[kolubinowicki]

D2D could be switched off as stated from troubleshooting information.

Windows 7 32bit
Adapter Description ATI MOBILITY RADEON X1300
Vendor ID 1002
Device ID714a
Adapter RAM 128MB 
Drivers atiumdag atiumdva atitmmxx 
Driver Version 8.593.100.0 
Driver Date 2-10-2010 
Direct2D Enabled false 
DirectWrite Enabled false 
GPU Accelerated Windows 1/1 Direct3D 9

Comment 4

[Bas Schouten (:bas.schouten)]

Unassigning as this is not a hardware acceleration bug, of course if noone else
has time to look into this I don't mind. But I'm not the most obvious choice.
This is trivially reproducable for me fwiw.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: More interesting screenshot glitch

Adding another screenshot as this shows some more interesting aspects of this glitch.

Comment 6

[Benjamin Smedberg]

Is this a regression from Firefox 3.6? If so, we could really use a regression range (even if it's just bug 130078, it would be nice to know).

Comment 7

[Timothy Nikkel (:tnikkel)]

Regression range
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b0c6a324e72f&tochange=ef42c524718f

Comment 8

[Timothy Nikkel (:tnikkel)]

Used hourlies to narrow this down to just bug 620608. It must be exposing an underlying bug.

Comment 9

[Timothy Nikkel (:tnikkel)]

It looks like maybe we aren't setting the glass region on the window correctly?

Comment 10

[Benjamin Smedberg]

roc/jimm, can one of you take this?

Comment 11

[Timothy Nikkel (:tnikkel)]

Should retest after bug 621222 lands.

Comment 12

[Dão Gottwald [:dao]]

Bug 621222 will likely mask the underlying bug again.

Comment 13

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=590568#c104.

The Windows DWM (desktop compositor) simply fails to properly composite child windows that are over glassy parts of our window. There is nothing we can do about it. I have demonstrated the bug in a standalone C++ program.

Assuming that's what's actually going on here, I think we should treat bug 621222 as a fix for this bug and close this bug.

Comment 14

[Bill Gianopoulos [:WG9s]]

Unfortunately, the patch in bug 621222 does not help at all for this issue.  I did some testing by doing userChrome.css changes and it seems that it is necessary to put the non-transparent background color on #browser, which essentially reverts the change from bug 620608.

This issue also occurs with the JAVA plug-in and makes it impossible to correctly view a presentation using IBM/Lotus Sametime on a Windows PC using VISTA aero glass.

Comment 15

[Alfred Kayser]

Attachment: A more visible example of the problem.

A quicker way to show this effect is to shrink the pdf document view, and then switch to tabview and back to normal view. Not only is large part of the #appcontent/browser transparent, even the document itself behaves more or transpaent (the blue letters). This is with my theme with Glass support, but with #appcontent and browser background color specified (to white resp. red).

Comment 16

[Alfred Kayser]

Notice, the issue of bug 621222 does not happen in this theme (nautipolis), nor in default. So, that is clearly a separate issue.

It seems that the plugin frame is behaving badly. The plugin container is transparent, which it should be, but somehow it forgets that there is background behind in the full page mode?

Comment 17

[Jim Mathies [:jimm]]

Attachment: pdf fix v.1

This fixes the problem. The result returned from GetLargestRectangle is incorrect, I'll try to track down the cause of this. Might be a bug in GetLargestRectangle.

Comment 18

[Bill Gianopoulos [:WG9s]]

(In reply to comment #17)
> Created attachment 504770 [details] [diff] [review]
> pdf fix v.1
> 
> This fixes the problem. The result returned from GetLargestRectangle is
> incorrect, I'll try to track down the cause of this. Might be a bug in
> GetLargestRectangle.

This avoids the issue for me.

Comment 19

[Jim Mathies [:jimm]]

(In reply to comment #18)
> (In reply to comment #17)
> > Created attachment 504770 [details] [diff] [review]
> > pdf fix v.1
> > 
> > This fixes the problem. The result returned from GetLargestRectangle is
> > incorrect, I'll try to track down the cause of this. Might be a bug in
> > GetLargestRectangle.
> 
> This avoids the issue for me.

It also (re?)introduces some problems related to bug 613449. I'll post an update here sometime today.

Comment 20

[Jim Mathies [:jimm]]

Attachment: opaque region fix v.1

This seems to address things. When a plugin is filling the entire window, we end up with an opaqueRegion that has thin bands of empty space. Due to this GetLargestRectangle was calculating an opaque rect smaller than we want. For example, this is the typical contents of opaqueRegion after resizing the window down:

y: 25 -> 33 x(14 -> 246)
y: 25 -> 33 x(264 -> 277)
y: 33 -> 46 x(5 -> 254)
y: 33 -> 46 x(255 -> 285)
y: 46 -> 47 x(5 -> 254)
y: 112 -> 474 x(1 -> 596)
y: 474 -> 476 x(579 -> 596)
y: 476 -> 480 x(1 -> 596)
y: 480 -> 481 x(579 -> 596)
y: 481 -> 483 x(1 -> 596)
y: 483 -> 484 x(579 -> 596)
y: 484 -> 489 x(1 -> 596)
y: 489 -> 490 x(579 -> 596)
y: 490 -> 494 x(1 -> 596)
y: 494 -> 495 x(579 -> 596)
y: 495 -> 498 x(1 -> 596)
y: 498 -> 499 x(579 -> 596)
plugin : left=-1 top=110 right=581 bottom=514
largest: left=1 top=112 right=596 bottom=474

GetLargestRectangle stops on the first full band y: 112 -> 474 x(1 -> 596) leaving us with margins that "stick".

The above patch adds the visible plugin area to opaqueRegion, which removes these bands.

Also up at the top I've clipped mPossiblyTransparentRegion to the current client bounds. mPossiblyTransparentRegion was accumulating everything that was fed into it, including rects outside the bounds after a resize. After resizing the window a few of times it's contents grew considerably.

This isn't quite complete, I need to work out a few remaining quirks.

Comment 21

[Jim Mathies [:jimm]]

Attachment: scrollable test plugin test case

Comment 22

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

(In reply to comment #20)
> This seems to address things. When a plugin is filling the entire window, we
> end up with an opaqueRegion that has thin bands of empty space. Due to this
> GetLargestRectangle was calculating an opaque rect smaller than we want. For
> example, this is the typical contents of opaqueRegion after resizing the window
> down:
> 
> y: 25 -> 33 x(14 -> 246)
> y: 25 -> 33 x(264 -> 277)
> y: 33 -> 46 x(5 -> 254)
> y: 33 -> 46 x(255 -> 285)
> y: 46 -> 47 x(5 -> 254)
> y: 112 -> 474 x(1 -> 596)
> y: 474 -> 476 x(579 -> 596)
> y: 476 -> 480 x(1 -> 596)
> y: 480 -> 481 x(579 -> 596)
> y: 481 -> 483 x(1 -> 596)
> y: 483 -> 484 x(579 -> 596)
> y: 484 -> 489 x(1 -> 596)
> y: 489 -> 490 x(579 -> 596)
> y: 490 -> 494 x(1 -> 596)
> y: 494 -> 495 x(579 -> 596)
> y: 495 -> 498 x(1 -> 596)
> y: 498 -> 499 x(579 -> 596)

I'd really like to know how these bands happen!!!

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #22)
> I'd really like to know how these bands happen!!!

They start showing up due to values in aDirtyRegion, which don't include the plugin. Here's an example, this is dump of all the region math we do as soon as the window is resized down:

mPossiblyTransparentRegion:
y: 0 -> 25 (0 -> 742)
y: 25 -> 33 (0 -> 14)
y: 25 -> 33 (246 -> 742)
y: 33 -> 47 (0 -> 5)
y: 33 -> 47 (254 -> 742)
y: 47 -> 56 (0 -> 742)
y: 56 -> 58 (0 -> 104)
y: 56 -> 58 (124 -> 742)
y: 58 -> 72 (0 -> 102)
y: 58 -> 72 (126 -> 742)
y: 72 -> 74 (0 -> 104)
y: 72 -> 74 (124 -> 742)
y: 74 -> 112 (0 -> 742)
y: 112 -> 853 (0 -> 1)
y: 112 -> 853 (741 -> 742)
y: 853 -> 854 (0 -> 742) ** lower strip
mPossiblyTransparentRegion+clientBounds:
y: 0 -> 25 (0 -> 742)
y: 25 -> 33 (0 -> 14)
y: 25 -> 33 (246 -> 742)
y: 33 -> 47 (0 -> 5)
y: 33 -> 47 (254 -> 742)
y: 47 -> 56 (0 -> 742)
y: 56 -> 58 (0 -> 104)
y: 56 -> 58 (124 -> 742)
y: 58 -> 72 (0 -> 102)
y: 58 -> 72 (126 -> 742)
y: 72 -> 74 (0 -> 104)
y: 72 -> 74 (124 -> 742)
y: 74 -> 112 (0 -> 742)
y: 112 -> 853 (0 -> 1)
y: 112 -> 853 (741 -> 742)
y: 853 -> 854 (0 -> 742) ** lower strip
aDirtyRegion:
y: 0 -> 112 (0 -> 742)
y: 112 -> 854 (0 -> 1)
y: 112 -> 854 (401 -> 742) ** everything in content but the plugin
y: 854 -> 855 (0 -> 742)
mPossiblyTransparentRegion-aDirtyRegion:
y: 853 -> 854 (1 -> 401) ** the first band that forms
mPossiblyTransparentRegion|aPossiblyTransparentRegion:
y: 0 -> 25 (0 -> 742)
y: 25 -> 33 (0 -> 14)
y: 25 -> 33 (246 -> 742)
y: 33 -> 47 (0 -> 5)
y: 33 -> 47 (254 -> 742)
y: 47 -> 56 (0 -> 742)
y: 56 -> 58 (0 -> 104)
y: 56 -> 58 (124 -> 742)
y: 58 -> 72 (0 -> 102)
y: 58 -> 72 (126 -> 742)
y: 72 -> 74 (0 -> 104)
y: 72 -> 74 (124 -> 742)
y: 74 -> 112 (0 -> 742)
y: 112 -> 854 (0 -> 1)
y: 112 -> 854 (741 -> 742)
y: 853 -> 854 (1 -> 401) ** band
y: 854 -> 855 (0 -> 742)
clientBounds-mPossiblyTransparentRegion:
y: 0 -> 25 (0 -> 742)
y: 25 -> 33 (0 -> 14)
y: 25 -> 33 (246 -> 742)
y: 33 -> 47 (0 -> 5)
y: 33 -> 47 (254 -> 742)
y: 47 -> 56 (0 -> 742)
y: 56 -> 58 (0 -> 104)
y: 56 -> 58 (124 -> 742)
y: 58 -> 72 (0 -> 102)
y: 58 -> 72 (126 -> 742)
y: 72 -> 74 (0 -> 104)
y: 72 -> 74 (124 -> 742)
y: 74 -> 112 (0 -> 742)
y: 112 -> 854 (0 -> 1)
y: 112 -> 854 (741 -> 742)
y: 853 -> 854 (1 -> 401) ** band
y: 854 -> 855 (0 -> 742)
opaqueRegion:
y: 25 -> 33 (14 -> 246)
y: 33 -> 47 (5 -> 254)
y: 56 -> 58 (104 -> 124)
y: 58 -> 72 (102 -> 126)
y: 72 -> 74 (104 -> 124)
y: 112 -> 853 (1 -> 741) ** GetLargestRectangle stop
y: 853 -> 854 (401 -> 741) ** ignored
largest: x=1 h=112 width=741 height=853 ** height
margins: left:1 top:112 right:1 bottom:2

I can dig a bit deeper to see why aDirtyRegion has that funny region in it. (You might be able to answer that quicker than I can though!)

Comment 24

[Jim Mathies [:jimm]]

My guess would be that this is by design since we don't handle painting that area of content, and therefore don't include it in dirty rect?

Comment 25

[Jim Mathies [:jimm]]

Different subject from the bands - I don't think we need eTransparencyBorderlessGlass/kGlassMarginAdjustment adjustments anymore. From reading bug 554982 it looks like it was added to remove some default borders. But I've tweaked the code to remove this adjustment, and tweaked css to remove the partially transparent border we set around the content area, and I get a clean transition from glass to content. Margins end up being zero. I'm wondering if this is fallout from but 130078, maybe we were compensating for the child window that used to hold content.

This may explain why we've had such a hard time getting these border to match the toolbar border color, and it also would explail why with a persona, you can see the persona texture in the content border. We are currently setting a semi transparent border around content, so personas ends up showing through, and glass gets extended over it.

Comment 26

[Jim Mathies [:jimm]]

(In reply to comment #25)
> Different subject from the bands - I don't think we need
> eTransparencyBorderlessGlass/kGlassMarginAdjustment adjustments anymore. From
> reading bug 554982 it looks like it was added to remove some default borders.
> But I've tweaked the code to remove this adjustment, and tweaked css to remove
> the partially transparent border we set around the content area, and I get a
> clean transition from glass to content. Margins end up being zero. I'm
> wondering if this is fallout from but 130078, maybe we were compensating for
> the child window that used to hold content.
> 
> This may explain why we've had such a hard time getting these border to match
> the toolbar border color, and it also would explail why with a persona, you can
> see the persona texture in the content border. We are currently setting a semi
> transparent border around content, so personas ends up showing through, and
> glass gets extended over it.

Ah, never mind. It's back after some additional tweaking. It's actually two pixels wide and is made up of a white and black line. We coud change kGlassMarginAdjustment to 1, and remove the black line, but keep the outer white 'haze' border fwiw.

Comment 27

[Timothy Nikkel (:tnikkel)]

The dirty region we pass to UpdatePossiblyTransparentRegion is probably wrong, I don't think it causes any problems though (or it didn't look like it when I noticed that). See bug 604221. If you're looking at the dirty region though you might want to use that patch so it isn't confusing.

Comment 28

[Jim Mathies [:jimm]]

(In reply to comment #27)
> The dirty region we pass to UpdatePossiblyTransparentRegion is probably wrong,
> I don't think it causes any problems though (or it didn't look like it when I
> noticed that). See bug 604221. If you're looking at the dirty region though you
> might want to use that patch so it isn't confusing.

Thanks, tn, I'll take that for a spin.

I'll be looking at the generation of dirty region data w/plugins more today.

Comment 29

[Jim Mathies [:jimm]]

Here's another interesting anomaly:

mPossiblyTransparentRegion clipped to clientBounds:
y: 0 -> 25 (0 -> 1081)
y: 25 -> 33 (0 -> 14)
y: 25 -> 33 (246 -> 1081)
y: 33 -> 47 (0 -> 5)
y: 33 -> 47 (254 -> 1081)
y: 47 -> 56 (0 -> 1081)
y: 56 -> 58 (0 -> 104)
y: 56 -> 58 (124 -> 1081)
y: 58 -> 72 (0 -> 102)
y: 58 -> 72 (126 -> 1081)
y: 72 -> 74 (0 -> 104)
y: 72 -> 74 (124 -> 1081)
y: 74 -> 112 (0 -> 1081)
y: 112 -> 206 (0 -> 1)
y: 112 -> 206 (1080 -> 1081)
aDirtyRegion:
y: 0 -> 206 (0 -> 1081)
mPossiblyTransparentRegion-aDirtyRegion: empty
mPossiblyTransparentRegion|aPossiblyTransparentRegion:
y: 0 -> 25 (0 -> 1081)
y: 25 -> 33 (0 -> 14)
y: 25 -> 33 (246 -> 264)
y: 25 -> 33 (277 -> 1081)
y: 33 -> 47 (0 -> 5)
y: 33 -> 47 (254 -> 255)
y: 33 -> 47 (285 -> 1081)
y: 47 -> 56 (0 -> 1081)
y: 56 -> 58 (0 -> 104)
y: 56 -> 58 (124 -> 1081)
y: 58 -> 72 (0 -> 102)
y: 58 -> 72 (126 -> 1081)
y: 72 -> 74 (0 -> 104)
y: 72 -> 74 (124 -> 1081)
y: 74 -> 206 (0 -> 1081)
clientBounds-mPossiblyTransparentRegion: (= opaque region)
y: 25 -> 33 (14 -> 246)
y: 25 -> 33 (264 -> 277)
y: 33 -> 47 (5 -> 254)
y: 33 -> 47 (255 -> 285)
y: 56 -> 58 (104 -> 124)
y: 58 -> 72 (102 -> 126)
y: 72 -> 74 (104 -> 124)
plugin bounds: x=0 h=0 width=0 height=0
opaqueRegion:
y: 25 -> 33 (14 -> 246)
y: 25 -> 33 (264 -> 277)
y: 33 -> 47 (5 -> 254)
y: 33 -> 47 (255 -> 285)
y: 56 -> 58 (104 -> 124)
y: 58 -> 72 (102 -> 126)
y: 72 -> 74 (104 -> 124)
largest: x=14 h=25 width=246 height=47
margins: left:2 top:2 right:2 bottom:2

When the window is sized up far enough (height=206 here), aPossiblyTransparentRegion contains the entire content area:

y: 74 -> 206 (0 -> 1081)

vs strips on the sides for the border:

y: 112 -> 206 (0 -> 1)
y: 112 -> 206 (1080 -> 1081)  

That can throw the calculation off as well and turn the upper glass area black.

Comment 30

[Jim Mathies [:jimm]]

(In reply to comment #29)
> When the window is sized up far enough (height=206 here),
> aPossiblyTransparentRegion contains the entire content area:
> 
> y: 74 -> 206 (0 -> 1081)
> 
> vs strips on the sides for the border:
> 
> y: 112 -> 206 (0 -> 1)
> y: 112 -> 206 (1080 -> 1081)  
> 
> That can throw the calculation off as well and turn the upper glass area black.

This is one of the causes of our 'messed up text in flash' bugs. If you extend the window up about 200px and resize down, glass margins get set to -1, extending glass over the entire content area. You can see this even better in a pdf. Plus with the banding issue, once it's set it tends to stay set over content.

Comment 31

[Bill Gianopoulos [:WG9s]]

For some positive news, the current patch here, besides fixing the Adobe plugin issue, also fixes the Sametime Java Applet issue that I mentioned in comment 14, which was NOT fixed by the first patch you had posted.

Comment 32

[Timothy Nikkel (:tnikkel)]

(In reply to comment #29)
> When the window is sized up far enough (height=206 here),
> aPossiblyTransparentRegion contains the entire content area:

We have a heuristic that allows the opaque region to get more complex if it increases the area by half or more. I think that is what you are seeing here. When the content area gets small enough it no longer increases the area of the opaque region by more than half so it doesn't get added.

Comment 33

[Jim Mathies [:jimm]]

(In reply to comment #32)
> (In reply to comment #29)
> > When the window is sized up far enough (height=206 here),
> > aPossiblyTransparentRegion contains the entire content area:
> 
> We have a heuristic that allows the opaque region to get more complex if it
> increases the area by half or more. I think that is what you are seeing here.
> When the content area gets small enough it no longer increases the area of the
> opaque region by more than half so it doesn't get added.

That sounds like what I've been digging for in the code, can you point me to it on mxr?

If this is the cause, is this something we can tweak or should I be looking for work arounds down in widget?

Comment 34

[Jim Mathies [:jimm]]

..or potentially some sort of a change in layout that hands different data down to widget via UpdatePossiblyTransparentRegion.

Comment 35

[Timothy Nikkel (:tnikkel)]

(In reply to comment #33)
> That sounds like what I've been digging for in the code, can you point me to it
> on mxr?

Sure thing, in nsDisplayListBuilder::SubtractFromVisibleRegion
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#230
 
> If this is the cause, is this something we can tweak or should I be looking for
> work arounds down in widget?

We can certainly make changes to that or other parts of layout so widget doesn't have to work around it.

Comment 36

[Jim Mathies [:jimm]]

Attachment: don't simplify plugins out of visible region v.1

Comment 37

[Jim Mathies [:jimm]]

Attachment: fixup UpdatePossiblyTransparentRegion v.1

Comment 38

[Jim Mathies [:jimm]]

Attachment: fixup kGlassMarginAdjustment v.1

Comment 39

[Jim Mathies [:jimm]]

I need to do some testing but I think this pretty much nails down all the problems.

tn, mind taking a look at part 1? I needed to disable simplification on everything within the list that held the plugin since a TYPE_CANVAS_BACKGROUND ended up simplifying out TYPE_PLUGIN while the elements were being processed. 

-> [6] empty:0 type:TYPE_PLUGIN
y: 112 -> 196 (1 -> 1227)
<- [6] empty:0 type:TYPE_PLUGIN
y: 112 -> 196 (401 -> 1227)

-> [6] empty:0 type:TYPE_BACKGROUND
<- [6] empty:0 type:TYPE_BACKGROUND

-> [6] empty:0 type:TYPE_CANVAS_BACKGROUND
y: 112 -> 196 (401 -> 1227)
<- [6] empty:1 type:TYPE_CANVAS_BACKGROUND


Seems like the right approach, but maybe there's some better, less intrusive way to do this.

Comment 40

[Timothy Nikkel (:tnikkel)]

(In reply to comment #39)
> tn, mind taking a look at part 1? I needed to disable simplification on
> everything within the list that held the plugin since a TYPE_CANVAS_BACKGROUND
> ended up simplifying out TYPE_PLUGIN while the elements were being processed. 

Because you were testing a full page plugin? A full page opaque plugin would make the visible region for the canvas background empty. So that makes sense.

This is a good patch, I'll think if there is a nicer way to do it.

I think we might want to include canvas backgrounds too: the 'removes more than half the visible region' heuristic was really to make sure we include the root content document as an opaque area. I can take the patch from here if you'd like Jim.

Comment 41

[Jim Mathies [:jimm]]

(In reply to comment #40)
> (In reply to comment #39)
> > tn, mind taking a look at part 1? I needed to disable simplification on
> > everything within the list that held the plugin since a TYPE_CANVAS_BACKGROUND
> > ended up simplifying out TYPE_PLUGIN while the elements were being processed. 
> 
> Because you were testing a full page plugin? A full page opaque plugin would
> make the visible region for the canvas background empty. So that makes sense.

Actually I was testing with the test case here, where the plugin was just in the first 400px of the left side. The plugin left 112 -> 196 (401 -> 1227) in visibleRegion and the processing of the background removed it.

> This is a good patch, I'll think if there is a nicer way to do it.
> 
> I think we might want to include canvas backgrounds too: the 'removes more than
> half the visible region' heuristic was really to make sure we include the root
> content document as an opaque area. I can take the patch from here if you'd
> like Jim.

Love the help!

Comment 42

[Jim Mathies [:jimm]]

Note there's still some wonkiness with the white glaze on the window border. Various bugs have been filed and fixed on this (bug 623463, bug 622733, bug 623092). These patches improve things a bit more but don't address the issue 100%. I'll file a follow up on this.

Comment 43

[Timothy Nikkel (:tnikkel)]

Oh, I see what you did. You set disableSimplification to true and leave it there if you encounter a plugin? That was on purpose? We only ever prevent the visible region from getting more complex, we don't simplify it ever. So we can just allow the visible region to get more complex if we have the canvas background for the root content document. I'll produce a patch for that.

Comment 44

[Jim Mathies [:jimm]]

(In reply to comment #43)
> Oh, I see what you did. You set disableSimplification to true and leave it
> there if you encounter a plugin? That was on purpose? We only ever prevent the
> visible region from getting more complex, we don't simplify it ever. So we can
> just allow the visible region to get more complex if we have the canvas
> background for the root content document. I'll produce a patch for that.

Yes, that way the value fell through to the background canvas. That was one thing I wondered about, I wasn't sure if the canvas would always be below the plugin in the element list.

Comment 45

[Timothy Nikkel (:tnikkel)]

Attachment: always make sure the area of the root content document is considered opaque

The root content document should contain all plugins, and we want it to be opaque anyway.

Let me know how this works for you Jim.

Comment 46

[Jim Mathies [:jimm]]

(In reply to comment #45)
> Created attachment 506087 [details] [diff] [review]
> always make sure the area of the root content document is consider opaque
> 
> The root content document should contain all plugins, and we want it to be
> opaque anyway.
> 
> Let me know how this works for you Jim.

Works great!

Comment 47

[Jim Mathies [:jimm]]

Attachment: fixup kGlassMarginAdjustment v.2

Simpilifed the logic in this a bit.

Comment 48

[Jim Mathies [:jimm]]

Pushed to try for a full set of test runs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2fe587592b16

Comment 49

[Bill Gianopoulos [:WG9s]]

I also have build available at http://www/wg9s.com/mozilla/firefox/ available that include these patches.

Comment 50

[Bill Gianopoulos [:WG9s]]

(In reply to comment #49)
> I also have build available at http://www/wg9s.com/mozilla/firefox/ available
> that include these patches.

HMm http://www.wg9s.com/mozilla/firefox/

Comment 51

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

Comment on attachment 505857 [details] [diff] [review]
fixup UpdatePossiblyTransparentRegion v.1

Let's just remove aDirtyRegion altogether and set mPossiblyTransparentRegion to aPossiblyTransparentRegion. aPossiblyTransparentRegion is always fully up-to-date now.

Comment 52

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

(In reply to comment #30)
> This is one of the causes of our 'messed up text in flash' bugs. If you extend
> the window up about 200px and resize down, glass margins get set to -1,
> extending glass over the entire content area. You can see this even better in a
> pdf. Plus with the banding issue, once it's set it tends to stay set over
> content.

Oh, I bet I regressed this in bug 623463 :-(.

    nsIntRect largest = opaqueRegion.GetLargestRectangle(pluginBounds);
    if (largest.x <= MAX_HORIZONTAL_GLASS_MARGIN &&
        clientBounds.width - largest.XMost() <= MAX_HORIZONTAL_GLASS_MARGIN &&
        largest.height >= MIN_OPAQUE_RECT_HEIGHT_FOR_GLASS_MARGINS) {

This 'if' should include a big "|| !pluginBounds.IsEmpty()" around the outside, so that if there are plugins we use glass margins no matter what. Better to have strange-looking glass margin effects than the plugin being composited badly by DWM, right?

Comment 53

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

+static bool
+IsRootContentDocumentBackground(nsDisplayItem* aItem)
+{
+  nsIFrame* f = aItem->GetUnderlyingFrame();
+  if (!f)
+    return false;
+  if (aItem->GetType() == nsDisplayItem::TYPE_CANVAS_BACKGROUND &&
+      f->PresContext()->IsRootContentDocument()) {
+    return true;
+  }
+  if (aItem->GetType() == nsDisplayItem::TYPE_SOLID_COLOR) {
+    nsIAtom* type = f->GetType();
+    if (type == nsGkAtoms::viewportFrame &&
+        f->PresContext()->IsRootContentDocument()) {
+      return true;
+    }
+    if (type == nsGkAtoms::subDocumentFrame && f->PresContext()->IsChrome())
+      return true;
+  }
+  return false;
+}

I'm not excited about this hard-to-maintain function.

Maybe we should pass the nsDisplayItem to SubtractFromVisibleRegion. Then, we can do a last-ditch check of the display item only if we were about to bail without subtracting from the visible region. How about adding a boolean field to nsDisplaySolidColor that records if it's the background for a root content document (or else just the canvas background for a subdocument whose parent is a chrome document?)

Also, make sure we only call aItem->GetType() once.

Comment 54

[Timothy Nikkel (:tnikkel)]

We can probably also skip all this code is IsPaintingToWindow is false.

Comment 55

[Jim Mathies [:jimm]]

(In reply to comment #52)
> (In reply to comment #30)
> This 'if' should include a big "|| !pluginBounds.IsEmpty()" around the outside,
> so that if there are plugins we use glass margins no matter what. Better to
> have strange-looking glass margin effects than the plugin being composited
> badly by DWM, right?

The 'visiblePlugin' flag I've added takes care of this. The only difference is it only triggers if a plugin has a visible region vs. the existence of a windowed plugin somewhere in the parent window. I think that's a bit better since we want MIN_OPAQUE_RECT_HEIGHT_FOR_GLASS_MARGINS to apply as often as possible when the content area is really small.

Comment 56

[Jim Mathies [:jimm]]

Attachment: fixup UpdatePossiblyTransparentRegion v.2

Removed dirty region plus a little cleanup.

Comment 57

[Jim Mathies [:jimm]]

Anyone mind if I change 'UpdatePossiblyTransparentRegion' to 'UpdateTransparentRegion' and 'aPossiblyTransparentRegion' to 'aTransparentRegion'?

Comment 58

[Timothy Nikkel (:tnikkel)]

Attachment: always make sure the area of the root content document is considered opaque v2

Comment 59

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

(In reply to comment #57)
> Anyone mind if I change 'UpdatePossiblyTransparentRegion' to
> 'UpdateTransparentRegion' and 'aPossiblyTransparentRegion' to
> 'aTransparentRegion'?

Make it so!

Comment 60

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

+          // aDirtyRegion may be missing parts of windowed plugins which
+          // can mess up GetLargestRectangle's results. Avoid this by adding
+          // the visible plugin area to opaqueRegion.

This comment no longer makes sense since aDirtyRegion doesn't exist. How much of this code is still necessary?

        // XXX we should simplify this API now that dirtyWindowRegion always
       // covers the entire window

You can delete this comment.

       nsIntRegion dirtyWindowRegion(aDirtyRegion.ToOutsidePixels(pixelRatio));

This can be removed.

Comment 61

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

+                                 nsDisplayItem* aItem = nsnull);

Can we have this always be valid please?

   PRBool  mTransparentBackground;
+  PRBool  mIsRootContentDocBackground;

PRPackedBool

Otherwise looks good.

Comment 62

[Timothy Nikkel (:tnikkel)]

(In reply to comment #61)
> +                                 nsDisplayItem* aItem = nsnull);
> 
> Can we have this always be valid please?

I did that so we can skip this code when we don't need it, for example when recomputing visibility in DrawThebesLayer.

Comment 63

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

Isn't it harmless in that context?

Comment 64

[Timothy Nikkel (:tnikkel)]

Attachment: always make sure the area of the root content document is considered opaque v3

Comment 65

[Timothy Nikkel (:tnikkel)]

Attachment: interdiff

Should make it easy to review.

Comment 66

[Jim Mathies [:jimm]]

(In reply to comment #60)
> +          // aDirtyRegion may be missing parts of windowed plugins which
> +          // can mess up GetLargestRectangle's results. Avoid this by adding
> +          // the visible plugin area to opaqueRegion.
> 
> This comment no longer makes sense since aDirtyRegion doesn't exist. How much
> of this code is still necessary?

I think we still need to add it, will confirm.

> 
>         // XXX we should simplify this API now that dirtyWindowRegion always
>        // covers the entire window
> 
> You can delete this comment.
> 
>        nsIntRegion dirtyWindowRegion(aDirtyRegion.ToOutsidePixels(pixelRatio));
> 
> This can be removed.

Sorry, through that together too quickly.

Comment 67

[Jim Mathies [:jimm]]

(In reply to comment #66)
> Sorry, through that together too quickly.

Threw that post together to quickly as well. ;)

Comment 68

[Jim Mathies [:jimm]]

Something isn't right here, with these applied the plugin is ending up back in the transparent region handed into UpdateTransparentRegion. I'm not seeing hits on either of the two true returns in IsRootContentDocumentBackground either, which seems odd.

(Also, minor nit, it'd probably be better to do the null check on aItem in IsRootContentDocumentBackground vs. down in SubtractFromVisibleRegion before we hand it in.)

Comment 69

[Timothy Nikkel (:tnikkel)]

I'll look into it.

Comment 70

[Jim Mathies [:jimm]]

Looks like the calculations for TYPE_CANVAS_BACKGROUND are correct though. In SubtractFromVisibleRegion I get 

aRegion
y: 112 -> 912 (1 -> 793)
aVisibleRegion
y: 112 -> 187 (401 -> 793) (content minus plugin)

'tmp' is empty after the subtraction and is stored in aVisibleRegion. Yet in widget the transparent region has:

y: 74 -> 188 (0 -> 811)

which is the whole content area. This is with the window sized up so that it's about 200px tall and 800px wide and the plugin test case here is loaded.

Comment 71

[Timothy Nikkel (:tnikkel)]

Are we passing the right region to UpdatePossiblyTransparentRegion?

Comment 72

[Jim Mathies [:jimm]]

(In reply to comment #71)
> Are we passing the right region to UpdatePossiblyTransparentRegion?

No, somewhere in here we're adding content back in, but the calculation for TYPE_CANVAS_BACKGROUND are correct:

SubtractFromVisibleRegion -> TYPE_CANVAS_BACKGROUND
aRegion
y: 112 -> 912 (1 -> 627)
aVisibleRegion
y: 112 -> 205 (401 -> 627)
tmp empty:1
returning tmp
SubtractFromVisibleRegion <- TYPE_CANVAS_BACKGROUND

This is right, tmp is empty, and we set *aVisibleRegion to tmp on exit.

But in Update(Possibly)TransparentRegion I get:

client bounds: x=0 y=0 width=645 height=206
aTransparentRegion:
y: 0 -> 25 (0 -> 645)
y: 25 -> 33 (0 -> 14)
y: 25 -> 33 (246 -> 264)
y: 25 -> 33 (277 -> 645)
y: 33 -> 47 (0 -> 5)
y: 33 -> 47 (254 -> 255)
y: 33 -> 47 (285 -> 645)
y: 47 -> 56 (0 -> 645)
y: 56 -> 58 (0 -> 104)
y: 56 -> 58 (124 -> 645)
y: 58 -> 72 (0 -> 102)
y: 58 -> 72 (126 -> 645)
y: 72 -> 74 (0 -> 104)
y: 72 -> 74 (124 -> 645)
y: 74 -> 206 (0 -> 645) * content

Comment 73

[Timothy Nikkel (:tnikkel)]

I'm guessing the problem is that background for the root content document is inside a clip and so we go through nsDisplayClip::ComputeVisibility and the background gets removed from the temp visible region in that function and then when we try to subtract based on that temp visible region SubtractFromVisibleRegion refuses to make the visible region that complex because it has no idea it contains the root content document background.

So perhaps we should just record the root content background bounds on the display list builder when we encounter them, and add it to the region after computing visibility in nsLayoutUtils::PaintFrame. Unless you have a better idea roc?

Comment 74

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

That sounds good. We'll have to check that the root content background bounds are not in fact clipped out or in a transform etc.

Comment 75

[Jim Mathies [:jimm]]

(In reply to comment #73)
> I'm guessing the problem is that background for the root content document is
> inside a clip and so we go through nsDisplayClip::ComputeVisibility and the
> background gets removed from the temp visible region in that function and then
> when we try to subtract based on that temp visible region
> SubtractFromVisibleRegion refuses to make the visible region that complex
> because it has no idea it contains the root content document background.

Yes!

> So perhaps we should just record the root content background bounds on the
> display list builder when we encounter them, and add it to the region after
> computing visibility in nsLayoutUtils::PaintFrame. Unless you have a better
> idea roc?

Comment 76

[Timothy Nikkel (:tnikkel)]

Hmm, zoom items and opacity items are going to make that more complex. I'm not sure how we should compensate for them.

Comment 77

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

Make IsRootContentDocumentBackground into ContainsRootContentDocumentBackground and have it dig through the children of a wrapped list? You probably only need to look at the first item on the child list.

Comment 78

[Timothy Nikkel (:tnikkel)]

Attachment: always make sure the area of the root content document is considered opaque v4

Tell me how much you hate this.

Comment 79

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

Comment on attachment 507248 [details] [diff] [review]
always make sure the area of the root content document is considered opaque v4

+    if (retval && mFrame->PresContext()->IsRootContentDocument())
+      aContainsRootContentDocBG = PR_TRUE;

{}

+      if (containsRootContentDocBG)
+        aContainsRootContentDocBG = PR_TRUE;

{}

+  if (retval && IsRootContentDocBackground())
+    aContainsRootContentDocBG = PR_TRUE;

{}

I don't hate it!

Comment 81

[Timothy Nikkel (:tnikkel)]

Jim, this latest version of the patch does what you need?

Comment 82

[Timothy Nikkel (:tnikkel)]

Attachment: always make sure the area of the root content document is considered opaque v5

Fixed the review comments.

Comment 83

[Bill Gianopoulos [:WG9s]]

I just tested a build with the v4 version (the one without the braces fix).  It seems to work fine for the original testcase pdf document.  I have a Lotus sametime meeting scheduled for an hour form now, so I can see if it still fixes that as well.  I had previously been running using your version 2 patch because the version 3 patch fixed neither issue.

Comment 84

[Timothy Nikkel (:tnikkel)]

v2 worked fine but v3 did not? That is surprising as the interdiff between them (https://bug622328.bugzilla.mozilla.org/attachment.cgi?id=506570) does not contain anything that should change it from working to not working. I really would like to know what caused v3 to fail where v2 worked.

Comment 85

[Bill Gianopoulos [:WG9s]]

(In reply to comment #84)
> v2 worked fine but v3 did not? That is surprising as the interdiff between them
> (https://bug622328.bugzilla.mozilla.org/attachment.cgi?id=506570) does not
> contain anything that should change it from working to not working. I really
> would like to know what caused v3 to fail where v2 worked.

I Might be confused.  Looking at the interdiff I don't see that much difference between the 2 either.  I can try another build with the same 3 patches I guess and see if I still see the issue.  Perhaps I screwed something up on that build somehow.  Since the new patch works for me I guess it is only a curiosity at this point though.

Comment 86

[Timothy Nikkel (:tnikkel)]

Although I hate the latest patch and would prefer any of the older patches.

Comment 87

[Bill Gianopoulos [:WG9s]]

(In reply to comment #86)
> Although I hate the latest patch and would prefer any of the older patches.

OK.  I will do build tomorrow on my windows 7 system so that i can build and test on the same system, whic8h is simpler than the way I usually do it.  and I will test all versions of your patch and post results as to which work and which do not for me.

Comment 88

[Jim Mathies [:jimm]]

Attachment: fixup UpdatePossiblyTransparentRegion v.3

This is working great here with all the latest patches.

roc, I kept visiblePlugin vs. the test on pluginBounds. The latter had a problem with the glass haze when the content area was obscured by sizing the bottom window frame all the way up.

Comment 89

[Jim Mathies [:jimm]]

(In reply to comment #88)
> Created attachment 507575 [details] [diff] [review]
> fixup UpdatePossiblyTransparentRegion v.3
> 
> This is working great here with all the latest patches.
> 
> roc, I kept visiblePlugin vs. the test on pluginBounds. The latter had a
> problem with the glass haze when the content area was obscured by sizing the
> bottom window frame all the way up.

I just noticed we don't need the

if (mTransparencyMode == eTransparencyBorderlessGlass) {
..
}

check anymore. I'll remove that from the final patch.

Comment 90

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/c1124d3fb216
http://hg.mozilla.org/mozilla-central/rev/217162ad801a
http://hg.mozilla.org/mozilla-central/rev/c165b3127975

Comment 91

[Bill Gianopoulos [:WG9s]]

(In reply to comment #87)
> (In reply to comment #86)
> > Although I hate the latest patch and would prefer any of the older patches.
> 
> OK.  I will do build tomorrow on my windows 7 system so that i can build and
> test on the same system, whic8h is simpler than the way I usually do it.  and I
> will test all versions of your patch and post results as to which work and
> which do not for me.

I managed to find time to retest the version 3 patch under Windows 7 and was unable to reproduce the issue I thought I had seen earlier.  I suspect I screwed up the previous testing somehow.

Comment 92

[Timothy Nikkel (:tnikkel)]

Ok, thanks for retesting. That just means it wasn't something simple that changed. The problem fixed by v4 is more than theoretical so I think we'll still have to go that way.

Comment 93

[Valerio]

it's possible that after the land of this patch the border around the content area is again opaque in win 7?

Comment 94

[Jim Mathies [:jimm]]

(In reply to comment #93)
> it's possible that after the land of this patch the border around the content
> area is again opaque in win 7?

Can you screen shot what you're seeing? I'm on win7 w/glass, and the window frame looks fine in this morning's nightly.

Comment 95

[Valerio]

Attachment: opaque border around content area

as you can see in this screenshot the border left of the toolbar is clearly transparent and the bottom border of the toolbar is by design opaque but the border around the content area should be transparent as the border left and right of the toolbar (until yesterday it was so)

Comment 97

[Ben]

Verified ok on Windows 7 with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8) Gecko/20100101 Firefox/4.0b8 ID:20101214170338

Comment 98

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I think I'm seeing this bug in Firefox 16.0 (Gecko/20100101) on Win7. Possible regression?

Comment 99

[u279076]

Chris, can you please file a new bug and be sure to include your steps to reproduce and indicate the exact plugin version you have installed? Thank you.