1402054 - [10.13] Rendering issues of close, minimize, maximize/fullscreen buttons

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Stephen A Pohl [:spohl]]

(In reply to Oana Botisan from bug 1400057 comment 18)
> Also trying to reproduce this bug I've noticed another issue. The close
> ('x'), minimize '-' and maximize '<>' buttons are not perfectly round. This
> link
> https://docs.google.com/document/d/1OiLwz4x-YR9T4NeO-ErLWJn5mJr1_xBXNJagfWR--
> xw/edit# will show you the comparison between another window (left) and
> Firefox (right).This problem doesn't depend on any theme, but I believe that
> it appears because the default theme has two colors.

Comment 1

[whmountains]

I'm getting a 404 on the google doc link.  Could you add it as an attachment?

Comment 2

[Stephen A Pohl [:spohl]]

Comment 0 added a line break to the link. The link is:
https://docs.google.com/document/d/1OiLwz4x-YR9T4NeO-ErLWJn5mJr1_xBXNJagfWR--xw/edit

Comment 3

[Stephen A Pohl [:spohl]]

Attachment: Patch

It appears that on High Sierra, the window buttons can only be drawn in the space that the native title bar would usually occupy. The native title bar is 22px. By default on nightly, the windowButtonRect.y is set to 17. The window button itself is 12px. Half of the window button, plus the windowButtonRect.y results in 23px, exceeding the 22px of the native title bar and resulting in cut off window buttons. The issue is much more obvious by adding drag space to the title bar:
1. Click on hamburger menu, customize.
2. Select "Drag Space"

Basically half of the window buttons will now be cut off.

It appears that we need to cap windowButtonRect.y at 16 to allow for sufficient space to draw the window buttons.

I've been trying to figure out where the rect for nsNativeThemeCocoa::eThemeGeometryTypeWindowButtons gets adjusted, as I would prefer to make this adjustment there. However, I've been unsuccessful so far.

Comment 4

[Markus Stange [:mstange]]

I'd like to understand more about what's happening here.

The rectangle for nsNativeThemeCocoa::eThemeGeometryTypeWindowButtons is determined by the position of a XUL element in the browser chrome which has -moz-appearance: -moz-window-button-box.

However, we're in full control of drawing these buttons. Things that could go wrong are:
 - We might be computing the wrong bounding rect for the window controls when setting up the drawing buffer for them. Can you check whether the rectangle returned by RectContainingTitlebarControls() is big enough, or whether the clipping already happens there?
 - If the drawing buffer (mTitlebarCGContext) is large enough, the buttons might get clipped when we draw them into that CGContext. You can check whether that's the case by dumping the contents of mTitlebarCGContext at the end of nsChildView::UpdateTitlebarCGContext(). (You can save the contents of a CGContext to a PNG file by creating a CGImageRef using CGBitmapContextCreateImage(context) and then following the code from https://stackoverflow.com/a/17510651/6740906 .)

Comment 5

[Mike Taylor [:miketaylr]]

Attachment: Screen Shot 2017-09-27 at 9.42.06 AM.png

Comment 7

[Marcia Knous [:marcia]]

Tracking 57+ for this visible UI issue.

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: img.png

