Prinzhorn / scrollmeister

Open-source JavaScript framework to declaratively build scrolling experiences
https://www.scrollmeister.com/
MIT License
37 stars 5 forks source link

Timing issue with appending new items #32

Open Prinzhorn opened 6 years ago

Prinzhorn commented 6 years ago

This right here https://github.com/Prinzhorn/scrollmeister/blob/4568a844a44ce1316a1464b198d75d471d7d8796/src/behaviors/GuidesLayoutBehavior.js#L141 will map all components, even if layout is not attached yet. Need to filter el.behaviors.hasOwnblabla(thing)

Prinzhorn commented 6 years ago

We can write tests for this case.

However, in general things like these can be fuzzed. I'm thinking about creating a list of behaviors an allowed props and then just append/remove/reorder a tons of elements and permutate all behaviors by using setAttribute/removeAttribute and wait until it crash.

Prinzhorn commented 6 years ago

When a new element with a layout behavior is attached we have another issue. _render is called immediately, because the parent guides-layout hasAlreadyNotifiedOnce. But rendering does not make sense at this stage because layout.width etc. are undefined and the resulting CSS is garbage. In the next frame the layout engine does layout and then the new item gets the correct CSS.

We're also doing unnecessary renders because of the events in _observeLayoutDependencies.

Here's what we want:

  1. A new element with a layout behavior is attached (scrollmeister:connected)
  2. The layout behavior is attached (layout:attach)
  3. All other elements update their dependency property
  4. The guides-layout does layout
  5. All elements, including the new one, rerender. This is the very first render for the new element.

Maybe connectTo is the wrong API to use anyway? Because we're not using the guidesLayoutBehavior instance anyway because the layout engine modifies the layout object directly inside the layout behavior.

Edit: same for connecting to ^scroll. It will fire immediately but we don't have layout yet.

Prinzhorn commented 6 years ago

The interpolate behavior suffers from a similar timing issue.

this.connectTo('^guides-layout', this._createInterpolators.bind(this));

For new elements _createInterpolators is called immediately. However, just because the layout engine did layout once doesn't mean the current element has layout already (it does not and that results in NaN interpolations).

We could connect it to layout and access the layout engine through its ^guides-layout.

Prinzhorn commented 6 years ago

In theory we could get rid of all these problems by recreating everything when an element is added or removed. This way there would be a guarantee that everything is as consistent as it was for the very first render.

Prinzhorn commented 6 years ago

So far the fuzz tests I'm writing right now caught all of these 🙌

Prinzhorn commented 6 years ago

So far the fuzz tests I'm writing right now caught all of these raised_hands

Aaaand another one. I assume this happens when an element is added and removed from the DOM within the same frame. this.parentEl is then null.

    TypeError: Cannot read property 'guides-layout' of null

      at LayoutBehavior.connectTo (http:/localhost:4444/dist/scrollmeister-extras.js:2625:26)
      at LayoutBehavior.behaviorDidAttach (http:/localhost:4444/dist/scrollmeister-extras.js:4699:9)
      at LayoutBehavior.Behavior (http:/localhost:4444/dist/scrollmeister-extras.js:2545:9)
      at new LayoutBehavior (http:/localhost:4444/dist/scrollmeister-extras.js:4681:111)
      at Scrollmeister.attachOrUpdateBehavior (http:/localhost:4444/dist/scrollmeister-extras.js:8102:5)
      at Scrollmeister.batchUpdateConditions (http:/localhost:4444/dist/scrollmeister-extras.js:8175:12)
Prinzhorn commented 6 years ago

Maybe connectTo is the wrong API to use anyway? Because we're not using the guidesLayoutBehavior instance anyway because the layout engine modifies the layout object directly inside the layout behavior.

Edit: same for connecting to ^scroll. It will fire immediately but we don't have layout yet.

Welp, layout.hasLayout works as a hack

Prinzhorn commented 6 years ago

the :detach event doesn't bubble because the behavior is detached in response to the component being disconnected (hence no parents to bubble to) 🤦‍♂️

Prinzhorn commented 6 years ago

The whole _observeLayoutDependencies is a bad idea and needs to be schedule with raf. There is no point in having every LayoutBehavior listen to those events for themselves. We can do that on a higher level and then tell all of them to update exactly once. E.g. using some sort of invalidation even or method.

Prinzhorn commented 6 years ago

In theory we could get rid of all these problems by recreating everything when an element is added or removed. This way there would be a guarantee that everything is as consistent as it was for the very first render.

Maybe that's the way to go. In Firefox the editor does currently not work because element-meister are handled before scroll-meister which means ^guides-layout is not defined yet. Instead of relying on scheduling raf in the correct order we should have on raf and process things in a deterministic order.