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

FF Mobile & Safari - ScrollY binding performance issue #1579

Closed
andreujuanc opened this issue Jul 6, 2018 · 13 comments
Closed

FF Mobile & Safari - ScrollY binding performance issue #1579

andreujuanc opened this issue Jul 6, 2018 · 13 comments

Comments

@andreujuanc
Copy link

Hi,

I took a template and I removed all the jQuery and redoing everything in svelte+sapper. This is just a POC to give as a starting base to a team who's going to actually make it.

Link: https://wgmsacom-hyuydhwlgb.now.sh/

It's using <svelte:window bind:scrollY=scrollY/>
and then to calculate when it's on certain scroll position.

      onstate({ changed, current, previous }) {
       if (changed.scrollY) {
               this.set({
                   isTransparent: current.scrollY < 65
               });
           }
       }

Works fine everywhere, but it used to be slow on my S8 when i was using a computed property to do the calc, after that, replaced it with onstate and got it smooth everywhere, except in the iPhone SE that i have available (thanks hun) where it runs like a potato 🥔.

I removed the binding and scrolls up and down smoothly again.

My question, Can it be an issue with iOS, Safari and/or the scroll binding?

PD: if the link is down, it's my fault. Just ask and I will now it ASAP.

Thanks

@andreujuanc
Copy link
Author

Copy the RELP paralax demo and published here:
https://sveltedemo-gzufesoawc.now.sh/
Run it in my S8 with Firefox and Chrome.

FF: choppy, slow, and not fluent
Chrome: nicely smooth.

@andreujuanc andreujuanc changed the title iOS - ScrollY binding performance issue FF Mobile & Safari - ScrollY binding performance issue Jul 8, 2018
@maxmilton
Copy link
Contributor

maxmilton commented Jul 10, 2018

I'm not sure that binding directly to scrollY is the best way to go about this. Typically for reacting to viewport scrolling you would listen to the document scroll event and use some kind of debounce since scroll triggers too quickly.

I recently done something along these lines in a navbar component to detect if a user is at the top of the page or has scrolled: https://github.com/WeAreGenki/minna-ui/blob/master/components/navbar/src/MinnaNavbar.html#L106

@andreujuanc
Copy link
Author

Hi Max, I see. It´s more or less what I´m doing now.

But I saw somewhere that binding to scroll was the way to go. Specially to simplify code and make it cleaner.

I´m pretty sure that it is an implementation issue with Firefox mobile, since this demo: https://svelte.technology/repl?version=2.9.3&demo=parallax

Works great in my phone using Chrome , but no other web browser. So it´s not a matter of whenever binding or not it´s a good idea, but more like, what is that binding doing under the hood on each browser.

Anyway, I might just patch it with code similar of what you did in that link.

Cheers

@arggh
Copy link
Contributor

arggh commented Oct 15, 2018

I just noticed that plain simple adding this...

<svelte:window bind:scrollY=y/>

...to a component causes massive scroll jank on iPhones. Even if I'm not using that y-variable anywhere.

I don't think it's relevant to say "you didn't use throttle", because when replaced with this...

oncreate() {
    this.listener = () => this.set({ y: window.pageYOffset });
    window.addEventListener('scroll', this.listener);
}

...scrolling is again smooth as butter. When checking from the REPL, the code generated by that binding does have some kind of scrolljacking logic in it. That would be my first guess.

@mrkishi
Copy link
Member

mrkishi commented Oct 15, 2018

Huh, the generated code seems... Broken? Unless I'm misunderstanding it, it looks like it's calling window.scrollTo in response to scroll events. I don't get what the purpose even was here?

	// <svelte:window bind:scrollY="y"/>
	function onwindowscroll(event) {
		if (window_updating) return;
		window_updating = true;
		component._updatingReadonlyProperty = true;

		component.set({
			y: this.pageYOffset
		});

		component._updatingReadonlyProperty = false;
		window_updating = false;
	}
	window.addEventListener("scroll", onwindowscroll);

	component.on("state", ({ changed, current }) => {
		if (changed["y"]) {
			window_updating = true;
			clearTimeout(window_updating_timeout);
			window.scrollTo(window.pageXOffset, current["y"]);
			window_updating_timeout = setTimeout(clear_window_updating, 100);
		}
	});

@arggh
Copy link
Contributor

arggh commented Oct 15, 2018

@mrkishi I had a hard time understanding the reasoning behind this as well.

