Improve unified titlebar / toolbar handling

RESOLVED FIXED in mozilla24

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Depends on: 1 bug)

Trunk
mozilla24
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
While working on bug 676241 I noticed some problems with the interaction between -moz-window-titlebar and toolbar.
(Assignee)

Comment 1

5 years ago
Created attachment 748859 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
Created attachment 748866 [details] [diff] [review]
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.
Attachment #748866 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #748859 - Attachment description: add basic handling of -moz-window-titlebar → part 1: add basic handling of -moz-window-titlebar
(Assignee)

Updated

5 years ago
Assignee: nobody → mstange
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?
(Assignee)

Comment 4

5 years ago
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 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?
Attachment #748866 - Flags: review?(roc) → review+
(Assignee)

Comment 6

5 years ago
Good call, will do.
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/331851701133
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd49cfa6711
(Assignee)

Comment 8

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

Without bug 676241 this appears to cause failed assertions.
(Assignee)

Comment 9

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/1eca75a9574f
https://hg.mozilla.org/mozilla-central/rev/5d158673711c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 880554
Depends on: 880620
(Assignee)

Updated

5 years ago
Depends on: 890361

Comment 11

4 years ago
https://hg.mozilla.org/mozilla-central/rev/e540eb3276a1
You need to log in before you can comment on or make changes to this bug.