555182 - Opening up a sidebar distorts text in sidebar and sometimes on the whole page.

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P1)

Description

[Gary [:streetwolf52]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100326 Minefield/3.7a4pre Firefox/3.6
Build Identifier: 20100326040105

http://img338.imageshack.us/i/68399803.png

Reproducible: Always

Steps to Reproduce:
1. Open up a sidebar
2.
3.
Actual Results:  
Text is a very light blue.



Expected Results:  
Text should be normal.

Screen Shot:  http://img338.imageshack.us/i/68399803.png

Comment 1

[Gary [:streetwolf52]]

http://img338.imageshack.us/i/68399803.png/

Comment 2

[Gary [:streetwolf52]]

d2d/dw has to be disabled for this to happen.  Enabled it works ok.

Comment 3

[Gary [:streetwolf52]]

Not necessarily true Comment 2.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Confirmed for Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a4pre) Gecko/20100326 Minefield/3.7a4pre

Tested with default settings of a new profile. It's not blue here but misty.

Comment 5

[Ria Klaassen (not reading all bugmail)]

Nearly right, it's Rob Arnold :)

Comment 6

[Dão Gottwald [:dao]]

Looks like this is new in the current nightly, so I guess it's a regression from bug 458407.

Comment 7

[Rob Arnold [:robarnold]]

I took a look at this. There are four opaque regions that all neighbor each
other - the sidebar header, the sidebar content area, the tab content area, and
the status bar. At some magical point which I don't know how to trigger
reliably yet (but can be done with only mouse clicks and movement), we start
getting one opaque region and then it all works.

I also found that if I start the browser up with the sidebar open, everything
works.

My 4 regions in my testcase are:
{ x, y, width, height }
{ 0, 52, 216, 25 }
{ 0, 77, 216, 634 }
{ 216, 52, 823, 659 }
{ 0, 711, 823, 22 }

The consolidated region is
{ 0, 52, 823, 681 }

We're not clipping the child window regions to the client area and this is
causing us to compute a negative margin which tells the DWM to make the entire
area glass.

Comment 8

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

Basic problem here is probably that nsRegion doesn't consolidate its internal rectangles, or does so unpredictably. So we need a smarter way to find the largest rect that fits in the region.

But also, why would things go wrong just because the window is all glass? Seems to me everything should still work in that case, just more slowly.

Comment 9

[Rob Arnold [:robarnold]]

