Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Virtual Target helper "element" #1445

Closed
wants to merge 2 commits into from

Conversation

Westbrook
Copy link
Contributor

@Westbrook Westbrook commented May 10, 2021

Description

By default overlays are dispatched from element triggers, however some magic around getBoundingClientRects() allows for a "virtual" trigger, e.g. https://popper.js.org/docs/v2/virtual-elements/. The ability to do so is useful for the context in the PopperJS docs where you want to overlay to follow your cursor, but also in the context of wanting to replace the contextmenu display in conjunction with a user interaction point along a canvas element, which that "point" to be the trigger of the overlay not the entire canvas element. Herein, you'll find a proposal for making that possible in SWC.

Requirements

On the PopperJS side, the "VirtualTrigger" object needs to at least include a synthetic implementation of getBoundingClientRects() in order to know where to place the overlay. This precipitates the need to be able to easily update the results of that call, for which this PR adds the updateBoundingClientRect() method to the VirtualTrigger element. This accepts the x and y coordinates of the virtual element.

On the SWC side there are a few more responsibilities of the trigger in an overlay experience.

  • it is the EventTarget from which sp-query-theme (which resolves the Spectrum theme information), as well as sp-closed and sp-opened (which announces the state of the overlay) are dispatched
  • they take focus() in certain contexts
  • their getRootNode() method is leveraged to resolve activeElement information
  • their closest() method is used to resolve overlay "stacks" or groups of modals demarcated by type="modal" overlays
    All of these responsibilities stem the the HTMLElement typing that they embody, and in relying on their DOM position for the majority of these things raise the bar on a virtual replacement

Approach

Herein I create a custom element, which directly satisfies the type requirements of HTMLElement, that rather than being used as an actual element in the HTML is constructed new VirtualTrigger() to be leveraged in a detached state for this functionality. The constructor is passed a host and the initial x and y positional values for the trigger. The host needs to be a DOM element that is attached to the tree and benefits from being a part of the actual application for the sake of resolving themes, etc. but doesn't strictly need to be associated to the actual interaction point. For ease of use, in our default example of triggering an overlay from a point on a canvas element, while the canvas can't be the target, it can, and likely should, be the host. In the constructor of the VirtualElement, the host (and the implementations for getBoundingClientRects and updateBoundingClientRect) are wrapped in a Proxy() and returned so that calls to the cached virtual trigger will be made to the Proxy rather than to an actual element.

Benefits

Providing a centralized VirtualTrigger API will be useful for code sharing and understandability across larger shared app surfaces while covering over foot gun situations like forgetting to resolve themes, etc. On top of adequately addressing the current needs of the trigger element being fairly HTMLElement-like, the approach of having VirtualTrigger extend HTMLElement and proxy calls as needed should be that we don't have to think about it too much when addressing future updates to the overlay system. While we could make a more synthetic "HTMLElement" to support this virtualization of the trigger, we'd likely have to take into account every API on a standard trigger that we leveraged and add it to the virtual one. Here we can rely fully on the fact that we have an actual HTMLElement for making those calls against. If at any point we need to add more virtual functionality then it simply becomes a matter of managing that forking in the Proxy, as well.

Questions

  • The proxy approach seems like a nice win both for the functional requirements, as well as the TypeScript requirements, but are there other patterns that would better serve us here?

  • Like any actual element, when this Virtual Trigger is used up and the closure from which it originates the complete, it should just GC, right? I've not thought a lot about leaks in this manner, but an additional benefit of it extending HTMLElement should be that the realities that govern there would govern here, right?

  • In the current demo, I'm using a fixed menu, which I think applies more cleanly to the initial use case. However, the demo on the PopperJS side points to the power of using this with a moving trigger. In that case, we already have the updateBoundingClientRect() method, but as outlined in the follow code on their page a consuming developer would subsequently need to update the instance:

        document.addEventListener('mousemove', ({ clientX: x, clientY: y }) => {
            virtualElement.getBoundingClientRect = generateGetBoundingClientRect(x, y);
            instance.update();
        });

    We surface this functionality via Overlay.update(), which is a nice helper, but in this case would we better serve our users by closing other that functionality and calling that for them when they update the position of their virtual trigger?

        updateBoundingClientRect(x: number, y: number): void {
            this.x = x;
            this.y = y;
            Overlay.update();      
        }
  • Are there use cases that you'd like to be sure are covered by this functionality in particular that you don't think is addressed here in?

Related Issue

fixes #1437

Motivation and Context

It should be easy to leverage the full capability of our overlay system.

How Has This Been Tested?

  • no tests yet, just demos for conversation

Screenshots (if appropriate):

Demo available here, right click to see the overlay:
https://westbrook-virtual-element--spectrum-web-components.netlify.app/storybook/?path=/story/overlay--virtual-element
This will look particularly nice when we get #1425 merged!

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. <=== ADD ME!
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes. <=== ADD ME!
  • All new and existing tests passed.

