Closed Bug 1533562 Opened 6 years ago Closed 6 years ago

Prepare unified titlebar drawing for CoreAnimation-backed windows

Categories

(Core :: Widget: Cocoa, enhancement, P1)

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(6 files)

Bug 1533562 - Remove synchronous repaint capability of setTitlebarNeedsDisplayInRect:sync: and also remove the rect parameter because we always pass the same value to it. r?spohl
47 bytes, text/x-phabricator-request
Details | Review
Bug 1533562 - Remove override of -[NSThemeFrame _unifiedToolbarFrame]. r?spohl
47 bytes, text/x-phabricator-request
Details | Review
Bug 1533562 - Always make the content view of ToolbarWindows cover the entire window. r?spohl
47 bytes, text/x-phabricator-request
Details | Review
Bug 1533562 - Implement titlebar gradient drawing with a new TitlebarGradientView. r?spohl
47 bytes, text/x-phabricator-request
Details | Review
Bug 1533562 - Remove code that deals with non-rounded bottom corners on regular windows. r?spohl
47 bytes, text/x-phabricator-request
Details | Review
Bug 1533562 - Do not move the TitlebarGradientView from a ToolbarWindow into a BorderlessWindow when hiding the window chrome. r?spohl
47 bytes, text/x-phabricator-request
Details | Review

Firefox still uses the "unified titlebar + toolbar" look in a few places:

  1. In the default theme, if you check "Title Bar" in the customization page, you will have a unified gradient from the titlebar into the tab bar.
  2. Utility windows such as the bookmarks library and the page info window also have the unified toolbar look.

In these windows, the gradient is drawn in two pieces: Cocoa draws the piece in the titlebar, and Gecko draws the piece below the titlebar. That's because our ChildView doesn't extend into the titlebar in these windows, so Gecko cannot draw the entire thing.
Both places need to know about the complete length of the gradient in order to make it look correct.

Currently, the gradient in the titlebar knows about the correct total length because we override -[NSThemeFrame _unifiedToolbarFrame]. However, this override only works in non-CoreAnimation-backed windows; once a window uses CoreAnimation, the gradient is implemented through a CAGradientLayer whose size is not based on what we return from _unifiedToolbarFrame.
So our current override of _unifiedToolbarFrame won't work in a CoreAnimation world. We will need to find a different solution.

The solution I'm going with is:

  • Always make the window's content view cover the entire height of the window. The window's ChildView will still cover only the part that it's covering at the moment. The titlebar rect will be contained in the content view's frame but not in the ChildView's frame.
  • Set our window's titlebarAppearsTransparent property to YES.
  • Fill the titlebar-sized gap within the content view with a new NSView, call it TitlebarGradientView. Draw the titlebar gradient in that view's drawRect method.

The titlebarAppearsTransparent property is only supported on 10.10 and up; Firefox still runs on 10.9, so this method will not work on 10.9. I think that's ok; 10.9 users can live with a small visual imperfection.

The synchronous paint was only needed a long time ago when we were calling this
method during drawRect. We're not doing that any more, we usually call it from
viewWillDraw now. But even at the time, forcing a synchronous paint within
a paint was extremely sketchy, so best just to remove the code.

This override has no effect in CoreAnimation-backed windows. The upcoming
patches will implement an alternative approach.

Depends on D22643

Priority: -- → P1

Rounded bottom corners have been the default since 10.7.

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/fcd1387a1dee Remove synchronous repaint capability of setTitlebarNeedsDisplayInRect:sync: and also remove the rect parameter because we always pass the same value to it. r=spohl https://hg.mozilla.org/integration/autoland/rev/6b5804bc63c3 Remove code that deals with non-rounded bottom corners on regular windows. r=spohl https://hg.mozilla.org/integration/autoland/rev/788b5dbea4e8 Remove override of -[NSThemeFrame _unifiedToolbarFrame]. r=spohl https://hg.mozilla.org/integration/autoland/rev/048e88ed260d Always make the content view of ToolbarWindows cover the entire window. r=spohl https://hg.mozilla.org/integration/autoland/rev/8d21cbdfdea7 Implement titlebar gradient drawing with a new TitlebarGradientView. r=spohl
Backout by rgurzau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d5170a12e11d Backed out 5 changesets for osx reftest failures on a CLOSED TREE.

Backed out 5 changesets (bug 1533562) for osx reftest failures on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/d5170a12e11d577d912de4c77bae54ea41709441

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=8d21cbdfdea7110dab696be065a4cfc345702d94&selectedJob=236810812&failure_classification_id=2

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236810812&repo=autoland&lineNumber=1356

Log snippet:

