Closed Bug 555182 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: streetwolf, Assigned: robarnold)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
http://img338.imageshack.us/i/68399803.png/
Severity: normal → critical
Priority: -- → P1
Version: unspecified → Trunk
d2d/dw has to be disabled for this to happen.  Enabled it works ok.
Not necessarily true Comment 2.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 546259
Severity: critical → major
Component: Widget → Widget: Win32
QA Contact: general → win32
Nearly right, it's Rob Arnold :)
Looks like this is new in the current nightly, so I guess it's a regression from bug 458407.
Blocks: 458407
Keywords: regression
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.
Assignee: nobody → tellrob
Status: NEW → ASSIGNED
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.
(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.
(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.
Depends on: 555477
(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.
I've had problems when notification bars show up. Is this the same bug?
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.
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
Blocks: 556967
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?
(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).
blocking2.0: --- → ?
blocking2.0: ? → beta1+
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.
(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.
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.
Attachment #443039 - Flags: superreview?(roc)
Attachment #443039 - Flags: review?(jmathies)
Would there be a way to work up a set of tests for GetLargestRectangle()? Windows bits look ok to me.
 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.
Now with a slower but hopefully correct algorithm and tests including the testcase mentioned in comment 21.
Attachment #443039 - Attachment is obsolete: true
Attachment #446778 - Flags: review?(roc)
Attachment #443039 - Flags: superreview?(roc)
Attachment #443039 - Flags: review?(jmathies)
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.
Attachment #446778 - Flags: review?(roc) → review+
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b375e530a90b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
Forgot to post the patch I initially pushed so here's that plus the followup patches.
Attachment #446778 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.