Animate arrow panel animations on macOS in a way that does not require window repaints

RESOLVED FIXED in Firefox 56

Status

()

Core
Widget: Cocoa
P1
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla56
All
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 affected, firefox56 fixed)

Details

(Whiteboard: [photon-visual][p1][tpi:+])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 6 obsolete attachments)

58 bytes, text/x-review-board-request
spohl
: review+
Details | Review
59 bytes, text/x-review-board-request
dao
: review+
Details | Review
(Assignee)

Description

a year ago
Our current arrow panel animation is janky and flickery, see bug 1036929 and bug 1175941.

This is because we use a CSS transform animation on a non-accelerated window with a shadow. Every frame of the animation is painted on the main thread, and after each paint we invalidate the window's shadow so that it goes around the new shape.

What we really want is to paint once, calculate the shadow once, and then use a transform that applies on both the panel and the shadow.
There are two ways to do this:
 - Option (1) is to hardware accelerate popups, and draw the shadow ourselves, so that both the the panel content and the shadow are in one retained layer and the compositor only needs to update the transform.
 - Option (2) is to stop animating the window contents, and instead apply the transform in the window server. This will transform both the window and the native window shadow.

I think we should do option (2). (Option 1 has the problem that the shadow extends beyond the window's bounds so we'd have to do funny business with hit testing in the shadow, and panel positioning at the screen edges.)

I wrote some patches to apply the native arrow panel animation to our panel windows. The patches use a private API to set the animation type, and then Cocoa creates an NSWindowTransformAnimation that sends transform updates to the window server.
(Assignee)

Comment 1

a year ago
Created attachment 8777095 [details]
Bug 1291457 - Don't use a CSS animation for animating arrow panels on Mac.

Review commit: https://reviewboard.mozilla.org/r/68694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68694/
(Assignee)

Comment 2

a year ago
Created attachment 8777096 [details]
Bug 1291457 - Set transform-origin on arrow panels even if we don't use the CSS transform animation.

This will allow nsMenuPopupFrame::LayoutPopup to compute the origin point around which the native arrow panel animation should happen.

Review commit: https://reviewboard.mozilla.org/r/68696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68696/
(Assignee)

Comment 3

a year ago
Created attachment 8777097 [details]
Bug 1291457 -  Allow the panel's NSWindow class to specify a native animation type for arrow panel opening and closing, and overriding that animation's origin.

This uses private APIs. The public API for native arrow panels doesn't map well to Gecko.

Review commit: https://reviewboard.mozilla.org/r/68698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68698/
(Assignee)

Comment 4

a year ago
Created attachment 8777098 [details]
Bug 1291457 - Add an nsIWidget API so that layout can declare a widget as being an arrow panel.

Review commit: https://reviewboard.mozilla.org/r/68700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68700/
(Assignee)

Comment 5

a year ago
Created attachment 8777099 [details]
Bug 1291457 - Call nsIWidget::SetIsArrowPanel for panels with type="arrow", and supply the computed value of the transform-origin CSS property as the panel's arrow tip location.

This does not work for the bookmarks popup because that popup is not type="arrow" and reimplements a bunch of arrowpanel stuff itself.

Review commit: https://reviewboard.mozilla.org/r/68702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68702/
(Assignee)

Comment 6

a year ago
Created attachment 8777100 [details]
Bug 1291457 - Remove shadow invalidation code.

Invalidating a window's shadow is really slow and leads to flickering. Now that
arrow panels don't use CSS animations any more, their shape stays the same
after the first paint, so we don't need the shadow invalidation functionality
for them any more. And as far as I know, we don't use transparent popups with
changing shapes anywhere else.
The system still updates the shadow if the popup window actually resizes. But
not when its size stays the same and only what we draw in the content is
updated.

Review commit: https://reviewboard.mozilla.org/r/68704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68704/
(Assignee)

Comment 7

a year ago
Created attachment 8777101 [details]
Bug 1291457 - Remove this shadow because it does not belong.

Review commit: https://reviewboard.mozilla.org/r/68706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68706/
(Assignee)

Comment 8

a year ago
Remaining work:
 - The comment at the top of nsDisplayTransform::GetDeltaToTransformOrigin needs to be addressed.
 - The bookmarks arrow panel does not animate, because it does not use type="arrow". This panel needs to be detected as an arrow panel by the code in nsMenuPopupFrame.cpp, not sure how.
 - If an arrow panel is open, clicking the button that opened it another time will re-open it with the animation, instead of just hiding it. This needs to be fixed.
(Assignee)

Updated

a year ago
Depends on: 1291469
Priority: -- → P2

Updated

a year ago
Whiteboard: tpi:+
(Assignee)

Updated

a year ago
Blocks: 1208437
(In reply to Markus Stange [:mstange] from comment #3)
> Created attachment 8777097 [details]
> Bug 1291457 -  Allow the panel's NSWindow class to specify a native
> animation type for arrow panel opening and closing, and overriding that
> animation's origin.
> 
> This uses private APIs. The public API for native arrow panels doesn't map
> well to Gecko.

Is the plan to still use these private APIs, or do we want to file requests for public APIs with Apple?
Flags: needinfo?(mstange)
(Assignee)

Comment 10

7 months ago
Not sure. It would be virtuous to at least find out how much work it would be for us to use the official APIs, before asking for other APIs.
Flags: needinfo?(mstange)

Updated

5 months ago
Blocks: 1348294
Markus, since bug 1348294/blurring arrow panels is (ideally) part of the Photon redesign project, do you think you'll be able to pick this up again anytime soon?

Thanks!
Flags: needinfo?(mstange)
(Assignee)

Comment 12

5 months ago
I can't promise I'll be able to pick this up anytime soon. The patches are in a good enough state that somebody else should be able to push them to completion, though. Items two and three from comment 8 should be worked on next. I can help with item 1.
Flags: needinfo?(mstange)

Updated

5 months ago
Priority: P2 → P1
Whiteboard: tpi:+ → [tpi:+][photon]

Updated

4 months ago
Whiteboard: [tpi:+][photon] → [tpi:+][photon-visual]

Updated

4 months ago
Blocks: 1296507

Updated

4 months ago
Flags: qe-verify-
Priority: P1 → P2

Updated

4 months ago
Whiteboard: [tpi:+][photon-visual] → [photon-visual][p1][tpi:+]

Comment 13

3 months ago
Hi Markus,

I looks like this fix is required to land Out of Process (see https://bugzilla.mozilla.org/show_bug.cgi?id=1356317#c15 and https://bugzilla.mozilla.org/show_bug.cgi?id=1356317#c21).  Is there any chance of this landing in the very near future?  OOP needs as long on Nightly as possible, since OOP is a big piece of work.

I wasn't sure who is triaging / assigned to this bug, but as a blocker for shipping OOP on Mac it should be a P1.  

Out of Process (bug 1190679) is a tracked performance gain (quantum flow) and P1 for webextensions. Out of Process code is ready to land, as soon as these last few impacted bugs are resolved.
Flags: needinfo?(mstange)
(In reply to :shell escalante from comment #13)
> Hi Markus,
> 
> I looks like this fix is required to land Out of Process (see
> https://bugzilla.mozilla.org/show_bug.cgi?id=1356317#c15 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1356317#c21).  Is there any
> chance of this landing in the very near future?  OOP needs as long on
> Nightly as possible, since OOP is a big piece of work.
> 
> I wasn't sure who is triaging / assigned to this bug, but as a blocker for
> shipping OOP on Mac it should be a P1.  
> 
> Out of Process (bug 1190679) is a tracked performance gain (quantum flow)
> and P1 for webextensions. Out of Process code is ready to land, as soon as
> these last few impacted bugs are resolved.

This bug has been on my radar and I could shift other quantum flow and photon work around to focus on this first. However, can you clarify bug 1356317 comment 24? From that comment, I got the impression that Mac was not part of the Quantum release criteria and OOP on Mac could be deferred until after 57, which means that I might want to continue focusing on other quantum flow bugs that affect Windows and/or all platforms instead.
Flags: needinfo?(mstange) → needinfo?(sescalante)

Comment 15

3 months ago
Hi Stephen,

I stand corrected (verified with andym).  Can you put this on your radar, but relative to higher priority Windows work.

It does block enabling Out of Process webextensions from OSX - and may creep up as a higher priority if there are issues found from not having OOP enabled - but you are right with QF prioritization. 

When you do get to it, this fix will unblock a very good perf gain for everyone with OSX, since everyone uses WE with screenshots (starting in 55).
Flags: needinfo?(sescalante)
I wanted to note that this is not only blocking OOP WE but also bug 1348294 which is Photon MVP, so I'd give it a high priority.
(Assignee)

Comment 17

3 months ago
I am going to pick this bug back up. I have a slightly different approach in mind now.

Instead of using the animation provided by the system, which we have little control over, I'd like to add APIs that let you change the opacity and the offset of a panel without requiring repaints. The photon animation doesn't use a scale transform anyway, so offset + opacity should be all we need to get the correct animation.
And since we won't be using the system animation, we don't need to call private APIs to trigger it.

I will start by adding a -moz-window-opacity CSS property so that we can use CSS transitions to control the animation.
Assignee: nobody → mstange
Status: NEW → ASSIGNED

Updated

3 months ago
Priority: P2 → P1
(Assignee)

Comment 18

3 months ago
Tweaking the summary to match the new plan.
Summary: Use native arrow panel animations on macOS → Animate arrow panel animations on macOS in a way that does not require window repaints
(Assignee)

Comment 19

3 months ago
Actually, I'm just going to implement -moz-window-transform as well. That means that we'll need to call the private API CGSSetWindowTransform, but that API has been around for such a long time and is called by so many apps that I don't feel that bad about it.

Patches for the new CSS properties are up in bug 1370034.
Depends on: 1370034
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8777095 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8777096 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8777097 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8777098 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8777099 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Attachment #8777101 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c9b0b57e7ebdcaf1f559c76128e70450c323730
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Blocks: 1370131

Updated

2 months ago
Blocks: 1352075
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

2 months ago
mozreview-review
Comment on attachment 8874173 [details]
Bug 1291457 - On Mac, use -moz-window-opacity and -moz-window-transform for the panel animations.

https://reviewboard.mozilla.org/r/145598/#review151736

::: toolkit/content/xul.css:446
(Diff revision 5)
>  
>  panel[type="arrow"] {
>    -moz-binding: url("chrome://global/content/bindings/popup.xml#arrowpanel");
>  }
>  
> -%ifndef MOZ_WIDGET_GTK
> +%ifdef XP_MACOSX

Should probably use MOZ_WIDGET_COCOA instead?
Attachment #8874173 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

2 months ago
mozreview-review-reply
Comment on attachment 8874173 [details]
Bug 1291457 - On Mac, use -moz-window-opacity and -moz-window-transform for the panel animations.

https://reviewboard.mozilla.org/r/145598/#review151736

> Should probably use MOZ_WIDGET_COCOA instead?

Good point. Done.

Comment 33

2 months ago
mozreview-review
Comment on attachment 8777100 [details]
Bug 1291457 - Remove shadow invalidation code.

https://reviewboard.mozilla.org/r/68704/#review152086
Attachment #8777100 - Flags: review?(spohl.mozilla.bugs) → review+
Hey mstange, is this ready to land?
Flags: needinfo?(mstange)
(Assignee)

Comment 35

2 months ago
No, it depends on the patches in bug 1370034.
Flags: needinfo?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

2 months ago
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/87d42add0b31
On Mac, use -moz-window-opacity and -moz-window-transform for the panel animations. r=dao
https://hg.mozilla.org/integration/autoland/rev/ae124a4dcf89
Remove shadow invalidation code. r=spohl
Backed out for test failures: https://hg.mozilla.org/integration/autoland/rev/0d08acc5a759

Comment 40

2 months ago
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/2bf975d104c5
On Mac, use -moz-window-opacity and -moz-window-transform for the panel animations. r=dao
https://hg.mozilla.org/integration/autoland/rev/ce17bd4b7de6
Remove shadow invalidation code. r=spohl

Comment 41

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2bf975d104c5
https://hg.mozilla.org/mozilla-central/rev/ce17bd4b7de6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

2 months ago
Iteration: --- → 56.1 - Jun 26
You need to log in before you can comment on or make changes to this bug.