Late 2016 MBP with Touch Bar logs messages from IMKInputSession when focusing text fields

RESOLVED FIXED in Firefox 61

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: jryans, Assigned: mstange)

Tracking

unspecified
mozilla61
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: tpi:-)

Attachments

(9 attachments, 5 obsolete attachments)

211.41 KB, image/png
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
Reporter

Description

2 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.
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.

Updated

2 years ago
Flags: needinfo?(spohl.mozilla.bugs)
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.
Flags: needinfo?(spohl.mozilla.bugs)
Whiteboard: tpi:-
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?
Flags: needinfo?(spohl.mozilla.bugs)
Not that I'm aware of.
Flags: needinfo?(spohl.mozilla.bugs)
Priority: -- → P3
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
Flags: needinfo?(mstange)
Priority: P3 → P1
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.
Flags: needinfo?(mstange)
Posted patch wip (obsolete) — Splinter Review
(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?
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #8944521 - Flags: feedback?(mstange)
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.
Attachment #8944521 - Flags: feedback?(mstange)
Posted patch wip (obsolete) — Splinter Review
Thank you! Now I get a valid context, but TitlebarDrawCallback is never called. Setting f? in case you had any ideas.
Attachment #8944521 - Attachment is obsolete: true
Attachment #8945506 - Flags: feedback?(mstange)
Posted patch wip that paints, but badly (obsolete) — Splinter Review
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?
Attachment #8945506 - Attachment is obsolete: true
Attachment #8945506 - Flags: feedback?(mstange)
Flags: needinfo?(spohl.mozilla.bugs)
Posted patch wip (obsolete) — Splinter Review
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?
Attachment #8945614 - Attachment is obsolete: true
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(mstange)
Attachment #8947999 - Flags: feedback?(mstange)
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.
Flags: needinfo?(mstange)
Attachment #8947999 - Flags: feedback?(mstange)
(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
Attachment #8950279 - Flags: feedback?(mstange)
That sounds great! Can you create a try build with that patch so that I can run it on a 10.9 machine?
Looks like I need to back out AutoBackgroundSetter. Will do so later tonight and resubmit to try.
Green build in comment 18. Thanks for checking it out on 10.9!
Flags: needinfo?(mstange)
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.
Flags: needinfo?(mstange)
(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?
Flags: needinfo?(mstange)
(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
Flags: needinfo?(mstange)
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?
(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.
Assignee: spohl.mozilla.bugs → mstange
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8950279 - Attachment is obsolete: true
Attachment #8950279 - Flags: feedback?(mstange)
Attachment #8947999 - Attachment is obsolete: true

Comment 34

a year 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!
Attachment #8962052 - Flags: review?(emilio) → review+

Comment 35

a year ago
mozreview-review
Comment on attachment 8962051 [details]
Bug 1335191 - Remove unused mTitlebarView field.

https://reviewboard.mozilla.org/r/230874/#review237128
Attachment #8962051 - Flags: review?(spohl.mozilla.bugs) → review+

Comment 36

a year ago
mozreview-review
Comment on attachment 8962054 [details]
Bug 1335191 - Remove TitlebarAndBackgroundColor.

https://reviewboard.mozilla.org/r/230880/#review237132
Attachment #8962054 - Flags: review?(spohl.mozilla.bugs) → review+

Comment 37

a year ago
mozreview-review
Comment on attachment 8962055 [details]
Bug 1335191 - Remove nsIWidget::SetWindowTitlebarColor and the nsCocoaWindow implementation.

https://reviewboard.mozilla.org/r/230882/#review237134
Attachment #8962055 - Flags: review?(spohl.mozilla.bugs) → review+

Comment 38

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

https://reviewboard.mozilla.org/r/230886/#review237136
Attachment #8962057 - Flags: review?(spohl.mozilla.bugs) → review+

Comment 39

a year 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 *
Attachment #8962056 - Flags: review?(spohl.mozilla.bugs) → review+

Comment 40

a year 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!
Attachment #8962058 - Flags: review?(spohl.mozilla.bugs) → review+

Comment 41

a year ago
mozreview-review
Comment on attachment 8962053 [details]
Bug 1335191 - Remove AutoBackgroundSetter.

https://reviewboard.mozilla.org/r/230878/#review237154
Attachment #8962053 - Flags: review?(spohl.mozilla.bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 45

a year 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
You need to log in before you can comment on or make changes to this bug.