513158 - [OS X] Drawing in the Title Bar for Firefox 4

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

This is the OS X version of bug 513157, details on the rational can be found in bug 513157 comment #0, or below.

For Firefox 4, there are a variety of reasons why we will need to either draw
in the title bar of the application window, or actually draw the title bar
entirely from scratch:

1) Having a control at the edge of the window frame to set the active Weave
account.

2) Drawing a persona throughout all of chrome (including the title bar).

3) In private browsing mode changing the appearance of the theme, including the
title bar.  Interactively this is to reduce the number of mode errors (a
darkened theme is easily viewable using peripheral vision).  On a more
emotional and design level, it just looks really cool.

4) Placing tabs slightly into the area of the title bar will help us free up
space for the content area.

Since the implementation varies significantly by platform, I'm setting this to
be a meta tracking bug, and I'll file a dependent bugs for each individual
platform.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

(disregard last paragraph, copied too much over)

Comment 2

[Markus Stange [:mstange]]

I have 2 and 3 working (i.e. painting-only, and not responding to mouse events).

Comment 3

[Markus Stange [:mstange]]

Er, that comment was bound to cause misunderstandings; let's try again... (I haven't touched Personas or the private browsing mode appearance)

There are two pieces to implementing what you need:
 1. Drawing in the titlebar. That's what I've got a patch for.
 2. Responding to mouse events in the titlebar.

Without 2., the button you want to put into the titlebar wouldn't be clickable.

However, I'm not yet sure how to implement it without a compromise:
As soon as we take over mouse event handling in the titlebar, we have to use our own faked window-dragging implementation which we're currently using for window-dragging with the toolbar (since bug 398928). This has several drawbacks compared to Cocoa's native window dragging that's provided in the titlebar:
 - You can't drag background windows (bug 465590). Can be fixed.
 - You can't move a window between spaces by dragging it (bug 465542).
   As far as I know, not fixable, but I haven't looked very hard.
 - You can't drag a window while the app is hung. Not fixable.
 - You can't drag a window that contains an open sheet. Might be fixable, but
   not easily.

Comment 4

[Markus Stange [:mstange]]

Attachment: v1

This applies on top of the patch in bug 515880. It only implements drawing in the titlebar, not responding to mouse events.

It works like this:
 - Something (e.g. an extension or a theme) sets drawintitlebar="true" on the
   window element.
 - This calls nsCocoaWindow::SetDrawsInTitlebar(PR_TRUE) which in turn
   propagates it to the ToolbarWindow.
 - The window sends a resize event to Gecko that causes Gecko to layout its
   contents as if the window had suddenly grown vertically by 22px.
 - This will make our root ChildView, which is a subview of the window's
   contentView, 22px higher than the contentView (its superview). But since the
   contentView isn't flipped, this will cause the ChildView to grow upwards
   outside its superview (instead of downwards). So it's in the right place.
 - However, the window's contentView will clip our ChildView, so whatever we
   draw into the ChildView's top 22px strip won't be visible. So we need a
   different way to draw there, using our existing titlebar drawing setup.
 - On trunk, titlebar drawing uses a repeated 1px-wide strip. For content-in-
   titlebar drawing, we need to make this as wide as the window. In this case
   we won't really "repeat" anything, but we still use the CGPattern stuff.
 - The ContentPatternDrawCallback that we use in the content-in-titlebar case
   just calls drawRect:inContext: on the ChildView. This made a few simple
   changes necessary in nsChildView.mm.
 - Proper invalidation in the titlebar is achieved by having the ChildView
   that's invalidated notify the ToolbarWindow, which in turn invalidates its
   border view if necessary.

So there are now three things that can get painted in the titlebar:
 - The normal grey unified gradient, when neither of drawintitlebar or
   (in)activetitlebarcolor is set on the window.
 - A solid color, when (in)activetitlebarcolor is set
 - Window contents, when drawintitlebar is set.
   (drawintitlebar has higher priority than (in)activetitlebarcolor)

Arguably, the titlebarcolor functionality could now be removed, but I'd like to do that in another bug.

Screencast is here: http://www.screencast.com/users/markus_stange/folders/Jing/media/286ee7c7-4ca0-4dea-ab44-ea64fe2e2dea

Comment 5

[Markus Stange [:mstange]]

Tryserver build is here:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-acee303e07e9/

Comment 6

[Josh Aas]

What exactly are the use cases for this without mouse support? Just items 2 and 3 from the bug summary? If so, do we need anything else to implement items 2 and 3? I'd prefer to land something like this along with a consumer.

Comment 7

[Markus Stange [:mstange]]

Yes, just items 2 and 3, and more power for 3rd party theme developers.

It's true that we need additional patches for 2 and 3. The one for item 2 is tiny and I'll take care of that. For item 3 I need some mockups from the UX team first.

