Closed Bug 1120917 Opened 5 years ago Closed 5 years ago

Make it possible to hide the window controls (osx)

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(2 files, 5 obsolete files)

Attached image screenshot
We want it be possible to hide the window controls.

I managed to do so by setting the rect of the buttons to 0x0:

> [[win standardWindowButton:NSWindowZoomButton] setFrame:NSZeroRect];

in widget/cocoa/nsChildView.mm (see screenshot).

Ideally, we want that to be controlled in CSS, from a HTML page. Maybe a pseudo element (::-moz-window-controls). It would be great if could set use `transform:translateY()` to move buttons out of the window instead of hiding them (we want to be able to show them dynamically, with an animation if possible). Or maybe use `opacity`.
Blocks: graphene
Not sure yet this is the way we want to do this, but this works. Here I'm just removing the constraints on the positions of the window buttons.
Comment on attachment 8551181 [details] [diff] [review]
allow negative margins for window-button-box

Markus, we'd like to be able to hide/move the window controls on Mac. Removing the NSIsEmptyRect restriction appears to work. Do you think it's reasonable thing to do?
Attachment #8551181 - Flags: feedback?(mstange)
Summary: Make it possible to hide the window controls → Make it possible to hide the window controls (osx)
Comment on attachment 8551181 [details] [diff] [review]
allow negative margins for window-button-box

I feared that this would cause problems with third-party themes that draw in the titlebar and don't use -moz-window-buttonbox anywhere. But the good news is that the rule that sets -moz-window-buttonbox for the browser window is in content CSS, so independent of the chosen theme, so it looks like this will work.
Attachment #8551181 - Flags: feedback?(mstange) → feedback+
Comment on attachment 8551181 [details] [diff] [review]
allow negative margins for window-button-box

Actually, I don't understand why this patch works. Wouldn't this put the buttons at (0, 0) right in the top left corner?
Attachment #8551181 - Flags: feedback+
Attached patch v1 (obsolete) — Splinter Review
(In reply to Markus Stange [:mstange] from comment #4)
> Actually, I don't understand why this patch works. Wouldn't this put the
> buttons at (0, 0) right in the top left corner?

In which condition?

As far as I understand, if there's a native titlebar (chromemargin=-1), it draws properly on the expected spot (top left corner, default position). If chromemargin=0, it draws the controls according to `moz-window-button-box` margins.
Assignee: nobody → paul
Attachment #8551181 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8555665 - Flags: review?(mstange)
(In reply to Paul Rouget [:paul] from comment #5)
> Created attachment 8555665 [details] [diff] [review]
> v1
> 
> (In reply to Markus Stange [:mstange] from comment #4)
> > Actually, I don't understand why this patch works. Wouldn't this put the
> > buttons at (0, 0) right in the top left corner?
> 
> In which condition?

When you paint in the titlebar and hide the -moz-window-button-box. Then nsChildView::UpdateThemeGeometries would be called without a window button box entry, FindFirstRectOfType(aThemeGeometries, NS_THEME_WINDOW_BUTTON_BOX) would return nsIntRect() (which is 0,0,0,0), and that gets assigned to mWindowButtonsRect, after a coordinate conversion. So windowButtonsPositionWithDefaultPosition will return (0,0) translated into window space. Maybe you're saved by that being an upside down coordinate system, and the buttons get pushed just off the top of the window?
(In reply to Markus Stange [:mstange] from comment #6)
> Maybe you're saved by that being an upside down coordinate
> system, and the buttons get pushed just off the top of the window?

Yep. Exactly what happens. mWindowButtonsRect.origin.y is always equal to the exact height of the window.

So what would you recommend to hide the controls?
The position of an empty rect is usually not meaningful information, so we shouldn't implicitly treat it as meaningful here. I think you should have an explicit NSIsEmptyRect check, and explicitly return a position outside the window if that's the case. It'll have the same result but it'll be much clearer why it works.
Attached patch v2 (obsolete) — Splinter Review
Same result, but more explicit.
Attachment #8556290 - Flags: review?(mstange)
Attachment #8555665 - Attachment is obsolete: true
Attachment #8555665 - Flags: review?(mstange)
Comment on attachment 8556290 [details] [diff] [review]
v2

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

::: widget/cocoa/nsCocoaWindow.mm
@@ +3271,5 @@
> +  if ([self drawsContentsIntoWindowFrame]) {
> +    if (NSIsEmptyRect(mWindowButtonsRect)) {
> +      // Move window buttons outside of the window.
> +      NSButton* closeButton = [self standardWindowButton:NSWindowCloseButton];
> +      return NSMakePoint(0, -1 * [closeButton frame].size.height);

return NSMakePoint(0, -[closeButton frame].size.height);

Can you check whether closeButton is ever null when this gets called? I could imagine that we don't have a closeButton the first time their position is queried.

@@ +3272,5 @@
> +    if (NSIsEmptyRect(mWindowButtonsRect)) {
> +      // Move window buttons outside of the window.
> +      NSButton* closeButton = [self standardWindowButton:NSWindowCloseButton];
> +      return NSMakePoint(0, -1 * [closeButton frame].size.height);
> +    } else {

No else after return, please.
Attached patch v3 (obsolete) — Splinter Review
Attachment #8556290 - Attachment is obsolete: true
Attachment #8556290 - Flags: review?(mstange)
Attachment #8556304 - Flags: review?(mstange)
(In reply to Markus Stange [:mstange] from comment #10)
> Can you check whether closeButton is ever null when this gets called?

Sorry, what I meant is: can you please debug locally whether that's ever the case? If it is, maybe we should use a different approach. For example you could use the window size:

return NSMakePoint(0, -[self frame].size.height);

Actually, this looks so simple that we should just use that, regardless of whether closeButton can ever be null.
Attached patch v3 (obsolete) — Splinter Review
Attachment #8556304 - Attachment is obsolete: true
Attachment #8556304 - Flags: review?(mstange)
Attachment #8556313 - Flags: review?(mstange)
Comment on attachment 8556313 [details] [diff] [review]
v3

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

::: widget/cocoa/nsCocoaWindow.mm
@@ +3270,5 @@
>  {
> +  if ([self drawsContentsIntoWindowFrame]) {
> +    if (NSIsEmptyRect(mWindowButtonsRect)) {
> +      // Move window buttons outside of the window.
> +      return NSMakePoint(0, -[self frame].size.height);

Lose the minus, and add a comment saying that the position is in non-flipped window coordinates, i.e. larger numbers towards the top of the window.

Sorry for the large number of iterations, it would probably have been easier if I had created a corrected version of the patch myself. Sometimes it's hard to express what I have in mind.
Attachment #8556313 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #14)
> Comment on attachment 8556313 [details] [diff] [review]
> v3
> 
> Review of attachment 8556313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/cocoa/nsCocoaWindow.mm
> @@ +3270,5 @@
> >  {
> > +  if ([self drawsContentsIntoWindowFrame]) {
> > +    if (NSIsEmptyRect(mWindowButtonsRect)) {
> > +      // Move window buttons outside of the window.
> > +      return NSMakePoint(0, -[self frame].size.height);
> 
> Lose the minus, and add a comment saying that the position is in non-flipped
> window coordinates, i.e. larger numbers towards the top of the window.

Ok!

> Sorry for the large number of iterations, it would probably have been easier
> if I had created a corrected version of the patch myself. Sometimes it's
> hard to express what I have in mind.

Np. Thanks for you help.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/672cdff6ba49
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1130746
Depends on: 1216245
You need to log in before you can comment on or make changes to this bug.