Closed
Bug 555182
Opened 15 years ago
Closed 15 years ago
Opening up a sidebar distorts text in sidebar and sometimes on the whole page.
Categories
(Core :: Widget: Win32, defect, P1)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta1+ |
People
(Reporter: streetwolf52, Assigned: robarnold)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•15 years ago
|
||
Severity: normal → critical
Priority: -- → P1
Version: unspecified → Trunk
Reporter | ||
Comment 2•15 years ago
|
||
d2d/dw has to be disabled for this to happen. Enabled it works ok.
Comment 4•15 years ago
|
||
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
Updated•15 years ago
|
Blocks: 546259
Severity: critical → major
Component: Widget → Widget: Win32
QA Contact: general → win32
Comment 5•15 years ago
|
||
Nearly right, it's Rob Arnold :)
Comment 6•15 years ago
|
||
Looks like this is new in the current nightly, so I guess it's a regression from bug 458407.
Blocks: 458407
Keywords: regression
Assignee | ||
Comment 7•15 years ago
|
||
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.
Assignee | ||
Comment 9•15 years ago
|
||
(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.
Assignee | ||
Comment 10•15 years ago
|
||
(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.
Assignee | ||
Comment 11•15 years ago
|
||
(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•15 years ago
|
||
I've had problems when notification bars show up. Is this the same bug?
Assignee | ||
Comment 13•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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?
Assignee | ||
Comment 16•15 years ago
|
||
(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).
Updated•15 years ago
|
blocking2.0: --- → ?
Updated•15 years ago
|
blocking2.0: ? → beta1+
Comment 17•15 years ago
|
||
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.
Assignee | ||
Comment 18•15 years ago
|
||
(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.
Assignee | ||
Comment 19•15 years ago
|
||
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)
Comment 20•15 years ago
|
||
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.
Assignee | ||
Comment 22•15 years ago
|
||
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+
Assignee | ||
Comment 24•15 years ago
|
||
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b375e530a90b
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 25•15 years ago
|
||
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.
Assignee | ||
Comment 26•15 years ago
|
||
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.
Description
•