fxos-components / dom-scheduler

Making the performant thing the easy thing.
67 stars 7 forks source link

The API is confusing #36

Open wilsonpage opened 9 years ago

wilsonpage commented 9 years ago

It's not clear to me which function I should use for a particular use-case. I find it hard to understand what names like 'attachDirect' and 'feedback' mean.

I'm probably overlooking details, but an API like this is clearer to me:

scheduler.interaction();
scheduler.transition();
scheduler.animation();
scheduler.mutation();
etiennesegonzac commented 9 years ago

Happy to brainstorm it! We still need a notion of add/remove for direct blocks and we still need a way to express that "feedbacks" (responding to direct user actions, like an active state) have higher priority than classic "transitions" (a panel slide etc...).

We could have separate transition/animation and abstract the event name on which we need to wait, but it might get even more confusing once we also express the priority (nothing about a CSS transition vs. a CSS animation gives us a hint about how to schedule them)

wilsonpage commented 8 years ago

I find it had to articulate the difference between feedback and attachDirect.

Might be quite nice to have something like:

scheduler.interaction(el, 'scroll', callback);
scheduler.interaction.off(el, 'scroll', callback);

Unrelated: We might also want a scheduler.measure() block so that scheduler can decide when is best to execute measuring operations (eg. el.clientWidth) that could force reflow/jank.

wilsonpage commented 8 years ago

(responding to direct user actions, like an active state) have higher priority than classic "transitions" (a panel slide etc...).

It would be nice if we could find a way to merge .attachDirect() and .feedback() as they seem similar. Is the only difference that .feedback() resolves once a transiiton/animation has completed?

Perhaps something like this:

scheduler.interaction(el, 'touchstart', () => {
  el.classList.add('pressed')
  return scheduler.once(el, 'transitionend')
}).then(() => el.classList.remove('pressed'));
scheduler.interaction(el, 'scroll', ...);

Gives the user the choice to delay the Promise resolving until an interaction has completed to allow us to support both use-cases without so much confusion.

Excuse me if I still don't fully understand :)

wilsonpage commented 8 years ago

After looking a little closer at the code I think the above example would look more like:

scheduler.interaction(el, 'touchstart', () => {
  el.classList.add('pressed')
  scheduler.once(el, 'transitionend')
    .then(() => el.classList.remove('pressed'));
});

scheduler.once() is no more than a simple helper function.

etiennesegonzac commented 8 years ago

oh, probably misunderstood the initial proposal. here's what I though it'd be at first, maybe it'll inspire a new solution.

we want to merge .feedback .attacDirect and .detachDirect under the same concept. and the core concept here is ~direct manipulation~, interaction is a nice keyword for it. especially since the API could then become interaction animation and mutation.

so the main use cases under the new interaction concept are:

so I though the proposal was scheduler.interaction.once(el, 'touchstart', block) -> listen for the next evt then remove the listener, should be scheduler.interaction.next() ? but unsure when we'd resolve/stop protecting scheduler.interaction.on(el, 'scroll', block) -> listen for all the events until... scheduler.interaction.off(el, 'scroll') -> or might need to give an id back when we start an ongoing listen if we want to support multiple listeners for the same el/evt pair

wilsonpage commented 8 years ago

So the main reason for scheduler.once() was to allow the user to indicate back to the scheduler that their interaction has finished. In the scroll case this isn't required as it only occupies one frame; but in the 'click with a fancy pressed transition' this is required.

So we have two cases to cover:

A. Block returns no promise B. Block returns a Promise that blocks until resolved.

In B, the scheduler.once() helper makes it simpler to do this.

etiennesegonzac commented 8 years ago

Oh, just realized big part of the confustion/friction comes from the fact that we have the current feedback api that takes an event to know when to resolve/stop protecting. And the current attachDirect/detachDirect that takes an event to know when to trigger the block.

wilsonpage commented 8 years ago

Regarding .transition()/.animation() we can either infer the end of the transition using transitionend/animationend or support the user returning a Promise. The latter is a useful escape hatch and scheduler.once() could be a useful helper.

etiennesegonzac commented 8 years ago

Just saw your second post, I think moving some of the hint to what the block returns could greatly help simplify the API! Need to think about it a bit more.

etiennesegonzac commented 8 years ago

And the notion of triggerEvent and endEvent (or better naming) is also something that runs through the whole API and needs to be cleaned up!

etiennesegonzac commented 8 years ago

I like the idea that some stuffs with high priority can only happen on user input :)

wilsonpage commented 8 years ago

Let's jot some full examples:

