Skip to content
This repository has been archived by the owner. It is now read-only.

Timing issue with appending new items #32

Open
Prinzhorn opened this issue May 25, 2018 · 10 comments
Labels
bug

Comments

@Prinzhorn
Copy link
Owner

@Prinzhorn Prinzhorn commented May 25, 2018

This right here

let nodes = Array.prototype.slice.call(this.el.querySelectorAll('[layout]')).map(el => el.layout);
will map all components, even if layout is not attached yet. Need to filter el.behaviors.hasOwnblabla(thing)

@Prinzhorn Prinzhorn added the bug label May 25, 2018
@Prinzhorn

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented May 29, 2018

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

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 4, 2018

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

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 19, 2018

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

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 19, 2018

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

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 19, 2018

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

@Prinzhorn

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 21, 2018

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

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 22, 2018

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 added a commit that referenced this issue Jun 22, 2018
…ncy type to only use attached layout behaviors, references #32
@Prinzhorn

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 22, 2018

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

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 22, 2018

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

This comment has been minimized.

Copy link
Owner Author

@Prinzhorn Prinzhorn commented Jun 25, 2018

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
1 participant
You can’t perform that action at this time.