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

Allow modifiers to interact with the element before it is insert into the DOM #652

Open
jelhan opened this issue Aug 4, 2020 · 11 comments

Comments

@jelhan
Copy link
Contributor

jelhan commented Aug 4, 2020

Modifier Manager should provide a hook, which has access to the DOM element but is executed before the element is insert into the DOM.

The installModifier hook is executed after the element is insert into the DOM:

This hook has the following timing semantics:

Always

  • called after all children modifier managers installModifer hook are called
  • called after DOM insertion

https://github.com/emberjs/rfcs/blob/master/text/0373-Element-Modifier-Managers.md#installmodifier

The createModifier hook does not have access to the DOM element. Accordingly to Modifier Manager RFC "called as discovered during DOM construction". Not sure if the element is created yet and just not passed in or does not even exist yet.

I have two use cases for a hook, which has access to the DOM element but is executed before it's insert into the DOM:

  1. A set-attribute and set-property modifier, which sets values as attributes or properties on the element before it's insert into the DOM.
  2. The existing ember-style-modifier, which manipulates the CSS styles of an element using CSSOM.

For both of them running before the element is insert into the DOM is important from a performance perspective. If executed after the element has been insert into the DOM the browser may do an unnecessary layout calculation in between. E.g. if Element.getBoundingClientRect() or a similar function is called in between.

It's even worse as there isn't any garantuee if the modifier is executed in the same tick as the DOM insertion:

May or May Not

  • be called in the same tick as DOM insertion

If it's executed in the next queue the Browser paint the state before the modifiers has been run. This may be visible to the end-user as an inconsistent and flickering UI.

Especially for set-attribute and set-property the timing is also important if considering custom elements. Custom elements are part of the web components feature set. Similar to @ember/component they have lifecycle hooks. One of them is the connectedCallback. It's executed when the element is insert into the DOM.

As modifiers are called only after the DOM element has been insert into the DOM manipulation attributes or properties set on the element using them are not available yet in connectedCallback. I have created a JSFiddle to illustrate the timing relevance: https://jsfiddle.net/jv0Lk3sb/ It's creating a custom element, which implements the connectedCallback. It sets two attributes on the custom element. One before and one after it has been insert into the DOM. Only the first one is available in connectedCallback.

The limitation for custom elements is especially relevant as one can not rely on Glimmer VM always setting values passed as attributes as attributes. Depending on the custom element it may be set as an attribute or a property. It may even change between first and subsequent runs. See this issue for more details. Modifier would be a good solution to get full control if the values are set as an attribute or a property on the element. But due to their timing issue with connectedCallback this is not possible in many cases.

While going this route we may want to consider allowing to run code with a modifier on destruction before the element is removed from DOM.

Thanks a lot to @rwjblue for the input on Discord, which helped creating this issue. I hope I got everything right.

@rwjblue
Copy link
Member

rwjblue commented Aug 4, 2020

Thanks for writing this up @jelhan! A few of us (@wycats, @krisselden, @chancancode, @pzuraq, @dgeb, and myself) discussed this (along with other GlimmerVM / Rendering issues) last night, and we are all generally in favor of solving this problem. I don't think we have a "for sure 100%" solution yet, but it likely involves both migrating to "attributes first" and providing more fine grained hooks in the modifier APIs.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

I'm closing this due to inactivity. This doesn't mean that the idea presented here is invalid, but that, unfortunately, nobody has taken the effort to spearhead it and bring it to completion. Please feel free to advocate for it if you believe that this is still worth pursuing. Thanks!

@wagenet wagenet closed this as completed Jul 23, 2022
@jelhan
Copy link
Contributor Author

jelhan commented Jul 26, 2022

I'm closing this due to inactivity.

It's not inactive. There has been recent discussion between different framework team members on Discord: https://discord.com/channels/480462759797063690/500803406676492298/966402401693229116 Sadly I haven't had time to add a summary (or at least that link) to this issue.

In that discussion we identified 4 different requirements for timing of modifiers:

  1. Any time after the element is added to DOM. Timing is not relevant.
  2. Before browser rerenders the page but maybe after element is added to DOM. (e.g. ember-style-modifier)
  3. Before element is added to DOM but regardless of order in template. (e.g. modifiers which set properties or attributes on custom elements/web components)
  4. In order of template. (E.g. modifier which grabs a reference to an element to be used later as target for {{in-element}})

The first use case is supported by RFC #373 today. The other three are not supported yet.

@wycats referenced the first option as idle and the second one as before paint in that Discord thread. I would add before insert for third option and in order for fourth option. I think that terms may help for further discussion.

@chancancode pointed out that third and fourth options could be a "performance hazard" and should be "avoided as much as possible" as both would need "to be synchronous".

@wycats also raised way less concerns with option 2 than with option 3 and 4:

I'm not confident that we can commit to running code literally before things are inserted into the DOM, but I am confident that we can commit to running code before browser paint

To move forward, I would recommend to focus on supporting option 2 (before paint) as next step. It seems to be less complex than option 3 and 4. And unblock relevant use cases like ember-style-modifier.

