modal sheets slide out from an odd place

VERIFIED FIXED in Firefox 57

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: heycam, Assigned: spohl)

Tracking

({regression})

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ verified, firefox58 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

2 years ago
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.
Reporter

Comment 1

2 years ago
Assignee

Comment 3

2 years ago
Did this happen before bug 1324892 landed?
Flags: needinfo?(cam)
Reporter

Comment 4

2 years ago
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.
Flags: needinfo?(cam)
Reporter

Comment 5

2 years ago
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
Assignee

Comment 6

2 years ago
Thanks!
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED

Updated

2 years ago
Keywords: regression
Assignee

Comment 7

2 years ago
Caused by bug 1401297.
Blocks: 1401297
Assignee

Comment 8

2 years ago
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8912678 - Flags: review?(mstange)
Comment on attachment 8912678 [details] [diff] [review]
Patch

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

I can confirm this fixes the issue btw.
Attachment #8912678 - Flags: feedback+
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?
Assignee

Comment 11

2 years ago
(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.
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.
Assignee

Comment 13

2 years ago
(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.
Attachment #8912678 - Flags: review?(mstange) → review+
Assignee

Comment 14

2 years ago
Posted patch PatchSplinter Review
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.
Attachment #8912678 - Attachment is obsolete: true
Attachment #8913323 - Flags: review?(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.
Attachment #8913323 - Flags: review?(mstange) → review+
Assignee

Comment 16

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dfb5fe61c3e6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Comment 18

2 years ago
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
Attachment #8913323 - Flags: approval-mozilla-beta?
Comment on attachment 8913323 [details] [diff] [review]
Patch

Fix a recent regression, taking it.
Attachment #8913323 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.