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

misaligned if parent initially hidden #86

Closed
jamesarosen opened this issue May 14, 2018 · 11 comments
Closed

misaligned if parent initially hidden #86

jamesarosen opened this issue May 14, 2018 · 11 comments

Comments

@jamesarosen
Copy link
Contributor

jamesarosen commented May 14, 2018

I have nested overlays:

{{#my-context-menu as |menu|}}
  {{#menu.button}}
    Download
    {{#ember-popper-targeting-parent class='tooltip'}}
      Download the thing
    {{/ember-popper-targeting-parent}}
  {{/menu.button}}

  {{#menu.button}}
    Delete
    {{#ember-popper-targeting-parent class='tooltip'}}
      Delete the thing
    {{/ember-popper-targeting-parent}}
  {{/menu.button}}
{{/my-context-menu}}

When the page first renders, all of those elements are in the DOM but hidden. That mean that this._initialParentNode is well defined, but this._initialParentNode.offsetParent is null. This causes the tooltip (popper content) to be aligned to the top-left of the screen.

If I open the context menu and then scroll or resize the viewport then the tooltip pops right into place.

There's no good way for the parent to inform the {{ember-popper-targeting-parent}} that its "rendering state" has changed. I also don't love the idea that calling contexts should be responsible for knowing about this limitation.

Unfortunately, the only solution I've come up with is to have {{ember-popper-targeting-parent}} (or ember-popper-base) call popper.scheduleUpdate() every X ms, which is wasteful for most cases.

It might be possible to do this with a mutation observer on the _initialParentNode, but I'm not sure that such an observer would fire if an ancestor of that element changes an attribute or style.

Possibly related: floating-ui/floating-ui#537

@pzuraq
Copy link
Collaborator

pzuraq commented May 14, 2018

The way we're structuring our usage of popper, we insert the poppers lazily when the user actually needs them:

{{#my-context-menu as |menu|}}
  {{#menu.button}}
    Download

    {{#if buttonIsHovered}}
      {{#ember-popper-targeting-parent class='tooltip'}}
        Download the thing
      {{/ember-popper-targeting-parent}}
    {{/if}}
  {{/menu.button}}
{{/my-context-menu}}

A separate tooltip component handles the logic of buttonIsHovered. I think this is a better choice when it is doable, it helps with initial load times (fewer components to render) but I can see cases where you would want to have the poppers in the DOM at all times (a11y, etc).

@jamesarosen
Copy link
Contributor Author

I opened floating-ui/floating-ui#630 on the upstream Popper.js library since this can happen without ember.

@jamesarosen
Copy link
Contributor Author

I can see cases where you would want to have the poppers in the DOM at all times (a11y, etc).

A11y is precisely our reason for wanting the tooltip to always be in the DOM.

A separate tooltip component handles the logic of buttonIsHovered

Indeed I have just such a component. The problem is that it doesn't have access to the Popper instance, so it can't call scheduleUpdate on it.

One possible solution would be to add some a hook like didRenderPopper that gets called with the Popper so the containing context has access to that object. (Currently, the {{yield}} gives the contained context access.)

@jamesarosen
Copy link
Contributor Author

jamesarosen commented May 14, 2018

Another possible solution would be something like

// ember-popper components will call popper.scheduleUpdate any time
// any passed attribute changes
didReceiveAttrs() {
  if (this._popper) { this._popper.scheduleUpdate() }
  return this._super(...arguments)
}

Then I could "invoke" that with

{{#ember-popper-targeting-parent
  class=(concat 'tooltip ' (if hidden 'tooltip--hidden'))}}

This is more automatic, but it doesn't communicate intent as well.

@kybishop
Copy link
Owner

kybishop commented May 14, 2018

@jamesarosen you can use the registerAPI argument to call an action any time an attribute changes/get access to scheduleUpdate: https://github.com/kybishop/ember-popper/blob/master/addon/components/ember-popper-base.js#L255

EDIT: {{ember-popper registerAPI=(action 'someActionWhichSavesScheduleAPIForLater')}}

@jamesarosen
Copy link
Contributor Author

jamesarosen commented May 14, 2018

you can use the registerAPI argument

This is perfect. Thanks!

@kybishop do you have any interest in an {{ember-popper-tooltip}} component that builds on ember-popper-base? So far, I have

  • position
  • caret
  • styling
  • render text in blockless or block form
  • show on click/focus/mouseover
  • hide on blur/mouseout
  • other a11y concerns

@jamesarosen
Copy link
Contributor Author

I do need the same parentFinder concept so I can add the event handlers to the relevant parent. So far, that's meant duplicating the logic from ember-popper-targeting-parent. Regardless of whether you think {{ember-popper-tooltip}} is appropriate for this library, I would love to find a way to extract that for reuse. A mixin might work.

@kybishop
Copy link
Owner

Woot!

I'd like to keep ember-popper as just the positioning abstraction layer since it is used by other tooltip libraries. I worry that adding our own tooltip component would bloat consuming addons.

Ember-popper was initially built for use in ember-attacher, and is now used in ember-bootstrap as well. It is more of a spiritual successor to ember-tether than anything else.

@kybishop
Copy link
Owner

kybishop commented May 14, 2018

Duplicating logic from ember-popper-targeting-parent has been the go-to for consuming addons, the normal ember-popper component just allows them to bypass doing the same work twice.

Abstracting the parent-finding behavior out to a mixin might be nice... I wonder what implications that would have for code-readability in consuming addons.

@pzuraq
Copy link
Collaborator

pzuraq commented May 14, 2018

I'd prefer a utility function to a mixin, or both if possible. We're trying to avoid using mixins until a solid native class based solution arises in the JS ecosystem.

@jamesarosen
Copy link
Contributor Author

It turns out that I can avoid copying parentFinder by using the popperTarget from registerAPI :)

I think this resolves all my outstanding issues.

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

No branches or pull requests

3 participants