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

scroll an element into view args #3131

Merged
merged 3 commits into from Jun 11, 2018

Conversation

5 participants
@zcorpan
Member

zcorpan commented Oct 16, 2017

This should fix #464 (although #2787 also adds another entry point for 'scroll an element into view', which 0cc76be fixes for that PR).

@annevk

annevk approved these changes Oct 16, 2017

It seems nicer those if the scroll an element into view algorithm would have these as default. Or is HTML the only caller that needs these defaults?

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Oct 16, 2017

Member

For focus(), block and inline should be undefined (for now, at least). It's probably confusing to have an explicit undefined vs argument not passed semantic difference.

Member

zcorpan commented Oct 16, 2017

For focus(), block and inline should be undefined (for now, at least). It's probably confusing to have an explicit undefined vs argument not passed semantic difference.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 16, 2017

Member

Yeah, that would be confusing.

Member

annevk commented Oct 16, 2017

Yeah, that would be confusing.

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Oct 16, 2017

Member

I found a related issue in Chromium for scrollPathIntoView(), https://bugs.chromium.org/p/chromium/issues/detail?id=775024

I did not find any tests for scrollPathIntoView() in web-platform-tests. :-(

Member

zcorpan commented Oct 16, 2017

I found a related issue in Chromium for scrollPathIntoView(), https://bugs.chromium.org/p/chromium/issues/detail?id=775024

I did not find any tests for scrollPathIntoView() in web-platform-tests. :-(

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 16, 2017

Member

I don't think we should block on that given that this is a clarification, but I'll let @domenic make that call. We should maybe raise an issue against web-platform-tests although generally those tend to go nowhere for a long time.

Member

annevk commented Oct 16, 2017

I don't think we should block on that given that this is a clarification, but I'll let @domenic make that call. We should maybe raise an issue against web-platform-tests although generally those tend to go nowhere for a long time.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 17, 2017

Member

I think it's always good to take opportunities like this to write web platform tests if you're up for it, but if you're not, filing an issue seems fine, yeah. Merge whenever you're comfortable :)

Member

domenic commented Oct 17, 2017

I think it's always good to take opportunities like this to write web platform tests if you're up for it, but if you're not, filing an issue seems fine, yeah. Merge whenever you're comfortable :)

@fserb

This comment has been minimized.

Show comment
Hide comment
@fserb

fserb Oct 20, 2017

Contributor

I just sent a change for review to comply to this on Chrome.

That said, a few points: I'm not sure what scrollMode always means. I don't see it on the CSS spec and it's a weird thing for nearest scrolling. Could you clarify to me where it is defined?

Contributor

fserb commented Oct 20, 2017

I just sent a change for review to comply to this on Chrome.

That said, a few points: I'm not sure what scrollMode always means. I don't see it on the CSS spec and it's a weird thing for nearest scrolling. Could you clarify to me where it is defined?

Show outdated Hide outdated source
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 26, 2017

Member

Marking do not merge yet per the fact that scrollMode has been removed.

Member

domenic commented Oct 26, 2017

Marking do not merge yet per the fact that scrollMode has been removed.

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Nov 3, 2017

Member

@fserb I've removed scrollMode here.

It looks like there's a test at https://chromium-review.googlesource.com/c/chromium/src/+/731106/7/third_party/WebKit/LayoutTests/fast/canvas-api/script-tests/canvas-scroll-path-into-view.js -- would it be possible to upstream that to web-platform-tests, or alternatively file an issue under web-platform-tests to follow up on doing that later?

Member

zcorpan commented Nov 3, 2017

@fserb I've removed scrollMode here.

It looks like there's a test at https://chromium-review.googlesource.com/c/chromium/src/+/731106/7/third_party/WebKit/LayoutTests/fast/canvas-api/script-tests/canvas-scroll-path-into-view.js -- would it be possible to upstream that to web-platform-tests, or alternatively file an issue under web-platform-tests to follow up on doing that later?

@nanto

This comment has been minimized.

Show comment
Hide comment
@nanto

nanto May 3, 2018

I tried to write tests in web-platform-tests/wpt#10828.

nanto commented May 3, 2018

I tried to write tests in web-platform-tests/wpt#10828.

@zcorpan zcorpan removed the needs tests label Jun 6, 2018

@zcorpan zcorpan merged commit c50e528 into master Jun 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zcorpan zcorpan deleted the zcorpan/scroll-an-element-into-view-args branch Jun 11, 2018

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Jun 11, 2018

Member

Thanks @nanto!

Member

zcorpan commented Jun 11, 2018

Thanks @nanto!

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Jun 11, 2018

"Scroll an element into view" in HTML spec
I use assert_approx_equals here since properties of DOMRect are of type double.

For whatwg/html#3131.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 11, 2018

Bug 1459015 [wpt PR 10828] - "Scroll an element into view" in HTML sp…
…ec, a=testonly

Automatic update from web-platform-tests"Scroll an element into view" in HTML spec

I use assert_approx_equals here since properties of DOMRect are of type double.

For whatwg/html#3131.
--

wpt-commits: 32f74fe70209b3df142a03957779a6d776a6f74a
wpt-pr: 10828

mykmelez pushed a commit to mozilla/gecko that referenced this pull request Jul 13, 2018

Bug 1459015 [wpt PR 10828] - "Scroll an element into view" in HTML sp…
…ec, a=testonly

Automatic update from web-platform-tests"Scroll an element into view" in HTML spec

I use assert_approx_equals here since properties of DOMRect are of type double.

For whatwg/html#3131.
--

wpt-commits: 32f74fe70209b3df142a03957779a6d776a6f74a
wpt-pr: 10828
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment