J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Reporter
	Description • 8 years ago

I recently switched to a Late 2016 MacBook Pro with the Touch Bar.

When running Firefox from the terminal, I see many logs like the following:

2017-01-30 15:28:43.564 firefox[37614:3719041] IMKInputSession presentFunctionRowItemTextInputViewWithEndpoint:completionHandler: : *NO* NSRemoteViewController to client, NSError=Error Domain=NSCocoaErrorDomain Code=4097 "connection from pid 0" UserInfo={NSDebugDescription=connection from pid 0}, com.apple.inputmethod.EmojiFunctionRowItem

It seems one message is logged each time an input field is focused.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 1 • 8 years ago

Looks like that other applications which handle keyboard events by themselves are also reported this bug, e.g., mvim, atom, etc. However, no threads in their communities have found the cause.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 2 • 7 years ago

This appears to originate from IMKInputSession, which is part of HIToolbox. I was able to confirm what Masayuki said in comment 1 that many other applications report the same log messages with no solutions available at the moment. Some users have reported difficulty switching between applications, but this does not seem to be the case with Firefox. I've been able to reproduce these log messages locally with no obvious ill-effects besides the log messages.

Unless we hear reports of actual usability issues with Firefox I'm tempted to just wait for a fix from Apple.

	Florian Quèze [:florian]
	Comment 3 • 7 years ago

8 months later, this is still very noisy, and makes it hard to see other messages in the terminal when running a local Firefox or Thunderbird build. Is there anything that can be done to silence this?

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 4 • 7 years ago

Not that I'm aware of.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 5 • 7 years ago

While looking into bug 1425027 I realized that these log messages were printed when the OS calls out to EmojiFunctionRowIM_Extension. By attaching to the EmojiFunctionRowIM_Extension process and breaking on Obj-C exceptions, the following exception was caught:

Exception: decodeObjectForKey: class "TitlebarAndBackgroundColor" not loaded or does not exist

TitlebarAndBackgroundColor is a subclass of NSColor that we use to draw separate colors in the titlebar and window background[1]. The fact that the exception is thrown in decodeObjectForKey indicates that the OS is encoding/decoding the NSWindow object[2]. This despite the fact that Apple's docs clearly state that NSWindow does not support coding[3].

I have not been able to come up with a solution so far. I did notice a project on github[4] that draws the window titlebar inside NSView's drawRect: method instead of tapping into NSColor's set: and setFill: methods. Markus, would this be a path worth exploring?

[1] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaWindow.h#164-166
[2] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaWindow.h#175-178
[3] https://developer.apple.com/documentation/appkit/nswindow?language=objc
[4] https://github.com/insidegui/FOTWindow

	Markus Stange [:mstange] Assignee
	Comment 6 • 7 years ago

On 10.10 and above, we can use a titlebar accessory view to draw the titlebar gradient. I don't know of a solution for 10.9.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 7 • 7 years ago