05:21:22 INFO - _BlockUntilNextEventMatchingListInModeWithFilter (in HIToolbox) + 71
05:21:22 INFO - _DPSNextEvent (in AppKit) + 978
05:21:22 INFO - -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 346
05:21:22 INFO - 0x0000000108770bae (in XUL) + 174
05:21:22 INFO - -[NSApplication run] (in AppKit) + 594
05:21:22 INFO - 0x000000010877257f (in XUL) + 559
05:21:22 INFO - 0x0000000109e81e59 (in XUL) + 121
05:21:22 INFO - 0x000000010a008455 (in XUL) + 3365
05:21:22 INFO - 0x000000010a00921f (in XUL) + 1151
05:21:22 INFO - 0x000000010a00a1cc (in XUL) + 156
05:21:22 INFO - 0x0000000104493f10 (in firefox) + 976
05:21:22 INFO - start (in libdyld.dylib) + 1
05:21:22 INFO - 0x00000005 (in firefox)
05:21:22 INFO - ++DOMWINDOW == 4 (0x11b0b0000) [pid = 791] [serial = 4] [outer = 0x11b031020]
05:21:22 INFO - REFTEST TEST-PASS | data:text/html, | (LOAD ONLY)
05:21:22 INFO - REFTEST TEST-END | data:text/html,
05:21:22 INFO - ++DOMWINDOW == 5 (0x11b0b1800) [pid = 791] [serial = 5] [outer = 0x11b031020]
05:21:22 INFO - REFTEST TEST-UNEXPECTED-FAIL | data:text/html, | assertion count 1 is more than expected 0 assertions
05:21:22 INFO - REFTEST TEST-START | data:text/html,PASS
05:21:22 INFO - REFTEST TEST-LOAD | data:text/html,PASS | 1 / 3704 (0%)
05:21:22 INFO - ++DOMWINDOW == 6 (0x11b0b2000) [pid = 791] [serial = 6] [outer = 0x11b031020]
05:21:22 INFO - ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /var/folders/_n/1r02c6ys0mv7x2gd6wbkj33400000x/T/tmpXgC5A9.mozrunner/runreftest_leaks_tab_pid797.log
05:21:22 INFO - REFTEST TEST-PASS | data:text/html,PASS | (LOAD ONLY)
05:21:22 INFO - REFTEST TEST-END | data:text/html,PASS

Flags: needinfo?(mstange)

There were two problems with these patches.
The first was in the part "Always make the content view of ToolbarWindows cover the entire window." and would cause us to create windows that were too small, because I had missed one caller of [mWindow frameRectForContentRect:] where I didn't account for the fact that the returned value no longer adds the titlebar size. I've now added two convenience methods "frameRectForChildViewRect" and "childViewRectForFrameRect" which do what the old callers would have expected.
The second problem was the fact that the new TitlebarGradientView would sometimes end up in a window that was not a ToolbarWindow, but a BorderlessWindow. This happened via the contentView reparenting mechanism that HideWindowChrome employs. I've added a new patch to the patch stack which fixes this problem.

Flags: needinfo?(mstange)
Blocks: 1542454
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/e3fc54ebcd15 Remove synchronous repaint capability of setTitlebarNeedsDisplayInRect:sync: and also remove the rect parameter because we always pass the same value to it. r=spohl https://hg.mozilla.org/integration/autoland/rev/5fae2f233aa0 Remove code that deals with non-rounded bottom corners on regular windows. r=spohl https://hg.mozilla.org/integration/autoland/rev/3a3b4d52e10a Remove override of -[NSThemeFrame _unifiedToolbarFrame]. r=spohl https://hg.mozilla.org/integration/autoland/rev/498cd34eea78 Always make the content view of ToolbarWindows cover the entire window. r=spohl https://hg.mozilla.org/integration/autoland/rev/8970cdb3c04a Implement titlebar gradient drawing with a new TitlebarGradientView. r=spohl https://hg.mozilla.org/integration/autoland/rev/b4440b1833ec Do not move the TitlebarGradientView from a ToolbarWindow into a BorderlessWindow when hiding the window chrome. r=spohl

Backed out 9 changesets (bug 1542454, bug 1533562) for failing at /browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/73e8dcb8be078f4d66058019f70c2cd0e4526a6b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=b4440b1833ece120e7680763b55168913b18e0d4&selectedJob=241874688

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241874688&repo=autoland&lineNumber=4453

Log snippet:

21:03:03 INFO - TEST-START | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js
21:03:49 INFO - TEST-INFO | started process screencapture
21:03:49 INFO - TEST-INFO | screencapture: exit 0
21:03:49 INFO - Buffered messages logged at 21:03:03
21:03:49 INFO - Entering test bound test_on_created_navigation_target_from_context_menu
21:03:49 INFO - Buffered messages logged at 21:03:04
21:03:49 INFO - Extension loaded
21:03:49 INFO - Console message: Warning: attempting to write 36421 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
21:03:49 INFO - Open link in a new tab from the context menu
21:03:49 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | found contextMenu item for "Open Link in New Tab" -
21:03:49 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | Got the expected tabId property -
21:03:49 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | Got the expected sourceTabId property -
21:03:49 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | Got the expected sourceFrameId property -
21:03:49 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | Got the expected url property -
21:03:49 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | Got the expected webNavigation.onCompleted tabId property -
21:03:49 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | Got the expected webNavigation.onCompleted url property -
21:03:49 INFO - Open link in a new window from the context menu
21:03:49 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | found contextMenu item for "Open Link in New Window" -
21:03:49 INFO - Console message: OpenGL compositor Initialized Succesfully.
21:03:49 INFO - Version: 2.1 INTEL-10.6.33
21:03:49 INFO - Vendor: Intel Inc.
21:03:49 INFO - Renderer: Intel Iris OpenGL Engine
21:03:49 INFO - FBO Texture Target: TEXTURE_2D
21:03:49 INFO - Buffered messages finished
21:03:49 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | Test timed out -
21:03:49 INFO - Not taking screenshot here: see the one that was previously logged
21:03:49 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_contextmenu.js | message queue is empty - Got ["tabsOnCreated","webNavOnCreated","webNavOnCompleted"], expected []
21:03:49 INFO - Stack trace:
21:03:49 INFO - chrome://mochikit/content/browser-test.js:test_is:1325
21:03:49 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:27
21:03:49 INFO - chrome://mochikit/content/browser-test.js:nextTest:715
21:03:49 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1212
21:03:49 INFO - setTimeout handler*chrome://mochikit/content/browser-test.js:Tester_execTest:1174
21:03:49 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1005
21:03:49 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
21:03:49 INFO - Not taking screenshot here: see the one that was previously logged

Flags: needinfo?(mstange)

There was a bug in the patch titled "Always make the content view of ToolbarWindows cover the entire window.": -[ToolbarWindow initWithContentRect:styleMask:backing:defer:] was creating windows whose frame size matched the specified content size, but that's not what the caller wanted: the caller wants the aContentSize argument to specify the size of the ChildView of the window, i.e. the titlebar size should be added on the top. I've renamed the argument to aChildViewRect and added the necessary adjustment. This fixed the test timeout.

Here are more details on what exactly caused the test to time out:

When the window's frame size doesn't match the expected frame size, we hit this code:

  // Make sure that the content rect we gave has been honored.
  NSRect wantedFrame = [mWindow frameRectForChildViewRect:contentRect];
  if (!NSEqualRects([mWindow frame], wantedFrame)) {
    // This can happen when the window is not on the primary screen.
    [mWindow setFrame:wantedFrame display:NO];
  }

This code runs after we set the window delegate on the window, so the delegate's resize handler fires, and that handler calls RollUpPopups(). This means that opening a browser window would now automatically close context menus.

The test was opening a window from a context menu and then closing the context menu. When closing the context menu, it was expecting a popuphidden event to fire after the manual closeContextMenu() call, and not earlier. With the bad patch, the popuphidden event fired earlier, during the menuItem.click() call.

Flags: needinfo?(mstange)
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/68559438c818 Remove synchronous repaint capability of setTitlebarNeedsDisplayInRect:sync: and also remove the rect parameter because we always pass the same value to it. r=spohl https://hg.mozilla.org/integration/autoland/rev/64720021f45c Remove code that deals with non-rounded bottom corners on regular windows. r=spohl https://hg.mozilla.org/integration/autoland/rev/710d3c0129de Remove override of -[NSThemeFrame _unifiedToolbarFrame]. r=spohl https://hg.mozilla.org/integration/autoland/rev/00ffda400dc5 Always make the content view of ToolbarWindows cover the entire window. r=spohl https://hg.mozilla.org/integration/autoland/rev/02adcc70efbe Implement titlebar gradient drawing with a new TitlebarGradientView. r=spohl https://hg.mozilla.org/integration/autoland/rev/1976a614f8ce Do not move the TitlebarGradientView from a ToolbarWindow into a BorderlessWindow when hiding the window chrome. r=spohl

Green-ish try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4d12a3f3e86449dad56fcfe86e19a6261fa8719

The latest problem was the fact that the patch that touched the rounded bottom corner drawing caused the reftest snapshots to be clipped to rounded corners. However, the reftest window is a window with square corners. So I put some of the rounded corner checks back where they were before.

Flags: needinfo?(mstange)
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/ffbfe4a683f6 Remove synchronous repaint capability of setTitlebarNeedsDisplayInRect:sync: and also remove the rect parameter because we always pass the same value to it. r=spohl https://hg.mozilla.org/integration/autoland/rev/19d8e4a62f4d Remove code that deals with non-rounded bottom corners on regular windows. r=spohl https://hg.mozilla.org/integration/autoland/rev/c252f4c4a3b4 Remove override of -[NSThemeFrame _unifiedToolbarFrame]. r=spohl https://hg.mozilla.org/integration/autoland/rev/931cc79f0768 Always make the content view of ToolbarWindows cover the entire window. r=spohl https://hg.mozilla.org/integration/autoland/rev/382967c743c8 Implement titlebar gradient drawing with a new TitlebarGradientView. r=spohl https://hg.mozilla.org/integration/autoland/rev/7b2d2da6fba8 Do not move the TitlebarGradientView from a ToolbarWindow into a BorderlessWindow when hiding the window chrome. r=spohl
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: