877767 - OpenGL context covers the window title

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josiah Bruner [:jsbruner]]

Bug 676241 now covers the title bar with an OpenGL context drawing. Unfortunately, it currently is drawing over the standard window titlebar thereby removing the window title. Although for the UX branch this is not an issue, it is for the current Nightly.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

The OS uses an NSCell object to draw the title (later I'll look up exactly which one).  I expect we can use the same strategy to composite it with the OpenGL context that we currently use for the titlebar buttons, or at least a similar one.

Comment 2

[Markus Stange [:mstange]]

I think we should just call _drawTitleBar.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> I think we should just call _drawTitleBar.

That might work.

A quick look at assembly code for -[NSThemeFrame _drawTitleBar] for OS X 10.7.5 shows that it's a fairly thin wrapper around -[NSTitledFrame _drawTitleStringIn:(NSRect)aRect withColor:(CGFloat)color], and that seems (basically) to find the frame view's "titleCell" and call its -[NSCell drawWithFrame:inView:] method.

Before you commit to this, though, you should test on all three supported platforms, and also compare the difference between doing this and simply calling drawWithFrame:inView: on the frame view's "titleCell".

Comment 4

[Markus Stange [:mstange]]

Attachment: part 1: set context flippedness properly

only tested on 10.8, attaching what I have so far

Comment 5

[Markus Stange [:mstange]]

Attachment: part 2: call _drawTitleBar:

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 756213 [details] [diff] [review]
part 1: set context flippedness properly

I think I've finally understood why NSCell drawing is sometimes randomly flipped. They seem to look at [[NSGraphicsContext currentContext] isFlipped] and do something random when it doesn't match their expectation.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 756214 [details] [diff] [review]
part 2: call _drawTitleBar:

I've tested this on 10.6 and 10.8.

I've tried the [[frameView titleCell] drawWithFrame:inView:] approach you suggested, but that looks a little more complicated. Calling it just like that results in white text in the upper left corner of the window. Using [frameView _titlebarTitleRect] instead of [frameView bounds] would probably work to get the position right, but would add one more undocumented call. I'm not sure why the color / shadow didn't work. Hopper tells me that on 10.8, -[NSTitledFrame _drawTitleStringIn:withColor:] calls __NSDrawTextCell instead of -[NSCell drawWithFrame:inView:], and __NSDrawTextCell looks... complicated. Not sure what's going on there.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 756213 [details] [diff] [review]
part 1: set context flippedness properly

This looks fine to me.

> I think I've finally understood why NSCell drawing is sometimes
> randomly flipped. They seem to look at [[NSGraphicsContext
> currentContext] isFlipped] and do something random when it doesn't
> match their expectation.

When I have time I'll look at the assembly code for -[NSCell drawWithFrame:inView:] to see what I can find.  You could also do this, Markus :-)

For completeness sake, we'll probably need to do it on all three supported platforms.

By the way, I'm going away for a long weekend.  So I won't be able to do any more work here until I get back next week.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #7)

Sounds like it might be best just to use _drawTitleBar:, even though it's something of a black box.

If we could just do [[frameView titleCell] drawWithFrame:inView:], with the NSRect set appropriately (even if this means using _titlebarTitleRect), that'd be safer, because it'd make fewer assumptions about Apple's undocumented behavior.  But the more code we have to add if we do this, the more additional assumptions we'll have to make.  And at some point it's not really any improvement over calling _drawTitleBar:.

Next week I'll take another crack at understanding _drawTitleBar: (and the methods it calls).  If they look pretty much the same from SnowLeopard to MountainLion, it's probably pretty safe to use it.

I'd also like to try to find out what part of Apple's behavior in _drawTitleBar: (and below) we really need.  If that's simple enough, we could do that instead.  The advantage would be that we'd be making our assumptions (about Apple's undocumented behavior) more explicit.

Comment 10

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #9)
> (In reply to comment #7)
> 
> Sounds like it might be best just to use _drawTitleBar:, even though it's
> something of a black box.

The 10.8 implementation does, if I recall correctly:

if (!([window styleMask] & NSTexturedBackgroundWindowMask)) {
  // Draw titlebar background gradient.
  // This case isn't reached for our ToolbarWindows because they
  // use NSTexturedBackgroundWindowMask.
}
// Draw title.
[self _drawTitleStringIn:rect withColor:[self _currentTitleColor]];

I'd prefer to call [self _drawTitleStringIn:rect withColor:[self _currentTitleColor]] directly, because it's more focused, but it looks like 10.6 doesn't have _currentTitleColor. I haven't installed Hopper on 10.6 yet and haven't checked what _drawTitleBar does there.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

I'm hoping to get back to this later today or tomorrow.

I've got a bunch of other reviews to deal with first, though (bug 880124, bug 880153 and bug 853105).

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Just noticed something interesting:

On OS X 10.8 there's only one implementation of _drawTitleBar: (the one implemented by NSThemeFrame).  So that's the one that always gets called.

But 10.7 and 10.6 have two implementations -- -[NSThemeFrame _drawTitleBar:] and -[NSGrayFrame _drawTitleBar:].  And (surprisingly) it's the NSGrayFrame implementation that's always called in current Firefox m-c code, even though it's called from -[NSThemeFrame drawFrame:].

So it's the NSGrayFrame implementation that we should probably be looking at on 10.7 and 10.6.

Interestingly, on all three platforms _drawTitleBar: (whichever implementation) is always called from -[NSThemeFrame drawFrame:].

Comment 13

[Steven Michaud [:smichaud] (Retired)]

> it's the NSGrayFrame implementation that's always called in current Firefox m-c code

At least this is where gdb breaks when I set breakpoints on both implementations.

But after looking at the assembly code I wonder if this is correct.  I'll dig further tomorrow.

(I've been trying to learn more about the _drawTitleBar: code by stepping through it in gdb.)

Comment 14

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #13)

Ignore comment #13.  NSGrayFrame inherits directly from NSThemeFrame, so it makes sense that it's the -[NSGrayFrame _drawTitleBar:] method that gets used (on 10.7 and below).

Comment 15

[Markus Stange [:mstange]]

Starting with Hopper's pseudocode output for -[NSGrayFrame _drawTitleBar:] on 10.6, it roughly seems to look like this:

- (void)_drawTitleBar:(NSRect)rect
{
  // (first some rect management with [self bounds] and [self _titlebarHeight])
  NSColor* color;
  if ([_window _hasActiveAppearanceIgnoringKeyFocus] || [self _isUtility]) {
    color = [NSColor windowFrameTextColor];
  } else if ([_window _isDarkWindow]) {
    // I think our window returns true for _isDarkWindow, but I haven't confirmed
    color = [NSColor disabledControlTextColor];
  } else {
    color = [NSColor colorWithCalibratedWhite:? alpha:?];
  }
  [rdi _drawTitleStringIn:rect withColor:color];
}

Comment 16

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #12)
> On OS X 10.8 there's only one implementation of _drawTitleBar: (the one
> implemented by NSThemeFrame).  So that's the one that always gets called.
> 
> But 10.7 and 10.6 have two implementations -- -[NSThemeFrame _drawTitleBar:]
> and -[NSGrayFrame _drawTitleBar:].

This probably explains the "if" condition in the 10.8 implementation (see comment 10): Before NSGrayFrame and NSThemeFrame were merged, NSGrayFrame only drew the text and NSThemeFrame first painted the titlebar background gradient and then painted the text on top. On 10.8, where there's only one implementation, it first checks the window's style mask in order to decide whether to use the old NSGrayFrame or NSThemeFrame behavior.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #15)

I've reconstructed -[NSGrayFrame _drawTitleBar:(NSRect)rect] on both
Lion and SnowLeopard.  My reconstruction is very similar to yours, but
adds a few more details.

I worked from assembler code -- reading it and stepping through it (I
didn't rely on Hopper's pseudocode, which I've sometimes found
mistaken).  I also used an interpose library to make -[NSWindow
_isDarkWindow] return NO (instead of its normal YES) to find the
correct parameters for the third branch of the if statement.

- (void)_drawTitleBar:(NSRect)rect
{
  // (first some rect management with [self bounds] and [self _titlebarHeight])
  NSColor* color;
  if ([_window _hasActiveAppearanceIgnoringKeyFocus] || [self _isUtility]) {
    color = [NSColor windowFrameTextColor];
  } else if ([_window _isDarkWindow]) {
    color = [NSColor disabledControlTextColor];
  } else {
    color = [NSColor colorWithCalibratedWhite:0 alpha:0.55];
  }
  [self _drawTitleStringIn:rect withColor:color];
}

As you supposed, our windows always return YES when -[NSWindow
_isDarkWindow] is called on them.  This will method will return NO on
an NSWindow when an undocumented styleMask flag is set (1 << 5) or
when -[NSWindow _usesCustomDrawing] returns NO.  -[NSWindow
_usesCustomDrawing] returns YES when its _borderView (its frame view)
doesn't belong to any of the following classes -- NSThemeFrame,
NSGrayFrame or NSNextStepFrame.  (This, incidentally, is probably why
my patch from bug 851652 comment #46 had wierd side effects -- it
created new NSFrameView subclasses that didn't inherit from any of
these three classes.)

Our frame views also always return NO when _isUtility is called on
them.  -[NSFrameView _isUtility] always returns NO.  -[NSThemeFrame
_isUtility] only returns YES if either of two undocumented styleMask
values is set -- (1 << 4) or (1 << 9).

Comment 18

[Steven Michaud [:smichaud] (Retired)]

> (This, incidentally, is probably why my patch from bug 851652
> comment #46 had wierd side effects -- it created new NSFrameView
> subclasses that didn't inherit from any of these three classes.)

It created new NSFrameView subclasses that didn't *belong* to any of
these three classes.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

I just took a quick look at the assembler code for -[NSTitledFrame _drawTitleStringIn:(NSRect)rect withColor:(NSColor *)color] on MountainLion, Lion and SnowLeopard.  I expect there won't be any trouble calling it from nsChildView::UpdateTitlebarImageBuffer().

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> As you supposed, our windows always return YES when -[NSWindow
> _isDarkWindow] is called on them.  This will method will return NO
> on an NSWindow when an undocumented styleMask flag is set (1 << 5)
> or when -[NSWindow _usesCustomDrawing] returns NO.

Oops.  The second sentence should read as follows:

As you supposed, our windows always return YES when -[NSWindow
_isDarkWindow] is called on them.  This will method will return NO on
an NSWindow when an undocumented styleMask flag is set (1 << 5) or
when -[NSWindow _usesCustomDrawing] returns *YES*.

The gist of it is that I think we can safely assume -[NSWindow
_isDarkWindow] will always return YES on our windows and that
-[frameView _isUtility] will always return NO on our frame views.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

For what it's worth, this seems to be the pseudocode for -[NSTitledFrame _drawTitleStringIn:withColor:] on Lion and MountainLion:

- (void)_drawTitleStringIn:(NSRect)rect withColor:(NSColor*)color
{
  NSCell *customTitleCell = [self _customTitleCell];
  if (!customTitleCell) {
    return;
  }
  NSRect _titlebarTitleRect = [self _titlebarTitleRect];
  if (!NSIntersectsRect(_titlebarTitleRect, rect)) {
    return;
  }
  if ([_customTitleCell isKindOfClass:[NSTextFieldCell class]]) {
    [_customTitleCell setTextColor:color];
  }
  if (_customTitleCell == [self titleCell] &&
      [_customTitleCell class] == [NSTextFieldCell class] &&
      ![_customTitleCell isBezeled] &&
      ![_customTitleCell isBordered] &&
      ![_customTitleCell drawsBackground]) {
    _NSDrawTextCell(self, customTitleCell, _titlebarTitleRect, 0, 1);
  } else {
    [_customTitleCell drawWithFrame:_titlebarTitleRect inView:self];
  }
}

Interesting that the entire title cell gets drawn even if only part of it intersects 'rect'.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> _NSDrawTextCell(self, customTitleCell, _titlebarTitleRect, 0, 1);

Oops.  This should be:

_NSDrawTextCell(customTitleCell, self, _titlebarTitleRect, 0, 1);

Comment 23

[Steven Michaud [:smichaud] (Retired)]

And here's pseudocode for -[NSTitledFrame _drawTitleStringIn:withColor:] on SnowLeopard:

- (void)_drawTitleStringIn:(NSRect)rect withColor:(NSColor*)color
{
  NSCell *customTitleCell = [self _customTitleCell];
  if (!customTitleCell) {
    return;
  }
  NSRect _titlebarTitleRect = [self _titlebarTitleRect];
  if (!NSIntersectsRect(_titlebarTitleRect, rect)) {
    return;
  }
  if ([_customTitleCell isKindOfClass:[NSTextFieldCell class]]) {
    [_customTitleCell setTextColor:color];
    BOOL appearanceEnabled = NO;
    if ([self._window _hasActiveAppearanceIgnoringKeyFocus] ||
        [fiddle with undocumented values of [self styleMask]]) {
      appearanceEnabled = YES;
    }
    if ([self._window _isDarkWindow]) {
      [_customTitleCell setEnabled:appearanceEnabled];
    }
  }
  if (_customTitleCell == [self titleCell] &&
      [_customTitleCell class] == [NSTextFieldCell class] &&
      ![_customTitleCell isBezeled] &&
      ![_customTitleCell isBordered] &&
      ![_customTitleCell drawsBackground]) {
    _NSDrawTextCell(customTitleCell, self, _titlebarTitleRect, 4, 1);
  } else {
    [_customTitleCell drawWithFrame:_titlebarTitleRect inView:self];
  }
}

Comment 24

[Steven Michaud [:smichaud] (Retired)]

I rather suspect that, instead of calling -[NSTitledFrame _drawTitleStringIn:withColor:], we can get away with using just the following parts of it:

  NSCell *customTitleCell = [self _customTitleCell];
  if (!customTitleCell) {
    return;
  }
  NSRect _titlebarTitleRect = [self _titlebarTitleRect];
  if (!NSIntersectsRect(_titlebarTitleRect, rect)) {
    return;
  }
  if ([_customTitleCell isKindOfClass:[NSTextFieldCell class]]) {
    [_customTitleCell setTextColor:color];
  }
  [_customTitleCell drawWithFrame:_titlebarTitleRect inView:self];

But I'm not sure it's worth the trouble to find out.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #17)

Here's full pseudocode for -[NSGrayFrame _drawTitleBar:(NSRect)rect] on Lion and SnowLeopard.  I translated the "rect management" that our earlier reconstructions left out.  Hardly anything substantive is added -- the "rect management" only actually increments rect.size.height, and then only on Lion.  But for the sake of completeness:

- (void)_drawTitleBar:(NSRect)rect
{
  NSRect bounds = [self bounds];
  double _titlebarheight = [self _titlebarheight];
#ifdef LION
  bounds.origin.y = bounds.origin.y + bounds.size.height - _titlebarheight;
  bounds.size.height = _titlebarheight;
  rect.size.height += 1.0; // A double constant
#endif
  NSColor* color;
  if ([_window _hasActiveAppearanceIgnoringKeyFocus] || [self _isUtility]) {
    color = [NSColor windowFrameTextColor];
  } else if ([_window _isDarkWindow]) {
    color = [NSColor disabledControlTextColor];
  } else {
    color = [NSColor colorWithCalibratedWhite:0 alpha:0.55];
  }
  [self _drawTitleStringIn:rect withColor:color];
}

Hopper note:

Very strangely, Hopper uses decimal numbers in its labels for local variables, but hexadecimal numbers in its labels for parameters passed on the stack.  So "var_64" is the variable at an offset of 64 bytes from the bottom of the space reserved for local variables when the stack frame is built (using e.g. "sub rsp, 0x48").  And "arg_10" is the variable at an offset of 0x10 (i.e. 16) bytes from the start of the first parameter passed on the stack.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

> rect.size.height += 1.0; // A double constant

This should be:

rect.origin.y += 1.0; // A double constant

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Here's the pseudocode for OS X 10.8.3's -[NSThemeFrame _drawTitleBar:] and -[NSThemeFrame _drawTitleStringInClip:].

- (void)_drawTitleBar:(NSRect)rect
{
  if (!_tFlags.suppressTitleBackgroundDrawing) && ![self _isTexturedWindow]) {
    [self _drawTitleBarBackgroundInClipRect:rect];
  }
  if (!_tFlags.suppressTitleDrawing && [self _wantsTitleString]) {
    [self _drawTitleStringInClip:rect];
  }
}

- (void)_drawTitleStringInClip:(NSRect)rect
{
  rect.origin.y += 1.0; // A double constant
  [NSGraphicsContext saveGraphicsState];
  // As best I can tell, for us [self layer] always returns NULL.
  if ([self layer]) {
    CGContextRef ctx = [[NSGraphicsContext currentContext] graphicsPort];
    _CGContextSetShouldSmoothFonts(ctx, 1);
  }
  [self _drawTitleStringIn:rect withColor:[self _currentTitleColor]];
  [NSGraphicsContext restoreGraphicsState];
}

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Do you want to keep working on this, Markus, or shall I pick it up?

Comment 29

[Markus Stange [:mstange]]

Feel free to pick it up. I'm not sure what's left to do here. As far as I can tell, the result of your investigation is that on all supported systems _drawTitleBar does exactly what we want it to, so we could go ahead with the current patch. Is that your opinion, too?

Comment 30

[Steven Michaud [:smichaud] (Retired)]

> Feel free to pick it up.

I will.

> we could go ahead with the current patch

I'd prefer to use _drawTitleStringIn:withColor: (or methods called by it) directly, if that works.  I should be able to spend a few hours on that later this week.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 756214 [details] [diff] [review]
part 2: call _drawTitleBar:

> I'd prefer to use _drawTitleStringIn:withColor: (or methods called
> by it) directly, if that works.  I should be able to spend a few
> hours on that later this week.

I've now changed my mind about this, and think it should be fine for
us to use _drawTitleBar.

+// Draws the title string of a window.
+// Present on NSThemeFrame since at least 10.6.
+- (void)_drawTitleBar:(NSRect)aRect;

But I'd like to add something like the following to your comment here:

// _drawTitleBar is somewhat complex, and has changed over the years
// since OS X 10.6.  But in that time it's never done anything that
// would break when called outside of -[NSView drawRect:] (which we
// sometimes do), or whose output can't be redirected to a
// CGContextRef object (which we also sometimes do).  This is likely
// to remain true for the indefinite future.  However we should
// check _drawTitleBar in each new major version of OS X.  For more
// information see bug 877767.

Comment 32

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2795fd5bce0
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4883c469e6

Comment 33

[Markus Stange [:mstange]]

Note that the second changeset should be backed out of UX again when it makes its way there through a merge. Otherwise we'll cover the tabs with the window title.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Aren't there some cases, even with Australis, when we want the title to be displayed?

Comment 35

[Matthew N. [:MattN]]

Probably a lot of the same places where we don't want to move the window controls down (bug 851652) such as popup windows without the tab strip.

Comment 36

[Markus Stange [:mstange]]

I've filed bug 888615 for that question.

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c2795fd5bce0
https://hg.mozilla.org/mozilla-central/rev/0d4883c469e6

Comment 38

[Markus Stange [:mstange]]

Comment on attachment 756213 [details] [diff] [review]
part 1: set context flippedness properly

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 676241
User impact if declined: Private browsing windows or windows with background themes / Personas won't have a window title
Testing completed (on m-c, etc.): 3 weeks on mozilla-central
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 39

[bhavana bajaj [:bajaj]]

Comment on attachment 756213 [details] [diff] [review]
part 1: set context flippedness properly

low risk patch for a Fx24 regression.Requesting QA verification to help verify once the patch landed on aurora.Thanks

Comment 40

[Markus Stange [:mstange]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93fd34a71fbd
https://hg.mozilla.org/releases/mozilla-aurora/rev/655e78727928

Comment 41

[u279076]

Markus, can you please advise QA how we might verify this is fixed?

Comment 42

[Markus Stange [:mstange]]

STR:
1. Open a new private browsing window.
2. Ensure that the window has a title (e.g. "Nightly - (Private Browsing)")

Comment 43

[Ioana (away)]

Verified as fixed on Firefox 25 beta 4 - 20131001024718 - on Mac OS X 10.7.5.