1402740 - modal sheets slide out from an odd place

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Cameron McCormack (:heycam)]

Not sure if this is a widget or XUL issue.  With the new Photon UI, modal dialog sheets appear to slide out from half way in the middle of the toolbar, whereas pre-Photon, they slide out from just under the toolbar.  Sliding out from just under the toolbar looks better IMO.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: pre-Photon screen shot

Comment 2

[Cameron McCormack (:heycam)]

Attachment: post-Photon screen shot

Comment 3

[Stephen A Pohl [:spohl]]

Did this happen before bug 1324892 landed?

Comment 4

[Cameron McCormack (:heycam)]

No, it looks fine as of a 2017-09-08 build.  But it also looks fine from a build four days later.  I will bisect.

Comment 5

[Cameron McCormack (:heycam)]

10:17.75 INFO: Last good revision: 4193c11e97aeab5a258d5499bf4a4d9177b27380
10:17.75 INFO: First bad revision: 938750d859b85600b84013bd6dea63db1c32d135
10:17.75 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4193c11e97aeab5a258d5499bf4a4d9177b27380&tochange=938750d859b85600b84013bd6dea63db1c32d135

Comment 6

[Stephen A Pohl [:spohl]]

Thanks!

Comment 7

[Stephen A Pohl [:spohl]]

Caused by bug 1401297.

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Patch

As of bug 1401297, we no longer need to offset the sheet by the titlebar height because when drawsContentsIntoWindowFrame is true, we actually hide the titlebar instead of drawing on top of it.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8912678 [details] [diff] [review]
Patch

Review of attachment 8912678 [details] [diff] [review]:
-----------------------------------------------------------------

I can confirm this fixes the issue btw.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8912678 [details] [diff] [review]
Patch

Review of attachment 8912678 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsCocoaWindow.mm
@@ +3600,1 @@
>    return NSMaxY(frameRect) - NSMaxY(originalContentRect);

Will this return zero now?

Comment 11

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #10)
> Comment on attachment 8912678 [details] [diff] [review]
> Patch
> 
> Review of attachment 8912678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/cocoa/nsCocoaWindow.mm
> @@ +3600,1 @@
> >    return NSMaxY(frameRect) - NSMaxY(originalContentRect);
> 
> Will this return zero now?

When drawsContentsIntoWindowFrame is true, yes.

Comment 12

[Markus Stange [:mstange]]

I'm worried about what that means for the mode where drawsContentsIntoWindowFrame is true and wantsTitleDrawn is also true. However, that mode seems a bit broken already; I'm not sure if that's due to one of your patches or due to frontend changes (e.g. bug 1392219). Here's how to get into that mode: In the toolbar customization dialog, check "Title Bar", and then select a theme that's not one of the default themes. In that mode, we should draw a title (this seems buggy at the moment), and double-clicking in the titlebar should cause us to zoom the window, or to minimize it depending on OS version and system settings.

Comment 13

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #12)
> I'm worried about what that means for the mode where
> drawsContentsIntoWindowFrame is true and wantsTitleDrawn is also true.
> However, that mode seems a bit broken already; I'm not sure if that's due to
> one of your patches or due to frontend changes (e.g. bug 1392219). Here's
> how to get into that mode: In the toolbar customization dialog, check "Title
> Bar", and then select a theme that's not one of the default themes. In that
> mode, we should draw a title (this seems buggy at the moment), and
> double-clicking in the titlebar should cause us to zoom the window, or to
> minimize it depending on OS version and system settings.

I just tried this and at first it looked indeed as if this wasn't working. However, I noticed that after switching themes, hiding the title bar and showing it again in the toolbar customization dialog, the window title appears again. I've checked with the 9/11 nightly (two days after we made the SDK switch) and this bug was already present. It did not occur with the 10.7 SDK. I will file a separate bug, but the patch here does not seem to make this existing problem any worse.

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch

I was able to track down the issue with the window title disappearing in non-default themes. I will file a separate bug to fix this. However, once the window titles are fixed, the previous patch here would cause the window titles to disappear again as you feared in comment 12. I've removed the changes to -(CGFloat)titlebarHeight, as they're not necessary to fix the issue with the modal sheet. I've confirmed that this patch works correctly with the title bar displayed and hidden, in default and non-default themes.

Sending back for a final review.

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8913323 [details] [diff] [review]
Patch

Review of attachment 8913323 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! I'm always relieved when it turns out that my concerns are warranted.

Comment 16

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb5fe61c3e648087af16b2571c04c28e55143ba
Bug 1402740: Position modal sheets correctly on macOS now that titlebars are being hidden by bug 1401297. r=mstange

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfb5fe61c3e6

Comment 18

[Stephen A Pohl [:spohl]]

Comment on attachment 8913323 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1401297
[User impact if declined]: Bad UI - Modal sheets wouldn't display in the right place.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Isolated, well understood change specific to modal sheets on macOS only.
[String changes made/needed]: none

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913323 [details] [diff] [review]
Patch

Fix a recent regression, taking it.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8d89adbab6e6

Comment 21

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using old Nightly from 2017-09-24, verified that the issue is not reproducible anymore using Firefox 57 beta 12 and latest Nightly 58.0a1 on macOS 10.13.