I'm perfectly fine with landing this patch along with a consumer, but we can still do the review part now, can't we?

Comment 8

[Markus Stange [:mstange]]

I uploaded a patch for Personas in bug 520637.

Comment 9

[Josh Aas]

Comment on attachment 401346 [details] [diff] [review]
v1

-  NSInteger count, i;
-  [self getRectsBeingDrawn:&rects count:&count];
+  int count, i;

You need NSInteger for "count" given:

- (void)getRectsBeingDrawn:(const NSRect **)rects count:(NSInteger *)count

Using "int" will break 64-bit builds.

+      [window setTitlebarNeedsDisplayInRect:[self convertRect:aRect toView:borderView]];

Shouldn't this only happen if the rect is somewhere in the border view that we care about?

Maybe it would be best for:

+void nsCocoaWindow::SetDrawsInTitlebar(PRBool aState)

to return an error code to the caller if this isn't possible?

Comment 10

[Josh Aas]

I'd like to have roc look at the drawRect: changes.

Comment 11

[Markus Stange [:mstange]]

Attachment: v2

(In reply to comment #9)
> (From update of attachment 401346 [details] [diff] [review])
> -  NSInteger count, i;
> -  [self getRectsBeingDrawn:&rects count:&count];
> +  int count, i;
> 
> You need NSInteger for "count" given:

Oops, that change was a bad merge.

> +      [window setTitlebarNeedsDisplayInRect:[self convertRect:aRect
> toView:borderView]];
> 
> Shouldn't this only happen if the rect is somewhere in the border view that we
> care about?

That's already the case; there's an intersection test in -[ToolbarWindow setTitlebarNeedsDisplayInRect:sync:].

> Maybe it would be best for:
> 
> +void nsCocoaWindow::SetDrawsInTitlebar(PRBool aState)
> 
> to return an error code to the caller if this isn't possible?

That might be useful, but right now no caller cares whether it succeeded, so I think we can keep it "void" for now.

Comment 12

[Markus Stange [:mstange]]

Attachment: v2.1

Tiny change: I've turned the rgnRects null check into an early return, to match roc's patch in bug 518758.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 405400 [details] [diff] [review]
v2.1

roc, could you review the drawRect changes? They're about the same as in bug 518758.

Comment 14

[Johnathan Nightingale [:johnath]]

Josh - if this clears reviews and bakes without incident, would you consider it safe for 1.9.2, with in-browser personas as the first consumer? It would be a nice thing to have, but I defer to you to gauge the risk.

Comment 15

[Josh Aas]

Comment on attachment 405400 [details] [diff] [review]
v2.1

This patch is right on the line for 1.9.2 safety. If drivers are OK with it on 1.9.2 then I am too, but it should be known that this comes with moderate risk. Drivers would know more than me about exactly how risky we want to be right now and how much this feature means to us on the product end.

We also need to verify that this works on 10.4 if we want to try for 1.9.2. Markus?

Comment 16

[Benjamin Smedberg]

AFAIK we don't want or need this bug for 1.9.2

Comment 17

[Johnathan Nightingale [:johnath]]

(In reply to comment #15)
> (From update of attachment 405400 [details] [diff] [review])
> This patch is right on the line for 1.9.2 safety. If drivers are OK with it on
> 1.9.2 then I am too, but it should be known that this comes with moderate risk.
> Drivers would know more than me about exactly how risky we want to be right now
> and how much this feature means to us on the product end.
> 
> We also need to verify that this works on 10.4 if we want to try for 1.9.2.
> Markus?

(In reply to comment #16)
> AFAIK we don't want or need this bug for 1.9.2

If it were free, and zero risk, we would absolutely want it for 1.9.2 because it would improve the personas experience on Mac. But if it's risky (and I take josh to be saying that it is) then it is not required - it's not a blocker and it wasn't something I even thought we *could* have until today.

Comment 18

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/766d68bdf639

Comment 19

[Markus Stange [:mstange]]

I added a test for the only testable aspect of this (inner and outer window dimensions) in the checkin  for bug 522217.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think we want this on 1.9.2 for the reasons mentioned, plus we don't have the capability on Windows yet. Let's save it for 1.9.3

Comment 21

[Alex Faaborg [:faaborg] (Firefox UX)]

Yeah no need to add this yet, but wow, nice work at being the first platform to implement the functionality.  That was really fast! :)

Comment 22

[Aronnax]

(In reply to comment #21)
> Yeah no need to add this yet 

Hi, 
this bug will improve as well the Personas look and feel very much. It is a really very huge difference ;-) 

I think it should be embedded even now - when possible. 

And some are not very interested about Personas stuff, but they have maybe (i have) already some other ideas to use this bug in a different kind of way ;-) 

Cheers