(In reply to comment #8)
> Basic problem here is probably that nsRegion doesn't consolidate its internal
> rectangles, or does so unpredictably. So we need a smarter way to find the
> largest rect that fits in the region.

Why can't we just clip to the client area of the toplevel widget?

> 
> But also, why would things go wrong just because the window is all glass? Seems
> to me everything should still work in that case, just more slowly.

The sidebar is painted in a child native widget as is the tab content. Since the content is blended with any glass under it, so would the sidebar. I think we are not painting with alpha for child windows because they are intended to be opaque and that means they have zeros in their surface's alpha channel since we're using GDI.

Comment 10

[Rob Arnold [:robarnold]]

(In reply to comment #9)
> I think
> we are not painting with alpha for child windows because they are intended to
> be opaque and that means they have zeros in their surface's alpha channel since
> we're using GDI.

So if we paint with alpha by checking the top level widget for glass instead of the widget-to-be-painted, it appears that we correctly draw everything. I swear I tried this a while ago and it didn't work so I'm pleasantly surprised it does now. There is a noticeable performance hit when moving around the fully-glassed window so we still need to fix this.

Comment 11

[Rob Arnold [:robarnold]]

(In reply to comment #8)
> Basic problem here is probably that nsRegion doesn't consolidate its internal
> rectangles, or does so unpredictably. So we need a smarter way to find the
> largest rect that fits in the region.

I think we could add a method to nsRegion to guarantee the largest rectangles possible. This might not be fast but I don't expect that we have a lot of rectangles either. We could also just improve nsRegion::Optimize but it's used in a lot of places that are already OK with its behavior and I don't want to regress performance.

Comment 12

[Wesley Johnston (:wesj)]

I've had problems when notification bars show up. Is this the same bug?

Comment 13

[Rob Arnold [:robarnold]]

Anything related to seeing glass blended with tab content/parts of chrome when you shouldn't is probably related to this bug. Side effects of this bug include a missing or improperly sized window border.

Comment 14

[bogas04]

How can we confirm now? Better make a about:config line to enable/disable aero so that we can confirm if the prob is fixed

Comment 15

[dpcnull]

Has glass been disabled due to this bug? I was using a month-old nightly with the glass effects until I updated to 3.7a5pre on 2010-04-09. Will glass ever be available again?

Comment 16

[Rob Arnold [:robarnold]]

(In reply to comment #15)
> Has glass been disabled due to this bug? I was using a month-old nightly with
> the glass effects until I updated to 3.7a5pre on 2010-04-09. Will glass ever be
> available again?

This was a big part of it. Fixing this bug will go a long way towards allowing glass to be re-enabled. I just need to find the time (or someone else does).

Comment 17

[ronin.achilles]

Has there been any progress on this? This is the core bug stopping the re-enabling of Aero Glass; and Aero being disabled means that no more user-testing is happening on it as well. Aero couldn't make it to alpha4... I hope we can get it into alpha5.

Comment 18

[Rob Arnold [:robarnold]]

(In reply to comment #17)
> Has there been any progress on this? This is the core bug stopping the
> re-enabling of Aero Glass; and Aero being disabled means that no more
> user-testing is happening on it as well. Aero couldn't make it to alpha4... I
> hope we can get it into alpha5.

Yes, some progress has been made. I have an algorithm that I believe will work to find the largest opaque rectangle in a reasonably efficient manner. Unfortunately I have not had time to devise an implementation.

Comment 19

[Rob Arnold [:robarnold]]

Attachment: Calculate the largest opaque region

This patch clips the opaqueRegion to the client rect because sometimes child windows extend outside of the client region for some period of time (most commonly when you open the sidebar). Uses a not-terribly-efficient-but-correct algorithm to calculate the largest opaque rectangle in the opaque region. Documentation is provided for those who are interested/future generations.

Comment 20

[Jim Mathies [:jimm]]

Would there be a way to work up a set of tests for GetLargestRectangle()? Windows bits look ok to me.

Comment 21

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

 Let S be a m-1 by
+// n-1 matrix where each element S[i][j] is either the largest rectangle whose
+// lower right corner is the lower right corner of G[i][j] if the portion of the
+// grid refers to part of the region or the constant NONE.

This would read better if you said "is either the constant NONE, or ..."

+// S[-1][j] = NONE
+// S[i][-1] = NONE

This is weird. What exactly do the indices of S range over?

+//                      max(rect(i',j',w',h'+h), rect(i'',j'',w''+w,h''))

What is the definition of max here? I assume it's the rectangle with max area?

There's some confusion in rect(). i and j are indices in the matrix, but w and h are coordinates in space? 

Actually I don't understand how this algorithm is correct. Suppose I have the grid

   *
   *
****
   *
   *

Then the matrix M is

 0   1x2
 3x1 1x1
 0   1x2

Then for S I get (writing 'rect' to use x,y,w,h coordinates)

 NONE          rect(3,0,1,2)
 rect(0,2,3,1) rect(0,2,4,1)
 NONE          R

with your algorithm as written, R = rect(0,2,4,3). That's clearly wrong.

If instead we change the algorithm to
    OR (rect(i',j',w',h'), NONE) => rect(i,j',w,h'+h)
we get R = rect(3,2,1,3), and we don't find the optimal solution.

Comment 22

[Rob Arnold [:robarnold]]

Attachment: Calculate the largest opaque rect

Now with a slower but hopefully correct algorithm and tests including the testcase mentioned in comment 21.

Comment 23

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

Comment on attachment 446778 [details] [diff] [review]
Calculate the largest opaque rect

Call MaxSum MaxSum1D to match your pseudocode. It needs documentation though.

AxisPartition needs documentation too.

+    void InsertPoint(nscoord p) {

InsertCoord? It's not a point.

+        PRInt64 &area = pareas[y*n+x] = areas[(y-1)*matrixWidth+x-1];

Making area a reference is a bit too tricky for my taste. I'd just use a normal local variable and store it at the end.

Comment 24

[Rob Arnold [:robarnold]]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b375e530a90b

Comment 25

[Rob Arnold [:robarnold]]

It seems my rapport with gcc has gone from unfriendly to hateful:
http://hg.mozilla.org/mozilla-central/rev/2d0ad835761c
http://hg.mozilla.org/mozilla-central/rev/cb0eb3105a01

Thanks to Michael Kohler for helping me patch things up quickly.

Comment 26

[Rob Arnold [:robarnold]]

Attachment: Net patch pushed to mozilla-central

Forgot to post the patch I initially pushed so here's that plus the followup patches.