// blocks others until 'transitionend'
scheduler.interaction(el, 'click', e => {
  el.classList.add('pressed');
  return scheduler.once(el, 'transitionend');
});
// blocks for a short hard coded setTimeout
scheduler.interaction(el, 'scroll', e => {
  ...
});
// blocked on .interaction(), resolves on 'animationend'
scheduler.animation(el, () => el.classList.add('slide-in'));
// blocked on .interaction(), resolves on 'transitionend'
scheduler.transition(el, () => el.style.opacity = 0);
// can return a Promise to trump inferred end
scheduler.transition(el1, () => {
  ...
  return new Promise(...);
});
// blocked by any pending above
scheduler.mutation(() => el.innerHTML = 'hello world');
wilsonpage commented 8 years ago

Throwing some other alternative ideas out there. Scheduler could stay away from event binding and instead just wrap functions, that, when called, get queued appropriately. Could help focus the library...?

el.addEventListener('click', e => {
  scheduler.interaction(() => {
    el.classList.add('pressed');
    return scheduler.once(el, 'transitionend');
  });
});
scheduler.transition(() => {
  el.style.opacity = 0;
  scheduler.once(el, 'transitionend');
});

This then begins to raise the question: Is there a difference between .transition() and .mutation(), other than duration? They are both low priority, and can be queued if a high-priority task (.interaction()) is in progress.

Could we focus the library even further to a core concept of high priority blocking tasks and low priority tasks that can be blocked?

el.addEventListener('scroll', e => {
  scheduler.now(() => {
    ...
  });
});
scheduler.later(() => {
  // start transition...
  return scheduler.once(el, 'transitionend');
});
scheduler.later(() => {
  // mutate dom
});

If this is the case we could base the library on this core concept, and build some higher level concepts (.interaction(), .animation(), .mutation()) on top ... ?

etiennesegonzac commented 8 years ago

I prefer the higher level API and having the scheduler do event binding because it enables a lot of niceties:

But I like the examples from https://github.com/fxos-components/dom-scheduler/issues/36#issuecomment-164472257 a lot!

I think I just need some clarification about when the listener for the "trigger event" gets removed in the interaction uses cases. Like:

wilsonpage commented 8 years ago

I prefer the higher level API and having the scheduler do event binding because it enables a lot of niceties:

I wasn't suggesting this as a replacement, but core primitive that the nice APIs sit on top of.

I think I just need some clarification about when the listener for the "trigger event" gets removed in the interaction uses cases.

Isn't that the responsibility of the user?

scheduler.interaction(el, 'touchstart', e => {
  scheduler.interaction(el, 'touchmove', ontouchmove);
  scheduler.interaction(el, 'touchend', ontouchend);
});

function ontouchend() {
    scheduler.interaction.off(el, 'touchmove', ontouchmove);
    scheduler.interaction.off(el, 'touchend', ontouchend);
}
scheduler.interaction(el, 'scroll', e => {
  scheduler.interaction(el, 'scrollend', onscrollend); // perhaps needs check
});

function onscrollend() {
  scheduler.interaction.off(el, 'scrollend', onscrollend);
}
etiennesegonzac commented 8 years ago
scheduler.interaction(el, 'touchstart', e => {
  scheduler.interaction(el, 'touchmove', ontouchmove);
  scheduler.interaction(el, 'touchend', ontouchend);
});

Looks a bit weird, if the block is just adding listeners it shouldn't be scheduled. (Totally understand this comes from my "having the scheduler do event binding" but using the interaction API for that feels weird.)

wilsonpage commented 8 years ago

Oh I see what you mean, perhaps we should just use standard addEventListener() here? I don't think I fully understand the question.

etiennesegonzac commented 8 years ago

Yes we'd use a standard addEventListener here probably. So the scheduler would handle event binding

which let's us with the use cases of the current feeback call, it works like a .transition and has the priority of an .interaction and it kinda started this thread.

It's mainly use for "pressed" effects on buttons and I'd be fine limiting its use case to this.

      return scheduler.feedback(function() {
        button.classList.add('effect');
      }, button, 'transitionend').then(function() {
        button.classList.remove('effect');
      });

surely we can make it prettier. maybe we can limit it to toggling a class

scheduler.interaction.toggle(el, 'click', 'className');
// adds the class and blocks others until we get an `animationend` or `transitionend`
// at which point we remove the class and stop blocking

which means you'd still add a classic event listener if you want to execute some code on the button press (which you probably want)... :/

wilsonpage commented 8 years ago

Oh ok so I guess the example would look more like:

el.addEventListener('touchstart', e => {
  scheduler.interaction(el, 'touchmove', ...);
  el.addEventListener('touchend', ontouchend);
});

function ontouchend() {
  el.removeEventListener('touchend', ontouchend);
  scheduler.interaction.off(el, 'touchmove', ...);
}

Is that correct? How would you do that today?

scheduler.interaction.toggle(el, 'click', 'className');

IMO this seems a bit too high level.


I'm not sure if you're agreeing or disagreeing with the new scheduler.interaction proposal?

