Fix bug where Grid context-menus lose focus when rendered inside `O…

[ghsolomon]

…verlays`

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

• Caught up with develop branch as of last change.
• Added CHANGELOG entry, or determined not required.
• Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
• Updated doc comments / prop-types, or determined not required.
• Reviewed and tested on Mobile, or determined not required.
• Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

[cnrudd]

Nice working finding that this fixes the issue.

To make it super safe can we:
only add the listener on grid render, maybe with a hook, or on gridModel instantiation, and then remove the listener on component unmount or gridmodel destroy?

I think that if the handler is a named function rather than a fat arrow anonymous function, it is easier to remove with removeEventListener.

I guess the problem with that is that you could end up with many listeners doing the exact same thing, or does adding a listener on the same event with the same function replace the listener?

And then if there is only one listener, and 3 grids, and one grid is removed and removes the listener... no more listener.

I can see now why you just added it like that, not tied to any rendering or model creation.

But... still leaving a listener around, or adding one even if a grid is never used....

[ghsolomon]

Good point, and yes - that's exactly why I added it the way that I did, but I think I've got something more elegant now (with a little more code complexity, but encapsulated in GridLocalModel which I think is better)

[lbwexler]

can we just add the global listener once if a grid ever gets created and that not worry about it? guess I am in favor of the encapsulation bit, but could skip the registration complexity

…

On Thu, Jul 25, 2024 at 6:13 PM Greg Solomon ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cmp/grid/Grid.ts <#3734 (comment)>: > @@ -867,3 +867,19 @@ export class GridLocalModel extends HoistModel { } }; } + +/** + * When a `Grid` context menu is open at the same time as a BP `Overlay2` with `enforceFocus`, the + * context menu will lose focus, causing menu items not to highlight on hover. By conditionally + * stopping the focus event from propagating, we can prevent this behavior. + */ Good point, and yes - that's exactly why I added it the way that I did, but I think I've got something more elegant now (with a little more code complexity, but encapsulated in GridLocalModel which I think is better) — Reply to this email directly, view it on GitHub <#3734 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARTL2N4OEQBIAQ3ETRAVEDZOF2BHAVCNFSM6AAAAABLPJTNRSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBQGU2TCMBXGM> . You are receiving this because your review was requested.Message ID: ***@***.***>

[ghsolomon]

can we just add the global listener once if a grid ever gets created and

that not worry about it?

On Thu, Jul 25, 2024 at 6:13 PM Greg Solomon @.***>

wrote:

@.**** commented on this pull request.

---

In cmp/grid/Grid.ts

#3734 (comment):

@@ -867,3 +867,19 @@ export class GridLocalModel extends HoistModel {

     }

 };

}

+/**

• • When a Grid context menu is open at the same time as a BP Overlay2 with enforceFocus, the

• • context menu will lose focus, causing menu items not to highlight on hover. By conditionally

• • stopping the focus event from propagating, we can prevent this behavior.

• */

Good point, and yes - that's exactly why I added it the way that I did,

but I think I've got something more elegant now (with a little more code

complexity, but encapsulated in GridLocalModel which I think is better)

—

Reply to this email directly, view it on GitHub

#3734 (comment), or

unsubscribe

https://github.com/notifications/unsubscribe-auth/AARTL2N4OEQBIAQ3ETRAVEDZOF2BHAVCNFSM6AAAAABLPJTNRSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBQGU2TCMBXGM

.

You are receiving this because your review was requested.Message ID:

@.***>

That's what I had been thinking originally by installing the event listener when the Grid module was read, but I respect Colin's desire to keep things clean. Ultimately I don't have a super strong preference, and this might all be theoretical (do any of our apps actually not have grids?)

[cnrudd]

do any of our apps actually not have grids?

I can think of 2 client apps (dedicated to charting) that do not have any grids.

I don't think that global listener would bother them. So, maybe complexity not necessary. Sorry to have encouraged it.

[lbwexler]

greg -- this looks great -- can we just add a method so this little annoying bug fix does not actually take over the constructor of this critical object . Also maybe give the variable/method associated names like addFocusFixListener() and didAddFocusFixListener()

[ghsolomon]

👍