(In reply to Markus Stange [:mstange] from comment #6)
> On 10.10 and above, we can use a titlebar accessory view to draw the
> titlebar gradient. I don't know of a solution for 10.9.

There doesn't seem to be a whole lot of documentation on how to achieve this. This patch adds a NSTitlebarAccessoryViewController to ToolbarWindows and tries to set the gradient from within NSView's setFrameSize:, but [NSGraphicsContext currentContext] is nil at this point. Am I on the right track or am I misunderstanding what we need to do here?

	Markus Stange [:mstange] Assignee
	Comment 8 • 7 years ago

Comment on attachment 8944521 [details] [diff] [review]
wip

I don't think you need to do anything in setFrameSize. The drawing code should go into the drawRect: method of the view. Window resizes will invalidate the whole window, so drawRect should automatically be called on window resizes. If the titlebar needs to be redrawn for other reasons (e.g. if the unified toolbar height changes), then you can call setNeedsDisplay on the view, which will result in a call to drawRect soon after.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 9 • 7 years ago

Thank you! Now I get a valid context, but TitlebarDrawCallback is never called. Setting f? in case you had any ideas.

	Markus Stange [:mstange] Assignee
	Comment 10 • 7 years ago

It wasn't drawing anything because CGContextSetFillPattern only sets the pattern, it doesn't actually draw it. You need a CGContextFillRect call for that. But instead, you can just run the drawing code in TitlebarDrawCallback directly instead of going through the pattern.
The old code was only using a pattern because an NSColor doesn't have a "draw" method; the only thing you can override is "set", in such a way that a future draw call with that color draws the right thing.

The other reason it wasn't drawing anything was that the view was sized to a height of 1 pixel. I've changed it to 22 pixels. However, this uncovered a fundamental problem with this approach: It seems titlebar accessory views are not made to be placed in the background; they either get put in free spaces within the titlebar (when layoutAttribute is NSLayoutAttributeRight or  NSLayoutAttributeLeft), or *under* the titlebar, moving the content down (when layoutAttribute is NSLayoutAttributeBottom). This patch here moves the content down. This is not what we want to happen.


So it seems this approach is not going to work. I'd like to suggest a more radical approach which should result in simpler code:

I think we can remove the class TitlebarAndBackgroundColor entirely, without replacement. It's currently serving two purposes:
 (1) It supports filling the titlebar with a random solid color, and
 (2) It supports drawing the titlebar part of the unified toolbar gradient,
     with an adjustable unified toolbar height.

(1) is not needed any more. Nothing uses the "activetitlebarcolor" and "inactivetitlebarcolor" XUL attributes, so I think we should remove all the related APIs.

For (2), we can take advantage of -[NSWindow setContentBorderThickness:forEdge:]: For NSWindows with the "textured background" window mask and the default background color, that API influences not just the sheet attachment position, but also the gradient that is drawn in the background of that window, including in the titlebar. So if we call that method with the unified toolbar height, then the titlebar should be filled with the correct gradient automatically.
We currently call that function with the sheet attachment position, which might be different from the unified toolbar height. I think we should resolve this in the following way:
 - For windows with titlebars, i.e. windows that don't draw their contents into the titlebar, ignore calls to setSheetAttachmentPosition and always pass the unified toolbar height to -[NSWindow setContentBorderThickness:forEdge:].
 - For windows without titlebars, i.e. windows that draw their contents into the titlebar, call -[NSWindow setContentBorderThickness:forEdge:] with the value passed to setSheetAttachmentPosition - in other words, keep doing what we're doing at the moment. In these windows, the background that the window draws behind our content isn't visible anyway.

I hope this works out. Does it make sense so far?

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 11 • 7 years ago

So, I started by addressing your step 1 by removing the TitlebarAndBackgroundColor class and removing references to our NSColor subclass. I then built the tree to see if the compiler would complain about any references that I forgot to remove. I expected the title bar to be drawn incorrectly when I ran this build. But to my surprise, the title bar was drawn correctly, vibrancy worked as expected, adding drag space to the title bar and showing the native title bar worked as well. I have also verified that sheets appear in the right place regardless of the "drag space" being enabled or disabled. And I've even verified that the "Trackpad Handwriting" dialog can now be dismissed as expected (bug 1425027), which failed before due to our custom NSColor subclass.

Clearly I'm missing something here. Why does this appear to draw the title bar part of the unified toolbar gradient, with an adjustable unified toolbar height, when I haven't even started on step 2 of your suggested approach yet?

	Markus Stange [:mstange] Assignee
	Comment 12 • 7 years ago

Nice!

(In reply to Stephen A Pohl [:spohl] from comment #11)
> Clearly I'm missing something here. Why does this appear to draw the title
> bar part of the unified toolbar gradient, with an adjustable unified toolbar
> height, when I haven't even started on step 2 of your suggested approach yet?

There are two things that aren't working entirely correctly:
 (1) In Windows with a titlebar but without a toolbar, the titlebar separator line is missing.
     This can be seen in the devtools in separate-window mode, for example. It can also be
     seen if you enable the titlebar for the main browser window and apply a lightweight
     theme.
 (2) In windows with a narrow unified toolbar and large <toolbox> element, the gradient will
     be slightly off. (The toolbox determines the sheet attachment position.)
     (1) can be seen as a special case of (2) where the toolbar is 0px high (i.e. really narrow).

(1) is something that I hadn't thought about when I wrote comment 10 and I'm not sure how to address it. Texture background windows never draw a separator line under the gradient, they only draw the gradient.
Maybe we can toggle the window mask at runtime and remove the textured window background mask when unifiedToolbarHeight == titlebarHeight? But that might cause things to flash or shift slightly.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 13 • 7 years ago

(In reply to Markus Stange [:mstange] from comment #12)
> Nice!
> 
> (In reply to Stephen A Pohl [:spohl] from comment #11)
> > Clearly I'm missing something here. Why does this appear to draw the title
> > bar part of the unified toolbar gradient, with an adjustable unified toolbar
> > height, when I haven't even started on step 2 of your suggested approach yet?
> 
> There are two things that aren't working entirely correctly:
>  (1) In Windows with a titlebar but without a toolbar, the titlebar
> separator line is missing.
>      This can be seen in the devtools in separate-window mode, for example.
> It can also be
>      seen if you enable the titlebar for the main browser window and apply a
> lightweight
>      theme.
>  (2) In windows with a narrow unified toolbar and large <toolbox> element,
> the gradient will
>      be slightly off. (The toolbox determines the sheet attachment position.)
>      (1) can be seen as a special case of (2) where the toolbar is 0px high
> (i.e. really narrow).
> 
> (1) is something that I hadn't thought about when I wrote comment 10 and I'm
> not sure how to address it. Texture background windows never draw a
> separator line under the gradient, they only draw the gradient.
> Maybe we can toggle the window mask at runtime and remove the textured
> window background mask when unifiedToolbarHeight == titlebarHeight? But that
> might cause things to flash or shift slightly.

From what I could figure out, changing window masks after initial creation isn't possible. We would have to recreate all the windows and omit the NSTexturedBackgroundWindowMask during creation, which I don't think is feasible. However, do we actually need to set the NSTexturedBackgroundWindowMask? I removed it as a quick test in the attached patch (and commented out calls that are only supported for windows with the NSTexturedBackgroundWindowMask set) and everything seems to work nominally... The windows appear correctly, the titlebar appears correctly, when the native titlebar is shown the titlebar separator is shown (including in the devtools window, for example), dragging works as expected, window corners are still rounded etc. I couldn't find any issues, at least on 10.12. Should I send this to try and request testing across all versions of macOS that we currently support to make sure that there is no fallout from this change? Or is there something that I missed?

NSTexturedBackgroundWindowMask is also marked as deprecated (10.0-10.12)[1].

[1] https://developer.apple.com/documentation/appkit/nstexturedbackgroundwindowmask

	Markus Stange [:mstange] Assignee
	Comment 14 • 7 years ago

That sounds great! Can you create a try build with that patch so that I can run it on a 10.9 machine?

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 15 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff0de591777043e542e0b2bbf968d041ce0b0206

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 16 • 7 years ago

Looks like I need to back out AutoBackgroundSetter. Will do so later tonight and resubmit to try.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 17 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94ed8b0af31b4dbff0931e64b12493461e3acfa1

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 18 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a8761a0a63293fd5c6123a0e207ac3bcb1a3da9

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 19 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf2a90167f98f21c38905e2948c3a46aa31cf93

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 20 • 7 years ago

Green build in comment 18. Thanks for checking it out on 10.9!

	Markus Stange [:mstange] Assignee
	Comment 21 • 7 years ago

I tested your build on my main machine, which runs 10.12.6, and it doesn't have any working unified gradients. So setContentBorderThickness seems to not be respected unless you have a textured background window.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 22 • 7 years ago

(In reply to Markus Stange [:mstange] from comment #21)
> Created attachment 8951378 [details]
> no unified gradient in library on 10.12
> 
> I tested your build on my main machine, which runs 10.12.6, and it doesn't
> have any working unified gradients. So setContentBorderThickness seems to
> not be respected unless you have a textured background window.

Heh, well, I now understand what setContentBorderThickness is responsible for... It isn't being respected because I removed it in my patches. It can't be called on a window unless it is a textured background window.

So, to recap, if we don't do anything, macOS is unable to encode/decode our window class due to the custom NSColor class in our NSWindow subclass. This causes bugs like bug 1425027. If we remove the custom NSColor subclass but leave the NSTexturedBackgroundWindowMask, we don't get a separator between native title bars and the rest of the winodw. If we remove the NSTexturedBackgroundWindowMask, we can't call setContentBorderThickness and we don't get unified gradients.

Any suggestion on how to proceed?

	Markus Stange [:mstange] Assignee
	Comment 23 • 7 years ago

(In reply to Stephen A Pohl [:spohl] from comment #22)
> So, to recap, if we don't do anything, macOS is unable to encode/decode our
> window class due to the custom NSColor class in our NSWindow subclass. This
> causes bugs like bug 1425027. If we remove the custom NSColor subclass but
> leave the NSTexturedBackgroundWindowMask, we don't get a separator between
> native title bars and the rest of the winodw. If we remove the
> NSTexturedBackgroundWindowMask, we can't call setContentBorderThickness and
> we don't get unified gradients.

This all matches my understanding, yes.

> Any suggestion on how to proceed?

Maybe you can give the change-styleMask-at-runtime idea another try?

(In reply to Stephen A Pohl [:spohl] from comment #13)
> > Maybe we can toggle the window mask at runtime and remove the textured
> > window background mask when unifiedToolbarHeight == titlebarHeight? But that
> > might cause things to flash or shift slightly.
> 
> From what I could figure out, changing window masks after initial creation
> isn't possible.

NSWindow has a styleMask property which is readwrite (the default): https://developer.apple.com/documentation/appkit/nswindow/1419078-stylemask?language=objc
So NSWindow should have a setStyleMask: method.

> NSTexturedBackgroundWindowMask is also marked as deprecated (10.0-10.12)[1].

That's because it got renamed to something that follows the "NSWindowStyleMask*" naming scheme:
NSWindowStyleMaskTexturedBackground
https://developer.apple.com/documentation/appkit/nswindowstylemask/nswindowstylemasktexturedbackground?language=objc

	Markus Stange [:mstange] Assignee
	Comment 24 • 6 years ago

I found solutions to all our problems.

If we drop NSTexturedBackgroundWindowMask, we can still make the titlebar gradient extend into the toolbar, by overriding part of the machanism that does this for windows with a real NSToolbar. More precisely, we can override the frame view's private _unifiedToolbarFrame method. (The alternative would be to emulate a real NSToolbar, but that seems more brittle. We could also try to affect other conditions so that the existing implementation of -[NSThemeFrame _unifiedToolbarFrame] would end up computing the value we want, but that'd involve somehow controlling the values returned by -[NSThemeFrame contentBorderThicknessForEdge:], -[NSThemeFrame _topBarHeightWithoutContentBorderThickness], and -[NSThemeFrame _topBarHeight], which sounds even harder to achieve.)

And for positioning sheets we can use the WindowDelegate method window:willPositionSheet:usingRect:.

Mind if I steal this bug, Stephen?

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 25 • 6 years ago

(In reply to Markus Stange [:mstange] from comment #24)
> Mind if I steal this bug, Stephen?

No, please go ahead! I've been side-tracked with top crashers. Sorry for the delay here.

	Emilio Cobos Álvarez (:emilio)
	Comment 34 • 6 years ago
mozreview-review

Comment on attachment 8962052 [details]
Bug 1335191 - Remove support for the attributes "activetitlebarcolor" and "inactivetitlebarcolor" for XUL window elements.

https://reviewboard.mozilla.org/r/230876/#review236358

r=me, thanks!

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 35 • 6 years ago
mozreview-review

Comment on attachment 8962051 [details]
Bug 1335191 - Remove unused mTitlebarView field.

https://reviewboard.mozilla.org/r/230874/#review237128

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 36 • 6 years ago
mozreview-review

Comment on attachment 8962054 [details]
Bug 1335191 - Remove TitlebarAndBackgroundColor.

https://reviewboard.mozilla.org/r/230880/#review237132

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 37 • 6 years ago
mozreview-review

Comment on attachment 8962055 [details]
Bug 1335191 - Remove nsIWidget::SetWindowTitlebarColor and the nsCocoaWindow implementation.

https://reviewboard.mozilla.org/r/230882/#review237134

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 38 • 6 years ago
mozreview-review

Comment on attachment 8962057 [details]
Bug 1335191 - Stop using a textured window.

https://reviewboard.mozilla.org/r/230886/#review237136

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 39 • 6 years ago
mozreview-review

Comment on attachment 8962056 [details]
Bug 1335191 - Position sheets using the NSWindowDelegate method window:willPositionSheet:usingRect:.

https://reviewboard.mozilla.org/r/230884/#review237138

::: widget/cocoa/nsCocoaWindow.mm:2832
(Diff revision 1)
>  
>    mHasEverBeenZoomed = YES;
>    return YES;
>  }
>  
> +- (NSRect)window:(NSWindow *)window willPositionSheet:(NSWindow *)sheet usingRect:(NSRect)rect

nit: no spaces between NSWindow and *

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 40 • 6 years ago
mozreview-review

Comment on attachment 8962058 [details]
Bug 1335191 - Override -[NSThemeFrame _unifiedToolbarFrame] in order to get a correctly-sized unified toolbar gradient drawn in the titlebar.

https://reviewboard.mozilla.org/r/230888/#review237152

This is great! Thank you!

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 41 • 6 years ago
mozreview-review

Comment on attachment 8962053 [details]
Bug 1335191 - Remove AutoBackgroundSetter.

https://reviewboard.mozilla.org/r/230878/#review237154

	Pulsebot
	Comment 45 • 6 years ago

Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/9abca887123c
Remove unused mTitlebarView field. r=spohl
https://hg.mozilla.org/integration/autoland/rev/193fca9196cd
Remove support for the attributes "activetitlebarcolor" and "inactivetitlebarcolor" for XUL window elements. r=emilio
https://hg.mozilla.org/integration/autoland/rev/85541a81ab96
Remove AutoBackgroundSetter. r=spohl
https://hg.mozilla.org/integration/autoland/rev/4ecb55b86f69
Remove TitlebarAndBackgroundColor. r=spohl
https://hg.mozilla.org/integration/autoland/rev/7590e61d2964
Remove nsIWidget::SetWindowTitlebarColor and the nsCocoaWindow implementation. r=spohl
https://hg.mozilla.org/integration/autoland/rev/3d665b0a95bf
Position sheets using the NSWindowDelegate method window:willPositionSheet:usingRect:. r=spohl
https://hg.mozilla.org/integration/autoland/rev/6e66dd67c94e
Stop using a textured window. r=spohl
https://hg.mozilla.org/integration/autoland/rev/27d30999d073
Override -[NSThemeFrame _unifiedToolbarFrame] in order to get a correctly-sized unified toolbar gradient drawn in the titlebar. r=spohl

	Eliza Balazs [:ebalazs_]
	Comment 46 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9abca887123c
https://hg.mozilla.org/mozilla-central/rev/193fca9196cd
https://hg.mozilla.org/mozilla-central/rev/85541a81ab96
https://hg.mozilla.org/mozilla-central/rev/4ecb55b86f69
https://hg.mozilla.org/mozilla-central/rev/7590e61d2964
https://hg.mozilla.org/mozilla-central/rev/3d665b0a95bf
https://hg.mozilla.org/mozilla-central/rev/6e66dd67c94e
https://hg.mozilla.org/mozilla-central/rev/27d30999d073