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

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Blocks: 1 bug)

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:+])

Attachments

(2 attachments, 6 obsolete attachments)

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

Description

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Depends on: 1291469
Priority: -- → P2

Updated

2 years ago
Whiteboard: tpi:+
(Assignee)

Updated

2 years 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

2 years 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)
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

2 years 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

2 years ago
Priority: P2 → P1
Whiteboard: tpi:+ → [tpi:+][photon]
Whiteboard: [tpi:+][photon] → [tpi:+][photon-visual]
Blocks: 1296507
Flags: qe-verify-
Priority: P1 → P2
Whiteboard: [tpi:+][photon-visual] → [photon-visual][p1][tpi:+]

Comment 13

2 years 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

2 years 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

2 years 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
Priority: P2 → P1
(Assignee)

Comment 18

2 years 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

2 years 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

2 years ago
Attachment #8777095 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8777096 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8777097 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8777098 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8777099 - Attachment is obsolete: true
(Assignee)

Updated

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

Updated

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

Comment 29

2 years 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 years 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 years 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 years ago
No, it depends on the patches in bug 1370034.
Flags: needinfo?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

2 years 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

Comment 40

2 years 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 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2bf975d104c5
https://hg.mozilla.org/mozilla-central/rev/ce17bd4b7de6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Iteration: --- → 56.1 - Jun 26
You need to log in before you can comment on or make changes to this bug.