Bug 218757

Created attachment 413707 [details]
Patch

Comment on attachment 413707 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413707&action=review

> LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:41
> +    var startPosX = targetElement.offsetLeft + 20;
> +    var startPosY = targetElement.offsetTop + 20;

This isn't going to work for something in side positioned elements. You need getBoundingClientRect().

> LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:60
> +function delay(time) {
> +    return new Promise(resolve => setTimeout(resolve, time));
> +}

This is the same as UIHelper.delayFor()

> LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:62
> +function shortScrollShouldSnapBack(targetElement, direction) {

Brace on new line.

> LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:70
> +function scrollGlideShouldScrollTo(targetElement, direction, expectedValue) {

Brace on new line.

> LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-legacy.html:36
> +            delay(0)
> +                .then(() => scrollGlideShouldScrollTo(document.scrollingElement, HORIZONTAL, "window.innerWidth"))
> +                .then(() => delay(0))
> +                .then(() => shortScrollShouldSnapBack(document.scrollingElement, HORIZONTAL))
> +                .finally(finishJSTest);

I much prefer async/await

Created attachment 413809 [details]
Patch

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 413707 [details]

Thanks for the review.

> > LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:41
> > +    var startPosX = targetElement.offsetLeft + 20;
> > +    var startPosY = targetElement.offsetTop + 20;
> 
> This isn't going to work for something in side positioned elements. You need
> getBoundingClientRect().

Okay. I've updated the code to use getBoundingClientRect(), but not when dealing with the root element. The offsets for that element are affected by the root scroll offset.

> > LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:60
> > +function delay(time) {
> > +    return new Promise(resolve => setTimeout(resolve, time));
> > +}
> 
> This is the same as UIHelper.delayFor()

I've replaced all the calls to `delay` with a call to `UIHelper.delayFor()`.

> > LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:62
> > +function shortScrollShouldSnapBack(targetElement, direction) {
> 
> Brace on new line.

Fixed.

> > LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:70
> > +function scrollGlideShouldScrollTo(targetElement, direction, expectedValue) {
> 
> Brace on new line.

Fixed.
 
> > LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-legacy.html:36
> > +            delay(0)
> > +                .then(() => scrollGlideShouldScrollTo(document.scrollingElement, HORIZONTAL, "window.innerWidth"))
> > +                .then(() => delay(0))
> > +                .then(() => shortScrollShouldSnapBack(document.scrollingElement, HORIZONTAL))
> > +                .finally(finishJSTest);
> 
> I much prefer async/await

I've modified the code to use async and wait everywhere.

Comment on attachment 413809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413809&action=review

> LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:69
> +    return new Promise((resolve, reject) => {
> +        eventSender.callAfterScrollingCompletes(resolve);
> +    })

UIHelper.waitForScrollCompletion()?

Created attachment 413924 [details]
Patch

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 413809 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413809&action=review
> 
> > LayoutTests/tiled-drawing/scrolling/scroll-snap/resources/mainframe-scroll-snap-test.js:69
> > +    return new Promise((resolve, reject) => {
> > +        eventSender.callAfterScrollingCompletes(resolve);
> > +    })
> 
> UIHelper.waitForScrollCompletion()?

Oh. Nice catch. I've started using this here.

Committed r269730: <https://trac.webkit.org/changeset/269730>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413924 [details].

<rdar://problem/71322154>