etiennesegonzac commented 8 years ago

I'm not sure if you're agreeing or disagreeing with the new scheduler.interaction proposal?

I like the general approach, just trying to cover the use cases of the current DomScheduler uses :) (we're still missing the feedback use case)

This last example looks good, but is there a javascript trickery enabling us to have scheduler.interaction be a function and still host scheduler.interaction.off ?

wilsonpage commented 8 years ago

I like the general approach, just trying to cover the use cases of the current DomScheduler uses :) (we're still missing the feedback use case)

Doesn't this solve it?

scheduler.interaction(el, 'click', e => {
  el.classList.add('pressed');
  return scheduler.once(el, 'transitionend');
});

This last example looks good, but is there a javascript trickery enabling us to have scheduler.interaction be a function and still host scheduler.interaction.off

We can bolt methods onto methods :dancers:

wilsonpage commented 8 years ago
function interaction() {}
interaction.off = function() {};
wilsonpage commented 8 years ago

We'd have to bind some context of course :)

etiennesegonzac commented 8 years ago

looks good, trying to recap the new versions for all the current examples, have some questions inlined:

API

scheduler.attachDirect()

Attaching a handler

scheduler.interaction(elm, evt, block)

Detaching a handler

scheduler.interaction.off(elm, evt, block)

Example

scheduler.interaction(el, 'touchmove', evt => {
  el.style.transform = computeTransform(evt);
});

=> Protects with a timeout (+ special case for scroll/scrollend when available (we stop protecting when we get a scrollend))

scheduler.feedback()

scheduler.interaction(elm, evt, block)
// where block returns a promise

BTW this refactoring will fix #39

scheduler.interaction(el, 'click', e => {
  el.classList.add('pressed');
  return scheduler.once(el, 'transitionend');
}).then(() => {
  el.classList.remove('pressed');
});

-> But how will this work for subsequent click event, the promise returned initially will already be resolved.... so we'll never remove the class :/

scheduler.transition()

scheduler.transition(elm, block);
// automatically resolve/stops protecting once we get a transitionend or after a safety timeout 

should we keep a parameter to set the timeout?

scheduler.transition(el, () => {
  el.style.transition = 'transform 0.25s ease';
  el.classList.remove('new');
}).then(() => {
  el.style.transition = '';
});

scheduler.mutation()

scheduler.mutation(block);
scheduler.mutation(() => {
  el.textContent = 'Main List (' + items.length + ')';
});

New APIs

scheduler.animation(elm, block);
// same as .transition for animations
scheduler.idle(block)

could solve the "measurement" use case, would be flushed after the mutations and never sync, would be related to / close #32

wilsonpage commented 8 years ago

=> Protects with a timeout (+ special case for scroll/scrollend when available (we stop protecting when we get a scrollend))

If the user cares about 'scrollend' can't they just bind to it themselves? Or do you want the library to handle this?

scheduler.interaction(el, 'scroll', e => {
  el.addEventListener('scrollend', ...);
});

-> But how will this work for subsequent click event, the promise returned initially will already be resolved.... so we'll never remove the class :/

scheduler.interaction(el, 'click', e => {
  el.classList.add('pressed');
  return scheduler.once(el, 'transitionend')
    .then(() => el.classList.remove('pressed'));
})

should we keep a parameter to set the timeout?

Yeah probably.

scheduler.transition(el, 250, () => {
  el.style.transition = 'transform 0.25s ease';
  el.classList.remove('new');
}).then(() => {
  el.style.transition = '';
});
etiennesegonzac commented 8 years ago

If the user cares about 'scrollend' can't they just bind to it themselves? Or do you want the library to handle this?

stopping the protection is super internal so the library needs to do it.

etiennesegonzac commented 8 years ago

Can we keep the timeout as an (optional) last parameter?

wilsonpage commented 8 years ago

Can we keep the timeout as an (optional) last parameter?

It could be optional second if that's more readable ;)

wilsonpage commented 8 years ago

stopping the protection is super internal so the library needs to do it.

IIUC protection is for 360ms and is debounced on each event. Without scrollend we could be blocking the next task for up to 360ms?

etiennesegonzac commented 8 years ago

Without scrollend we could be blocking the next task for up to 360ms?

we are :) (excepts other tasks with the same priority)

etiennesegonzac commented 8 years ago

(Which makes me realize task might be a better term than block for a scheduler.)

etiennesegonzac commented 8 years ago

It could be optional second if that's more readable ;)

without losing browser support?

etiennesegonzac commented 8 years ago

Nit: how about manipulation instead of interaction? I'm seeing it a bunch in touch-action specs, might be a bit clearer.

wilsonpage commented 8 years ago

I personally think:

The user was interacting with the content

is clearer than:

The user was manipulating the content