Closed Bug 1291457 Opened 8 years ago Closed 7 years ago

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

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Iteration:
56.1 - Jun 26
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox56 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 6 obsolete files)

Bug 1291457 - Remove shadow invalidation code.
58 bytes, text/x-review-board-request
spohl
: review+
Details
Bug 1291457 - On Mac, use -moz-window-opacity and -moz-window-transform for the panel animations.
59 bytes, text/x-review-board-request
dao
: review+
Details 
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.
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/
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/
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/
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.
Depends on: 1291469
Priority: -- → P2
Whiteboard: tpi:+
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)
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)
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)
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:+]
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)
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.
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
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
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
Attachment #8777095 - Attachment is obsolete: true
Attachment #8777096 - Attachment is obsolete: true
Attachment #8777097 - Attachment is obsolete: true
Attachment #8777098 - Attachment is obsolete: true
Attachment #8777099 - Attachment is obsolete: true
Attachment #8777101 - Attachment is obsolete: true
Blocks: 1370131
Blocks: 1352075
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 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 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)
No, it depends on the patches in bug 1370034.
Flags: needinfo?(mstange)
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
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
https://hg.mozilla.org/mozilla-central/rev/2bf975d104c5
https://hg.mozilla.org/mozilla-central/rev/ce17bd4b7de6
Status: ASSIGNED → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: