Why do we need a postframe callback? would be good to have some comments on this

Changes in MediaQuery.sizeOf(context) cause RawMenuAnchor to close during didChangeDependencies. When this happens, calling setState during the closing sequence (i.e. handleCloseRequest -> onCloseRequested -> hideOverlay) will throw an error, since we're scheduling a build during a build. I think this could be fixed if OverlayPortal is given a declarative API (see #136769). I'll add comments explaining this.

While I was investigating how to phrase the comments, I realized that we could remove the post-frame callbacks if we used didChangeMetrics instead of MediaQuery, since didChangeMetrics is called during the idle phase. The drawback of this approach is that we would be rebuilding when the screen size changes rather than when the MediaQuery size changes. As a result, if a user modified the size of an ancestor MediaQuery widget, RawMenuAnchor wouldn't close. I submitted an issue about this here: #168539

I filed a PR regarding this issue here: #168623. For now, I'll comment in the explanation.

Done

Somewhere in here or other place should document the invoking order of onOpenRequested, showOverlay and onOpen same for onCloseRequest and onClose

So, I initially wrote out a detailed list, but I found it easier to digest when reading:

onOpenRequested:

  /// When [MenuController.open] is called, the full opening sequence is:
  /// 1. [onOpenRequested]
  /// 2. (optional delay)
  /// 3. `showOverlay`
  /// 4. [onOpen]
  /// 5. (optional animation)

onCloseRequested:

  /// When [MenuController.close] is called, the full closing sequence is:
  /// 1. [onCloseRequested]
  /// 2. (optional animation)
  /// 3. `hideOverlay`
  /// 4. [onClose]

I also tried combining both steps and placing them in the widget description, but I found it more intuitive to read parameter definitions.

Let me know if I should add more detail or change anything.

Replaced with VoidCallback

typedef this

Done

curious why we expose the position parameter if it is just going to be fed into the showOverlay callback. is there a use case for position parameter?

Actually, I meant to remove the position parameter from the showOverlay callback. My mistake.

We kept the position in RawMenuAnchorOpenRequestedCallback for animations. It's the only way to access the position outside of overlayBuilder. I don't mind removing it -- we can always provide a different method of accessing the position if needed. Let me know and I'll push an update.

• I forgot: the other reason was to allow users to determine if the position has changed. Tong suggested that, when a user calls showOverlay, we stop checking if the position has changed before calling onOpen and continuing on with the function execution. Previously, we would only run the function body if the menu was previously closed/closing or when the position changed. Now, we call onOpen any time showOverlay is called. If users want to change this behavior, the position is an escape hatch.

Separate question: we could choose to only call onOpen and onClose when the overlay is first shown or hidden (e.g. not during opening <-> closing or position changes). I'm not sure what is most useful for users.

The position is provided by the caller of MenuController.open. For example, it might be from the pointer event outside.

MenuController.open passed this position to RawMenuController.handleOpenRequest, which then send this position to the themed menu via onOpenRequested. Later it will also send this position to overlayBuilder. (The showOverlay no longer receives a position.)

Although in most typical cases the position is only used for the overlayBuilder to build an overlay at the correct position, we decided to also send it to onOpenRequested to maximize flexibility of the themed menu. This is because onOpenRequested is the earliest occasion when the themed menu can know the position. The themed menu might use for different reasons. One big reason could be to build something that also relies on the position but is not part of the overlay. There might be other usages, which I can't name, but I like this design for being simple and ensuring maximal flexibility.

Animating reopening when the position changes is another good use case

sounds good to me to keep this position and also removing the position from showOverlay

Done

maybe mention this callback can be used for triggering animation for the opened menu

Done

handleCloseRequest is actually called regardless of whether the overlay is showing.

Is it only possible when someone called MenuController.closeChildren? still curious why we trigger this callback if menu is already closed

if the possition is open in a different place, is it typical to retrigger the animation in the new place?

OneDrive and Google Drive run the opening animation every time, MacOS, Google Docs, and Microsoft Word don't animate at all. If we wanted to animate on repositions, we'd need access to the position.

Calling a widget onClose after a delay which the state may or may not have been destroyed is a footgun API. the parent widget which pass in the onClose callback may not expect it to be called after it is disposed.

not entirely sure what may be the best way to fix this issue with this type of api. Should the onClose be called synchronously when onCloseRequested is called? same of onOpen

I recently came across this in #162062 (comment)

Right now, onClose can't be called after a disposal. If you check the dispose implementation in _RawMenuAnchorBaseMixin:

And if you check the close method on _RawMenuAnchorState:

When overlayController.hide() is called, isOpen synchronously flips to false. When isOpen is false, onClose won't get called. So, since dispose() calls close(), which hides the overlay and flips isOpen to false, onClose will not be called after disposal.

However, the open() method doesn't perform the same checks, so calling showOverlay after dispose() could call onOpen.

That said, even if showOverlay is called, isOpen will remain false since OverlayPortal checks to see whether a target is attached when returning a value for isShowing:

and the target (_OverlayPortalState) is detached during disposal:

This doesn't solve onOpen being called after disposal, but it does mean that onClose will never be called after disposal, since isOpen will always be false after disposal.

If you do try to call showOverlay after disposal, you should get an assertion, because the menu controller is no longer attached to RawMenuAnchor. This assertion prevents onOpen from being called, but if we wanted to block calls to showOverlay instead of asserting, we could just check for whether the component is mounted.

Do you think you can make it a noop instead of an assertion? (Or make it throw an exception or a FlutterError if you deem proper) Assertions should only be used for internal errors (bugs with the framework's implementation) instead of user errors.

Oh I didn't realize that... I switched it to checking if the component is mounted. Lmk if you all have a preference.

In this case, I vote for noop. This is likely only show up in a corner case, which may not be easy to catch during development cycle. and it will make it slightly verbose to need to be aware of lifecycle during the async callback, and all customers will probably use the same code to prevent calls to showOverlay, might as well handle in the RawMenuAnchorState for the developer.

We should document this behavior in the api doc though, that calling showOverlay will be noop and onOpen will not be called if the widget is disposed

Done. It was previously documented for onClose and onOpen, but I added docs for onCloseRequested and onOpenRequested. I also changed tests to reflect this behavior (before, I was testing for assertion)

Same since this may potentially become a footgun

should this check if the widget has been disposed? i.e context.mounted

I couldn't create a situation where this threw an error, possibly because of the phase checking. The finalization phase comes before the post frame callbacks, so I would expect an error. That said, since it's minimal overhead to add the mounted check for safety, I'll add a mounted check. Should I include a comment that clarifies that this may be an unnecessary check? I ask because there were many ambigious phase checks in the original codebase.

I left out the comment, but did perform the mounted check.

voidCallback

or just use void callback in RawMenuAnchorOpenRequestedCallback, RawMenuAnchorCloseRequestedCallback

Done

somewhere in this should mention this will be noop if disposed

Added:

/// Calling `hideOverlay` after disposal is a no-op, meaning it will not
/// trigger [onClose] or hide the menu overlay.

should also mention noop when disposed

Added:

/// Calling `showOverlay` after disposal is a no-op, meaning it will not trigger
/// [onOpen] or show the menu overlay.