wilsonpage / fastdom

Eliminates layout thrashing by batching DOM measurement and mutation tasks
6.85k stars 240 forks source link

Separating DOM writes in touchmove and touchend handlers with promises #121

Closed CodeWithOz closed 4 years ago

CodeWithOz commented 4 years ago

I'm using fastdom as @aFarkas described in this thread i.e. I only use fastdom.mutate to schedule DOM writes, and I only perform DOM writes inside fastdom.mutate; I never use fastdom.measure to read from the DOM. I have a certain touchmove handler where I have logic that writes to the DOM, and I need those DOM writes to have taken effect by the time the corresponding touchend handler is executed. In other words, I need the touchend handler to wait until touchmove has updated the DOM. What I have done is something like this:

function writeToDOM(cb) {
  fastdom.mutate(cb);
}

let touchMovePromise;
node.addEventListener('touchmove', event => {
  touchMovePromise = new Promise(resolve => {
    // ... stuff (not DOM updates)
    writeToDOM(() => {
      someNode.classList.add('changeYourStyles');
      // ... and some other DOM updates
      resolve();
    });
  });
});

node.addEventListener('touchend', async event => {
  await touchMovePromise;

  // ... stuff (not DOM updates)
  writeToDOM(() => {
    anotherNode.classList.add('changeYourStylesToo');
    // ... and some more DOM updates
  });
});

So the idea is what I described at the beginning: execute the touchend code only after touchmove has updated the DOM, and I confirm that the touchmove handler has finished updating the DOM by using a promise that resolves inside the rAF that was scheduled by touchmove.

I'd like to know if either @wilsonpage or @aFarkas (or anyone else) sees any weaknesses in this approach. In this thread and also in this one, you've both mentioned different reasons for which promises may not be ideal in combination with rAF (due to timing issues), but I think what I'm doing is different from those scenarios and doesn't suffer from the problems you both highlighted. However, the sooner I'm corrected, the better! :smile:

wilsonpage commented 4 years ago

Hi @CodeWithOz 👋

node.addEventListener('touchmove', event => {
  // ... stuff (not DOM updates)

  requestAnimationFrame(() => {
    someNode.classList.add('changeYourStyles');
    // ... and some other DOM updates
  });
});

node.addEventListener('touchend', event => {
  // ... stuff (not DOM updates)

  requestAnimationFrame(() => {
    anotherNode.classList.add('changeYourStylesToo');
    // ... and some more DOM updates
  });
});
CodeWithOz commented 4 years ago

@wilsonpage thanks for clarifying :+1:. I've realized that my code snippets didn't make it clear that the "stuff" happening inside the touchend handlers depend on the DOM updates that get scheduled inside the touchmove handler. That's why I'm using the promise to wait for those scheduled updates before continuing with the meat of the touchend handler. To be clear, this is closer to what's happening:

let touchMovePromise;
node.addEventListener('touchmove', event => {
  touchMovePromise = new Promise(resolve => {
    // ... stuff (not DOM updates)
    writeToDOM(() => {
      someNode.classList.add('changeYourStyles');
      // ... and some other DOM updates
      resolve();
    });
  });
});

node.addEventListener('touchend', async event => {
  await touchMovePromise;

  // ... stuff (not DOM updates)
  if (someNode.classList.contains('changeYourStyles')) {
    writeToDOM(() => {
      anotherNode.classList.add('changeYourStylesToo');
      // ... and some more DOM updates
    });
  }
});

Does that make a difference?


Also as you've pointed out about touchend always happening after the last touchmove, it's now occurred to me that I may be able to achieve the same delayed response in touchend by scheduling the touchend logic in a timeout inside rAF, as @aFarkas mentioned in one of those threads. Something like this:

function delayUntilNextFrame(cb) {
  writeToDOM(() => {
    setTimeout(cb, 0);
  });
}

node.addEventListener('touchmove', event => {
  // ... stuff (not DOM updates)
  writeToDOM(() => {
    someNode.classList.add('changeYourStyles');
    // ... and some other DOM updates
  });
});

node.addEventListener('touchend', event => {
  delayUntilNextFrame(() => {
    // ... stuff (not DOM updates)
    if (someNode.classList.contains('changeYourStyles')) {
      writeToDOM(() => {
        anotherNode.classList.add('changeYourStylesToo');
        // ... and some more DOM updates
      });
    }
  });
});

What do you think of this approach?


Lastly, I understand that I don't need to use fastdom, but I've seen somewhere (can't find the link right now) that it's generally better to schedule one rAF (or as few as possible) and have a task queue that gets executed inside that single rAF. The reasoning was that setup/teardown of the rAF callbacks can be a perf cost if there are lots of scheduled rAF callbacks. Fastdom already provides that task queue and schedules all of it in one rAF, and fastdom provides the throttling you mentioned. So that's why I thought to use it. But let me know if you still feel it's still not necessary.

Thanks!

wilsonpage commented 4 years ago

Thanks for the extra info. It seems like your trying to protect against the touchend listener from firing before the last touchmove which (even when scheduled via rAF or fastdom.mutate()) shouldn't ever happen.

I think you'd be fine with something like this. I've added an abstract throttledRaf() wrapper that would avoid you scheduling more work than could be painted (although I think some browsers do do this internally anyway).

const throttleRaf = (task) => {
  const isPending = false;

  return () => {
    if (isPending) return;
    isPending = true;

    requestAnimationFrame(() => {
      isPending = false;
      task();
    });
  }
}

node.addEventListener('touchmove', throttledRaf(event => {
    someNode.classList.add('a');
    // ... and some other DOM updates
}));

node.addEventListener('touchend', throttledRaf(event => {
  // will always have class 'a'
  anotherNode.classList.add('b');

  // ... and some more DOM updates
}));
CodeWithOz commented 4 years ago

Awesome, thanks! I'll go with this implementation :+1: .