@arggh
Copy link
Contributor

arggh commented Oct 15, 2018

This is the old version of relevant compiler code from 1.64.1:

if (bindings.scrollX && bindings.scrollY) {
  const observerCallback = block.getUniqueName(`scrollobserver`);

  block.builders.init.addBlock(deindent`
    function ${observerCallback}() {
      ${lock} = true;
      clearTimeout(${timeout});
      var x = ${bindings.scrollX
        ? `#component.get("${bindings.scrollX}")`
        : `window.pageXOffset`};
      var y = ${bindings.scrollY
        ? `#component.get("${bindings.scrollY}")`
        : `window.pageYOffset`};
      window.scrollTo(x, y);
      ${timeout} = setTimeout(${clear}, 100);
    }
  `);

  if (bindings.scrollX)
    block.builders.init.addLine(
      `#component.observe("${bindings.scrollX}", ${observerCallback});`
    );
  if (bindings.scrollY)
    block.builders.init.addLine(
      `#component.observe("${bindings.scrollY}", ${observerCallback});`
    );
} else if (bindings.scrollX || bindings.scrollY) {
  const isX = !!bindings.scrollX;

  block.builders.init.addBlock(deindent`
    #component.observe("${bindings.scrollX || bindings.scrollY}", function(${isX ? 'x' : 'y'}) {
      ${lock} = true;
      clearTimeout(${timeout});
      window.scrollTo(${isX ? 'x, window.pageYOffset' : 'window.pageXOffset, y'});
      ${timeout} = setTimeout(${clear}, 100);
    });
  `);
}

...and here is the new one...

if (bindings.scrollX || bindings.scrollY) {
  block.builders.init.addBlock(deindent`
    #component.on("state", ({ changed, current }) => {
      if (${
        [bindings.scrollX, bindings.scrollY].map(
          binding => binding && `changed["${binding}"]`
        ).filter(Boolean).join(' || ')
      }) {
        ${lock} = true;
        clearTimeout(${timeout});
        window.scrollTo(${
          bindings.scrollX ? `current["${bindings.scrollX}"]` : `window.pageXOffset`
        }, ${
          bindings.scrollY ? `current["${bindings.scrollY}"]` : `window.pageYOffset`
        });
        ${timeout} = setTimeout(${clear}, 100);
      }
    });
  `);
}

They both seem to have the same oddity.

@Rich-Harris
Copy link
Member

scrollY is a two-way binding — if you set the scroll position programmatically (e.g. you're tweening the scroll position to the next 'checkpoint', or whatever) then window.scrollY should change.

See this fork of the parallax example to see it in action (change the value in the top left).

when replaced with this...

oncreate() {
    this.listener = () => this.set({ y: window.pageYOffset });
    window.addEventListener('scroll', this.listener);
}

...scrolling is again smooth as butter. When checking from the REPL, the code generated by that binding does have some kind of scrolljacking logic in it. That would be my first guess.

Well that's odd! That's basically all the built-in scroll binding is doing, as far as I can tell — what am I missing?

@mrkishi
Copy link
Member

mrkishi commented Oct 15, 2018

Probably something like this?

// <svelte:window bind:scrollY="y"/>
	function onwindowscroll(event) {
		if (window_updating) return;
		window_updating = true;
		component._updatingReadonlyProperty = true;

		component.set({
			y: this.pageYOffset
		});

		component._updatingReadonlyProperty = false;
		window_updating = false;
	}
	window.addEventListener("scroll", onwindowscroll);

	component.on("state", ({ changed, current }) => {
--		if (changed["y"]) {
++		if (changed["y"] && !window_updating) {
			window_updating = true;
			clearTimeout(window_updating_timeout);
			window.scrollTo(window.pageXOffset, current["y"]);
			window_updating_timeout = setTimeout(clear_window_updating, 100);
		}
	});

@Rich-Harris
Copy link
Member

oh, right!

@andreujuanc
Copy link
Author

I see! I was not crazy after all!!! So it was just not being throttled?

@arggh
Copy link
Contributor

arggh commented Oct 15, 2018

@andreujuanc the two-way binding was a little too eager. The state change listener did not check if the value was just being updated due to scrolling event.

arggh added a commit to arggh/svelte that referenced this issue Oct 15, 2018
@arggh
Copy link
Contributor

arggh commented Oct 15, 2018

I had to verify.

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