(In reply to Markus Stange [:mstange] from comment #4)
> I'd like to understand more about what's happening here.
> 
> The rectangle for nsNativeThemeCocoa::eThemeGeometryTypeWindowButtons is
> determined by the position of a XUL element in the browser chrome which has
> -moz-appearance: -moz-window-button-box.
> 
> However, we're in full control of drawing these buttons. Things that could
> go wrong are:
>  - We might be computing the wrong bounding rect for the window controls
> when setting up the drawing buffer for them. Can you check whether the
> rectangle returned by RectContainingTitlebarControls() is big enough, or
> whether the clipping already happens there?

The NSRect has a height of 22.0f and a width that's equivalent to the width of the browser window. So this appears big enough.

>  - If the drawing buffer (mTitlebarCGContext) is large enough, the buttons
> might get clipped when we draw them into that CGContext. You can check
> whether that's the case by dumping the contents of mTitlebarCGContext at the
> end of nsChildView::UpdateTitlebarCGContext(). (You can save the contents of
> a CGContext to a PNG file by creating a CGImageRef using
> CGBitmapContextCreateImage(context) and then following the code from
> https://stackoverflow.com/a/17510651/6740906 .)

I have tried this, but I'm getting a solid grey image that's 64px high and 2048px wide (see attachment). The 64px could maybe be explained by the fact that I'm using a retina screen and the titlebar is 32px high (equivalent to 64px on a retina screen), but the image height doesn't increase if I select "Drag Space" in customization to make the titlebar larger. Not quite sure why the image is 2048px wide as the browser window is neither 1024 nor 2048px wide, nor does the screen have a resolution of 1024 or 2048. The bigger question though is why the image is simply solid grey and no titlebar elements are drawn.

I will keep looking into this.

Comment 9

[Markus Stange [:mstange]]

This has been fixed by the patch in bug 1400057.

Some more details on the problem and the fix:
nsChildView::UpdateTitlebarCGContext iterates over an array of NSViews that is supposed to contain the titlebar controls, i.e. the individual NSButtons for the window buttons, in order to paint them. However, with a "floating titlebar" (which seems to be the default when running on 10.13 and building with the 10.11 SDK), the NSView structure of the window has changed, so the array does not actually contain the buttons. Instead it contains an NSView that wraps the buttons somewhere deeper in the NSView hierarchy.
The patch in bug 1400057 fixes this bug because it causes the NSView hierarchy of the window to return to the flatter structure that was used in previous macOS releases. As a consequence, -[BaseWindow titlebarControls] finds the correct buttons.

In order to fix this properly, either nsChildView::UpdateTitlebarCGContext will need to recurse over the NSView structure, or -[BaseWindow titlebarControls] needs to gather the leaf NSViews into an array, or we need to make some other change that makes nsChildView::UpdateTitlebarCGContext completely unnecessary. The latter would be preferred.

Comment 10

[Markus Stange [:mstange]]

For future reference, here's what the NSView hierarchy looks like with a floating titlebar:

> [ D A  O  P    ] h=--- v=--- NSThemeFrame 0x1114c5180 "Nightly" f=(0,0,892,634) b=(-)
>   [ D A          ] h=--- v=--- NSView 0x1117e9560 f=(0,0,892,634) b=(-)
>     [ D AF O      #] h=--- v=--- ChildView 0x11a826c80 f=(0,0,892,634) b=(-)
>       ...
>   [ D A      W  #] h=-&- v=--- NSTitlebarContainerView 0x103cf0f80 f=(0,612,892,22) b=(-) => <_NSViewBackingLayer: 0x111465190>
>     [ D A      W   ] h=-&- v=-&- NSTitlebarView 0x1112671d0 f=(0,0,892,22) b=(-) => <_NSViewBackingLayer: 0x111465340>
>       [ D AF     w   ] h=--- v=--- _NSThemeCloseWidget 0x115632d10 "Button" f=(7,-2.5,14,16) b=(-) => <_NSViewBackingLayer: 0x11a117a90>
>       [ D AF     w   ] h=--- v=--- _NSThemeZoomWidget 0x115632de0 "Button" f=(47,-2.5,14,16) b=(-) => <_NSViewBackingLayer: 0x11a117bb0>
>       [ D AF     w   ] h=--- v=--- _NSThemeWidget 0x115632eb0 "Button" f=(27,-2.5,14,16) b=(-) => <_NSViewBackingLayer: 0x11a117ca0>
>       [  HAF   V W   ] h=--- v=&-- NSTextField 0x110d39ae0 "Nightly" f=(422,3,48,17) b=(-) => <NSTextLayer: 0x1114644c0>

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Carrying over the status of 57 from bug 1400057