Before committing to support option 3 (before insert) and 4 (in order) we may want to investigate if the target use cases could be achieved in another way:

  • Using a modifier to set attributes and properties feels like a work around. Maybe support should be added in Glimmer syntax itself.
  • Using a modifier to get a reference to an element and passing that one around feels wrong to myself. But there seems to be several use cases, which require a reference to an element created by Glimmer VM. As proven by popularity of ember-ref-bucket, ember-ref-modifier and other implementations of that pattern. Maybe a dedicated API could be added to Glimmer templates for that use case as well.

@wagenet wagenet reopened this Jul 26, 2022
@wagenet
Copy link
Member

wagenet commented Jul 26, 2022

I’m reopening for further discussion. Thanks for the update @jelhan!

@wycats
Copy link
Member

wycats commented Jul 28, 2022

@jelhan I've made decent progress on the Starbeam work I was doing that helped me clarify my own thoughts on this originally.

I think where I'm at right now is that we (definitely) want two different possible timings for modifiers:

  • layout (i.e. before paint). This would run after the element was inserted into the DOM, but (synchronously) before the browser painted the DOM.
  • idle (current timing). This would run after the browser painted the element and returned back to the task queue (and possibly had a chance to take care of higher priority tasks).

I agree that using modifiers as a registration pattern to gain access to DOM elements is a problem. It is especially problematic because that one use-case is largely responsible for the situations in which the timing between modifiers is important. This is one of the primary use-cases that then leads to the assumption that we need an earlier bucket.

In my opinion, we want syntax that provides access to DOM elements. This is the straw-man I've been kicking around for a few years:

<video #player src={{@url}}></video>

<button {{on "click" (fn this.toggle #player)}}>⏯️</button>

The basic idea is that you could attach a tag to any HTML element, and that name (in this case #player) would be available in the template scope as a reference to the element.

You could then turn the play/pause button into a component pretty easily.

In this design, I think you'd make it an error to attempt to access the #player synchronously during initial render, just like what happens in JavaScript if you attempt to access a const variable before it was initialized.

The key consequence is that by making element tagging declarative, Ember becomes responsible for ensuring that the variable is initialized before any modifier behavior, just like other syntactic variables. In my opinion, treating this use-case like variable initialization sidesteps z-index-style problems that are created when we attempt to treat it as an imperative lifecycle phase.

@jelhan What are your thoughts? Assuming we actually do implement a solution along these lines for getting a reference to an element, what are use-cases are still left unsolved?

@nightire
Copy link

nightire commented Jul 29, 2022

It's important to consider how to access the tags attached to the HTML elements from outside the component. It sounds a lot like forwarded reference in React: we pass tags when invoking components, and the component responsible for delivering these tags to the target elements. It's handy when developing complex structured UI components where we often need to hold multiple tags in the top-level root component.

<video #player={{@playerTag}} />  <!-- assign to passing arguments -->

<video #player={{this.setPlayerTag}} />  <!-- accessible by passing functions -->

I also like the idea in the https://github.com/lifeart/ember-ref-bucket, in which we can create locally or globally tags and use them in whatever ways we use other values.

@jelhan
Copy link
Contributor Author

jelhan commented Jul 29, 2022

@wycats Thanks a lot for sharing that detailed thoughts. Looks good to me. But haven't thought about the details yet. Especially how such a reference could be shared with templates outside current scope. Sometimes the element one what to target is created and consumed in different components. Modals and other overlays are typical example for this.

Assuming we actually do implement a solution along these lines for getting a reference to an element, what are use-cases are still left unsolved?

Using custom elements is not addressed yet. For these is might be important if something is set as attribute or property. Glimmer VM does not allow yet to reliable set attributes or properties. It depends on implementation details of the custom element.

At the same time it might be important for custom elements to set attributes and properties before inserting it into DOM. Again this depends on implementation details of the custom element.

Custom elements have a lifecycle hook called connectedCallback, which is invoked when the custom element is insert into DOM. A custom element may rely on specific attributes and properties being set before this hook is called.

But as said above, I think this would be better solved by improving template syntax to reliable set attributes and properties.

In the meantime the element can be created manually (e.g. using a template helper), setting attributes and properties on it, and using that one in the template. It's not blocking custom element usage. It "only" affects developer ergonomics.

I would recommend that going forward we move discussion about 1) element reference and 2) reliable setting attributes and properties out to separate issues. Both seem to be complex enough to deserve their own issues.

I think main questions regarding before paint timing have been resolved. Next step might be drafting a RFC for that one. Or did I missed something important?

@jelhan
Copy link
Contributor Author

jelhan commented Aug 8, 2022

There seems to be similar limitations die to timing of destroyModifier hook: emberjs/ember.js#18873

Not sure if both should be addressed in the same RFC. But it might be good to at least consider it.

@wycats
Copy link
Member

wycats commented Aug 25, 2022

Glimmer VM does not allow yet to reliable set attributes or properties. It depends on implementation details of the custom element.

In general, I think we should introduce a narrow kind of (declarative) modifier that can only set attributes or properties on the element.

A rough sketch:

  • A modifier that only sets attributes would run in SSR.
  • A modifier that sets a property could specify an attribute that runs in SSR and a reconciliation rule for upgrading the attribute into a property on the client. Properties would always be set during DOM tree construction, either immediately before insertion into the DOM or immediately after.

But as said above, I think this would be better solved by improving template syntax to reliable set attributes and properties.

Among other things, this would make it possible to implement a reliable {{attr}} or {{prop}} modifier. I have long believe (and still believe) that the correct behavior of attribute syntax should be to reliably set attributes (with the prop modifier available as an escape valve), and perhaps that's something we could do for Polaris.

Any thoughts?

@jelhan
Copy link
Contributor Author

jelhan commented Oct 13, 2022

I had a pairing session with @wycats on modifier capabilities and timing this week.

We discussed the following steps forward:

  1. Timing capability for modifiers

    Support layout and idle timing for modifiers. Layout would be for things, which must happen before browser paints the page. Idle would be for everything else. Currently only idle timing is supported, which causes unnecessary repaints if a modifier changes layout of an element (e.g. ember-style-modifiee). Overcoming that limitation is main motivation for this first step.

    Disclaimer: We run out of time before being able to discuss potential ModifierManager API. The following is soley my own thoughts.

    It might be implemented by adding installModifierOnLayout and installModifierOnIdle hooks to ModifierManager class and deprecating installModifier hook at a later time:

     export interface ModifierManager<ModifierStateBucket> {
       capabilities: ModifierCapabilities;
       createModifier(factory: unknown, args: Arguments): ModifierStateBucket;
    -  installModifier(instance: ModifierStateBucket, element: Element, args: Arguments): void;
    +  installModifierOnLayout(instance: ModifierStateBucket, element: Element, args: Arguments): void;
    +  installModifierOnIdle(instance: ModifierStateBucket, element: Element, args: Arguments): void;
       updateModifier(instance: ModifierStateBucket, args: Arguments): void;
       destroyModifier(instance: ModifierStateBucket, args: Arguments): void;
     }

    A consumer would be able to implement both installModifierOnLayout and installModifierOnIdle in the same modifier.

  2. Preinsert hook for modifiers

    Allow modifiers to define attributes and properties, which should be set on the HTMLElement before attaching it to the DOM. This may be called a preinsert hook. It would run on server-side rendering (SSR) as well.

    Unlike layout and idle hooks, it wouldn't get a HTMLElement as argument. Mainly because we would not have one on server-side rendering. Instead a developer would declare, which HTML attributes and element properties should be set by returning meta-data. Such meta-data may look like the following:

    [
       { type: "attribute", name: "some-html-attribute", value: "value-for-this-html-attribute" },
       { type: "property", name: "an-element-property", value: "value-for-this-element-property" },
    ]

    It could be used to reliably set attributes and properties of an element using modifiers. For example a modifier could set value attribute of an <input> element initially and only update the value property of that <input> element going forward. This would fix issues with reset buttons. It would also solve issues with using custom elements, which rely on specific attributes or properties being set before their connectedCallback is executed.

    Exposing those capabilities to consumers is a required step if we want to change semantics of Glimmer VM at some point of time to always set HTML attributes (instead of setting element properties in some cases like <input value={{this.value}} >).

    To fully support server-side rendering it would need a capability to define rehydrate logic for element properties.

  3. Improving syntax to apply modifiers

    We may want to explore more ergonomic syntax in handlebar templates to apply a modifier to an element. @wycats proposed something similar to Vue and Svelte:

    <button on:click={{this.handleClick}}>
    
    {{! Today's equivalent}}
    <button {{on "click" this.handleClick}}>

    It could be implemented in userland. No need to add new primitives to enable such experiments.

    Note: This is not a priority for myself. If you are interested in exploring improvements in this area, please step up and get engaged.

  4. Improving high-level APIs to write modifiers

    The current high-level APIs to write modifiers provided by ember-modifiers doesn't seem to fit well for handling this additional capabilities.

    Function-based modifiers can only handle two hooks by design. It's hard to imagine how they could allow a developer to do some work on layout and some other work on idle.

    Same applies for class-based modifiers. They were recently changed to only have one hook: modify. Having two hooks (modifyOnLayout, modifyOnIdle) seems to be a step backwards.

    @wycats experiments with an event based API for modifiers in Starbeams:

    modifier.on('layout', (element) => element.style.backgroundColor('green'));
    modifier.on('idle', (element) => element.addEventListener('click', doSomething));

    Such experiments could be done in userland already today. No additional primitives are needed.

    My personal focus is on adding the timing and preinsert capabilities. Not on improving high-level authoring API. If you are interested in this topic, you are encouraged to work on it.

We haven't had the time to discuss the use case of getting a reference to an element. Please have a look at @wycats post from July 28 for that topic.

@jelhan
Copy link
Contributor Author

jelhan commented Dec 10, 2022

I published a RFC addressing some of the limitations of modifiers in Ember: #883

I'm working on another RFC server-side rendering capability to modifiers. I expect that it won't be the last one. 🙃

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

5 participants