Unified titlebar/toolbar sometimes paints incorrectly

RESOLVED FIXED in mozilla2.0b3

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
mozilla2.0b3
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

One way to reproduce this is to open a page containing an file input control and click on the "browse" button: 
data:text/html,<input type="file">

This is because the Cocoa code depends on us repainting toolbars in the chrome window whenever we repaint the top row of pixels in the chrome window. But with retained layers we don't necessarily do that, because we can have part of the window cached in a layer. We need to rework the Cocoa unified titlebar/toolbar code, probably with new API.
blocking2.0: --- → ?
For Beta 2 we can just back out bug 461291 - the most common way of collapsing the main browser window toolbar was via the toolbar collapse button in the titlebar which was removed by bug 573412.
Blocks: 461291
Wouldn't there still be a bug where hiding a toolbar using the "View" menu might not repaint the chrome correctly?
Sure, but doing that is not as common, I think. I'm not saying that we shouldn't fix this bug, I'm just saying that backing out bug 461291 might defuse this until we (or rather you) fix it properly.

Comment 4

8 years ago
(In reply to comment #1)
> For Beta 2 we can just back out bug 461291 - the most common way of collapsing
> the main browser window toolbar was via the toolbar collapse button in the
> titlebar which was removed by bug 573412.

It could actually be more common to hide/show the toolbar using the View menu. At least https://bugzilla.mozilla.org/show_bug.cgi?id=573412#c9 seems to indicate that.
blocking2.0: ? → final+

Updated

8 years ago
Duplicate of this bug: 580078
Should try to get this earlier than final as it's a visible and fugly regression, even if not one that prevents people from using their browser.
Created attachment 458913 [details] [diff] [review]
fix

Creates a new nsITheme interface, RegisterWidgetGeometry, that we can call to notify theme engines about UI items in particular locations, even if those UI items aren't drawn. Use that instead of relying on DrawWidgetBackground being called.

Note that with retained layers, whenever we paint in the chrome window we build a display list for all of it, so RegisterWidgetGeometry will be called on all toolbars.
Attachment #458913 - Flags: review?(mstange)
diff --git a/widget/src/cocoa/nsCocoaWindow.mm b/widget/src/cocoa/nsCocoaWindow.mm

+- (void)notifyToolbarAt:(float)aY height:(float)aHeight
 {

+  if (aY <= mUnifiedToolbarHeight &&
+      aY + aHeight > mUnifiedToolbarHeight) {
+    mUnifiedToolbarHeight = aY + aHeight;
+  }

Why aY <= mUnifiedToolbarHeight? I would have expected something like aY <= 0, or even aY == 0.
Similarly, here:

diff --git a/widget/src/cocoa/nsNativeThemeCocoa.mm b/widget/src/cocoa/nsNativeThemeCocoa.mm

+static PRBool
 ToolbarCanBeUnified(CGContextRef cgContext, const HIRect& inBoxRect, NSWindow* aWindow)
 {

+  return inBoxRect.origin.x == 0 &&
+         inBoxRect.size.width == [aWindow frame].size.width &&
+         inBoxRect.origin.y + inBoxRect.size.height <= unifiedToolbarHeight;
 }
 
This needs inBoxRect.origin.y <= 0 - we only want to unify toolbars that connect to the title bar.

Looks great otherwise.
(In reply to comment #8)
> diff --git a/widget/src/cocoa/nsCocoaWindow.mm
> b/widget/src/cocoa/nsCocoaWindow.mm
> 
> +- (void)notifyToolbarAt:(float)aY height:(float)aHeight
>  {
> 
> +  if (aY <= mUnifiedToolbarHeight &&
> +      aY + aHeight > mUnifiedToolbarHeight) {
> +    mUnifiedToolbarHeight = aY + aHeight;
> +  }
> 
> Why aY <= mUnifiedToolbarHeight? I would have expected something like aY <= 0,
> or even aY == 0.

The first toolbar in the window starts at y=-1, for me anyway, so I can't use aY == 0.

The point here is that there might be multiple toolbars, say one from -1 to 19, another one from 19 to 40, etc. This code will build one large unified toolbar as long as the toolbars' z-order matches their y-order and they are vertically adjacent or overlapping.

Once we've computed mUnifiedToolbarHeight during display list construction, during painting ToolbarCanBeUnified will return true for any toolbar that is contained in the unified toolbar.
(In reply to comment #10)
> The first toolbar in the window starts at y=-1, for me anyway, so I can't use
> aY == 0.

OK, aY <= 0 is good.
Are you testing with tabs on top?

> The point here is that there might be multiple toolbars, say one from -1 to 19,
> another one from 19 to 40, etc. This code will build one large unified toolbar
> as long as the toolbars' z-order matches their y-order and they are vertically
> adjacent or overlapping.
> 
> Once we've computed mUnifiedToolbarHeight during display list construction,
> during painting ToolbarCanBeUnified will return true for any toolbar that is
> contained in the unified toolbar.

Ah, I see. Great idea, but I don't think it's what we want here :)
Let's keep the current behaviour.
(In reply to comment #11)
> (In reply to comment #10)
> > The first toolbar in the window starts at y=-1, for me anyway, so I can't use
> > aY == 0.
> 
> OK, aY <= 0 is good.
> Are you testing with tabs on top?

Yes, I've tested both.
Attachment #459366 - Flags: review? → review?(mstange)
Attachment #458913 - Attachment is obsolete: true
Attachment #458913 - Flags: review?(mstange)
Attachment #459366 - Flags: review?(mstange) → review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Duplicate of this bug: 581009

Updated

8 years ago
Duplicate of this bug: 581471
http://hg.mozilla.org/mozilla-central/rev/4e2ae4080393
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b3

Updated

8 years ago
Duplicate of this bug: 579320
Duplicate of this bug: 580801

Updated

8 years ago
Duplicate of this bug: 582533
Duplicate of this bug: 585102

Comment 21

8 years ago
This problem still exists in FF4b9. I had no problem with beta 8 but since installing beta 9, the line under the title bar flickers in and out constantly.
(In reply to comment #21)
> This problem still exists in FF4b9. I had no problem with beta 8 but since
> installing beta 9, the line under the title bar flickers in and out constantly.

No, you are probably seeing bug 625912.
You need to log in before you can comment on or make changes to this bug.