Attach user popovers to DOM nodes added after the initial DOM query
What does this MR do?
For #296621 (closed)
Problem
When the user popovers are added, every node must be present in the DOM.
It's not always the case that every user name link is in the DOM tree, so some links that are added later don't trigger the user popover.
Solution
Add a DOM Mutation Observer that rechecks and applies the user popover when the DOM tree is modified.
Notes
- The mutation observer only begins observing the DOM tree after the popovers have been applied the first time. It should be okay to have the mutation observer start working before anyone asks for it, but it's safer to make it inert until some code actually initializes the popovers.
- A mutation observer cannot observe the same node more than once, and it manages its own deduplication. This isn't a problem right now, but it is a good safety net in case duplicate observations are triggered.
- The popover initialization code already checks for nodes that have been bound and won't rebind the popover, so we don't need to worry about accidental duplicate popovers.
Additional Concerns
- In my testing, I found that there was enough of a race condition (the body is ready, but the popover initialization simply scans the DOM for certain nodes. If nothing has loaded yet, no popovers will be assigned) that even top level username links didn't get the popover bound (see the before video). This fix also resolves that if any DOM mutations are observed after the popovers are initialized (this is usually the case, even when there aren't any threads, but there may be exceedingly rare situations where the mutation observer is applied, but no DOM mutations ever occur).
- A fix for this would be to avoid binding directly to the nodes and instead have a single listener that observes events at a very high level and manages the user popover for all pertinent hover events. This way, any arbitrary new DOM nodes will bubble events to the existing handler, rather than requiring a component to be bound directly to each node.
- I've disabled one ESLint rule for use before definition. It isn't really true that the function to add the popovers is used before it's defined. The mutation observer needs to be defined to be used to observe the DOM in the function. This could be resolved by constructing a brand new MutationObserver every time the function is called, but that could potentially bind multiple observers. Having a single observer avoids duplicate observers.
Screenshots (strongly suggested)
| Before | After | |
|---|---|---|
| Video | beforeMaster | afterBranch |
| Notes | Note here that even a top-level user name link doesn't show the popover |
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry - [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides - [-] Database guides
- [-] Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers - [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec - [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Thomas Randolph