@adixon-adobe
Copy link
Collaborator

This definitely works and overall I think this approach makes sense, but I'd like to push a little harder on the need for a proxy target. The explanations you've provided have convinced me that it's convenient, but not fully that it's the right approach.

I think it's worth re-visiting whether the current trigger responsibilities that depend on HTMLElement still should be responsibilities in the virtual trigger case. What would the alternate behavior be if there wasn't a proxied element? Is there a default element that could serve this purpose?

Base automatically changed from westbrook/overlay-stories to main May 10, 2021 21:39
@Westbrook Westbrook force-pushed the westbrook/virtual-element branch from eba7e41 to a3821b3 Compare May 10, 2021 21:45
@Westbrook
Copy link
Contributor Author

Makes sense to push on this, especially because it might point out specific short comings in the overlay system in general. While I am attempting to approach this in the context of "add don't change", we might need to change or we might be able to leverage the change in conjunction with the work in #1436. Before suggesting a fully alternate path, I'd like to revisit the things a trigger does now from the position of making a synthetic trigger object to support that thought process.

In the context of PopperJS, it accepts an HTMLElement | VirutalElement type in this context, the VirtualElement type simply maintaining its own implementation of getBoundingClientRects(), so something like:

{
    getBoundingClientRects(): DOMRect;
}

It's the additional responsibilities that we've placed on it in SWC that make issue here, for sure. Our extended overlay functionality expects the following type:

{
    getBoundingClientRects(): DOMRect;
    // this event needs to bubble inline of the overlay "trigger" to resolve the scoped theme context to render the overlay with
    dispatchEvent(): boolean;
    // out current opened/closed API hangs off off the event system, so we'd need access to listen to those events.
    // these events currently bubble, so the expectation would be that they bubble inline of the "trigger" so they could be listened for above the triggering location
    addEventListener(): void;
    // the following is used to resolve `shadowRoot.activeElement` to decide where to those focus when an overlay is closed
    getRootNode(): node;
    // resolve the modal group of an overlay by finding the containing `active-overlay`
    closest(): Element | null;
}

My initial approach to this expected to be able to build up from the VirutalElement type in PopperJS, but I felt that fell short when starting to address the rest of the functionality we manage from this. Some things that I'll put some more thought to here:

  • extend the API to separate the Overlay API trigger and the PopperJS trigger so that we didn't need a proxy
  • cover over the above separation by "resolving" a trigger from the throw content: e.g. content.getRootNode().host or the like.
  • replace the overlaid element with a "trigger" instead of a placeholder comment
    I'll work up some pros/cons on these, but if you have other paths that you think would of benefit to investigate, please let me know!

@Westbrook Westbrook force-pushed the westbrook/virtual-element branch 2 times, most recently from 027e52e to c99f91f Compare May 11, 2021 21:08
@spdev3000
Copy link
Collaborator

Just my 2cents here: It would really make sense for us to have this VirtualTrigger when using context menu from our drawing stage/canvas. Not sure if we would need a separation between Overlay API trigger and the PopperJS trigger though. So far it looks good to me.

@Westbrook Westbrook force-pushed the westbrook/virtual-element branch 2 times, most recently from a8bd224 to bbfd83d Compare May 18, 2021 19:55
@Westbrook Westbrook force-pushed the westbrook/virtual-element branch from 5b9300d to ed16a85 Compare May 19, 2021 20:06
@Westbrook Westbrook marked this pull request as draft May 19, 2021 22:18
@Westbrook
Copy link
Contributor Author

@adixon-adobe thinking about this some more, I wonder if it makes better sense to conditional make the PopperJS trigger and the SWC Overlay trigger two different things. In this way we could leverage the current trigger in all of our current usage, but in a case where we want to position the Overlay in a way not associated with an element, we could send in the options a virtualTrigger that merely managed access to a synthetic getBoundingClientRects() with customization for which surfaced via an updateBoundingClientRect() method. Thoughts?

I'll mark up a separate branch with how I see that working for comparison.

@Westbrook
Copy link
Contributor Author

@adixon-adobe when you have a moment, I've created #1477 with an alternate proposal that separates the responsibilities of the "Trigger Element" for SWC and PopperJS when needed.

@Westbrook Westbrook force-pushed the westbrook/virtual-element branch from 55dd1f4 to 38d5044 Compare May 28, 2021 21:25
@Westbrook
Copy link
Contributor Author

Closing in favor of #1477

@Westbrook Westbrook closed this Jun 1, 2021
@Westbrook Westbrook deleted the westbrook/virtual-element branch June 24, 2021 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Overlay] Add Virtual Element support for triggering overlays
3 participants