871590 - Improve unified titlebar / toolbar handling

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

While working on bug 676241 I noticed some problems with the interaction between -moz-window-titlebar and toolbar.

Comment 1

[Markus Stange [:mstange]]

Attachment: part 1: add basic handling of -moz-window-titlebar

This patch consists of parts from the patch in bug 625989. It was already reviewed there and is already present on the UX repo as https://hg.mozilla.org/projects/ux/rev/35d68446e000 - these are the parts that can land on mozilla-central.

Comment 2

[Markus Stange [:mstange]]

Attachment: part 2: make it more robust

This patch improves how -moz-appearance:-moz-window-titlebar and -moz-appearance:toolbar determine the unified toolbar height and fixes problems that occured when the content area is extended into the titlebar.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 748866 [details] [diff] [review]
part 2: make it more robust

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

::: widget/cocoa/nsChildView.mm
@@ +2080,3 @@
>    for (uint32_t i = 0; i < aThemeGeometries.Length(); ++i) {
> +    const nsIWidget::ThemeGeometry& g = aThemeGeometries[i];
> +    if ((g.mWidgetType == NS_THEME_WINDOW_TITLEBAR) &&

Don't need the extra parens here

@@ +2099,5 @@
>      if ((g.mWidgetType == NS_THEME_MOZ_MAC_UNIFIED_TOOLBAR ||
>           g.mWidgetType == NS_THEME_TOOLBAR) &&
>          g.mRect.X() <= 0 &&
> +        g.mRect.XMost() >= aWindowWidth &&
> +        g.mRect.Y() <= aTitlebarBottom) {

Shouldn't aTitlebarBottom be unifiedToolbarbottom here, so we can keep extending the unified toolbar bottom with multiple toolbars?

Comment 4

[Markus Stange [:mstange]]

We could do that, but I don't think we want to. I don't know of any native apps which do it.
Here's a native example where the lower toolbar does not extend the upper one: http://dl.dropbox.com/u/2901861/Screenshots/Bildschirmfoto%202013-05-14%20um%2022.48.08.png

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 748866 [details] [diff] [review]
part 2: make it more robust

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

r+ with that

::: widget/cocoa/nsChildView.mm
@@ +2099,5 @@
>      if ((g.mWidgetType == NS_THEME_MOZ_MAC_UNIFIED_TOOLBAR ||
>           g.mWidgetType == NS_THEME_TOOLBAR) &&
>          g.mRect.X() <= 0 &&
> +        g.mRect.XMost() >= aWindowWidth &&
> +        g.mRect.Y() <= aTitlebarBottom) {

OK, can you at least set unifiedToolbarBottom to the max of unifiedToolbarBottom and g.mRect.YMost(), so that this doesn't depend on the order of aThemeGeometries?

Comment 6

[Markus Stange [:mstange]]

Good call, will do.

Comment 7

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/331851701133
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd49cfa6711

Comment 8

[Markus Stange [:mstange]]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2861d9fba479

Without bug 676241 this appears to cause failed assertions.

Comment 9

[Markus Stange [:mstange]]

Relanded on inbound together with bug 676241:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eca75a9574f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d158673711c

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1eca75a9574f
https://hg.mozilla.org/mozilla-central/rev/5d158673711c

Comment 11

[:Gijs (he/him)]

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