[cssom-view] Change scrollIntoView(undefined) to be like true #1505

zcorpan · 2017-06-07T13:14:33Z

This aligns with the behavior of Gecko/WebKit/Chromium; EdgeHTML
treats explicit undefined like false.

Fixes #1367.

Tests: web-platform-tests/wpt#6253

Follows w3c/csswg-drafts#1505

zcorpan · 2017-06-21T07:38:41Z

Ping @travisleithead - is this change OK for Edge?

zcorpan · 2017-08-23T19:09:51Z

https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/13398547/

annevk · 2017-09-08T08:17:39Z

cssom-view/Overview.bs

@@ -1082,8 +1082,7 @@ dictionary ScrollIntoViewOptions : ScrollOptions {
 partial interface Element {
  sequence&lt;DOMRect> getClientRects();
  [NewObject] DOMRect getBoundingClientRect();
-  void scrollIntoView();
-  void scrollIntoView((boolean or object) arg);
+  void scrollIntoView(optional (boolean or ScrollIntoViewOptions) arg = true);


I don't think you need to define a default here. I had a similar situation with addEventListener. Basically if no argument is given you'll end up getting a dictionary passed to your algorithm and that dictionary should have the appropriate defaults already. Does that make sense?

annevk · 2017-09-08T08:18:47Z

cssom-view/Overview.bs

+1. If <var>arg</var> is a {{ScrollIntoViewOptions}} dictionary, set <var>options</var> to <var>arg</var>. Otherwise:
+    1. <a lt="converted to an IDL value">Convert</a> <var>options</var> to a {{ScrollIntoViewOptions}} dictionary. [[!WEBIDL]]
+    1. If <var>arg</var> is true, set the {{ScrollIntoViewOptions/block}} dictionary member of <var>options</var> to "<code>start</code>", and set the {{ScrollIntoViewOptions/inline}} dictionary member of <var>options</var> to "<code>nearest</code>".
+    1. If <var>arg</var> is false, let the {{ScrollIntoViewOptions/block}} dictionary member of <var>options</var> to "<code>end</code>", and set the {{ScrollIntoViewOptions/inline}} dictionary member of <var>options</var> to "<code>nearest</code>".


This should basically no longer be needed. All you need is some logic for what to do when arg is a boolean. Or is there another caller of sorts?

annevk · 2017-09-08T08:19:17Z

cssom-view/Overview.bs

+<code>false</code> is equivalent to
+<code>{&nbsp;block:&nbsp;"end",&nbsp;inline:&nbsp;"nearest"&nbsp;}</code>. Since centering is a
+common case, passing in an empty dictionary <code>{}</code> is equivalent to
+<code>{&nbsp;block:&nbsp;"center",&nbsp;inline:&nbsp;"center"&nbsp;}</code>.


I thought we wouldn't do this?

zcorpan · 2017-09-08T09:16:54Z

Anne, this is for fixing #1367 -- my thought was that the fix for #1720 would be on top of this change.

annevk · 2017-09-08T09:24:46Z

Okay, isn't that a little confusing?

zcorpan · 2017-09-08T09:31:53Z

Maybe it is, yeah. I can address both issues at once. I'll push new commits here and update the tests.

This aligns with the behavior of Gecko/WebKit/Chromium; EdgeHTML treats explicit undefined like false. Fixes #1367. Tests: web-platform-tests/wpt#6253

Fixes #1720.

Follows w3c/csswg-drafts#1505

zcorpan · 2017-09-08T11:23:42Z

@annevk PTAL

annevk

let/set nit must be fixed, the rest would be nice.

annevk · 2017-09-08T11:36:58Z

cssom-view/Overview.bs

-1. If <var>arg</var> is false, let the {{ScrollIntoViewOptions/block}} dictionary member of <var>options</var> have the value "<code>end</code>", and let the {{ScrollIntoViewOptions/inline}} dictionary member of <var>options</var> have the value "<code>nearest</code>".
+1. If <var>arg</var> is a {{ScrollIntoViewOptions}} dictionary, set <var>options</var> to <var>arg</var>. Otherwise:
+    1. <a lt="converted to an IDL value">Convert</a> <var>options</var> to a {{ScrollIntoViewOptions}} dictionary. [[!WEBIDL]]
+    1. If <var>arg</var> is false, let the {{ScrollIntoViewOptions/block}} dictionary member of <var>options</var> to "<code>end</code>".


annevk · 2017-09-08T11:37:28Z

cssom-view/Overview.bs

-1. <a lt="converted to an IDL value">Convert</a> <var>options</var> to a {{ScrollIntoViewOptions}} dictionary. [[!WEBIDL]]
-1. If <var>arg</var> is not specified or is true, let the {{ScrollIntoViewOptions/block}} dictionary member of <var>options</var> have the value "<code>start</code>", and let the {{ScrollIntoViewOptions/inline}} dictionary member of <var>options</var> have the value "<code>nearest</code>".
-1. If <var>arg</var> is false, let the {{ScrollIntoViewOptions/block}} dictionary member of <var>options</var> have the value "<code>end</code>", and let the {{ScrollIntoViewOptions/inline}} dictionary member of <var>options</var> have the value "<code>nearest</code>".
+1. If <var>arg</var> is a {{ScrollIntoViewOptions}} dictionary, set <var>options</var> to <var>arg</var>. Otherwise:


seems clearer if Otherwise is a new step

annevk · 2017-09-08T11:38:30Z

cssom-view/Overview.bs

+dictionary <code>{}</code> to {{Element/scrollIntoView()}} is equivalent to
+<code>{&nbsp;block:&nbsp;"start",&nbsp;inline:&nbsp;"nearest"&nbsp;}</code>. Passing in
+<code>false</code> is equivalent to
+<code>{&nbsp;block:&nbsp;"end",&nbsp;inline:&nbsp;"nearest"&nbsp;}</code>.


I'm not sure this is needed as we don't explain all the other nuances of IDL either.

annevk · 2017-09-08T11:39:50Z

cssom-view/Overview.bs

-1. If <var>arg</var> is not specified or is true, let the {{ScrollIntoViewOptions/block}} dictionary member of <var>options</var> have the value "<code>start</code>", and let the {{ScrollIntoViewOptions/inline}} dictionary member of <var>options</var> have the value "<code>nearest</code>".
-1. If <var>arg</var> is false, let the {{ScrollIntoViewOptions/block}} dictionary member of <var>options</var> have the value "<code>end</code>", and let the {{ScrollIntoViewOptions/inline}} dictionary member of <var>options</var> have the value "<code>nearest</code>".
+1. If <var>arg</var> is a {{ScrollIntoViewOptions}} dictionary, set <var>options</var> to <var>arg</var>. Otherwise:
+    1. <a lt="converted to an IDL value">Convert</a> <var>options</var> to a {{ScrollIntoViewOptions}} dictionary. [[!WEBIDL]]


I wish this could be avoided somehow, but I guess it's okay. It's a little ugly to invoke JS->IDL conversions when you're supposedly already in IDL-prose land.

…iew'

annevk · 2017-09-08T12:30:55Z

cssom-view/Overview.bs

@@ -1301,7 +1298,9 @@ The <dfn attribute for=Element>clientHeight</dfn> attribute must run these steps
 <h3 id=element-scrolling-members>{{Element}} Scrolling Members</h3>

 To <dfn>scroll an element into view</dfn> <var>element</var>,
-with a {{ScrollIntoViewOptions}} dictionary <var>options</var>,
+with a scroll behavior <var>behavior</var>,
+a block flow direction position <var>block</var>,


and an optional block flow ... and an inline base ...

Not sure I follow. Would you like me to tweak the wording? Or make these optional? Both?

I think you treat the second and third argument as optional because there's one invocation that just invokes this with "auto", afaict.

I don't see that. Within cssom-view it's only invoked here

1. <a lt='scroll an element into view'>Scroll the element into view</a> with <var>behavior</var>, <var>block</var>, and <var>inline</var>.

Yeah you're right. I got confused with "scroll an element".

…ewOptions Follows w3c/csswg-drafts#1505 w3c/csswg-drafts#1720

zcorpan · 2017-09-11T14:28:13Z

https://bugzilla.mozilla.org/show_bug.cgi?id=1389274
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/13398547/
https://bugs.webkit.org/show_bug.cgi?id=161611
https://bugs.chromium.org/p/chromium/issues/detail?id=764282

…ewOptions Follows w3c/csswg-drafts#1505 w3c/csswg-drafts#1720

zcorpan mentioned this pull request Jun 7, 2017

[cssom-view-1] Can scrollIntoView param type be more specifc? #1367

Closed

syncbot force-pushed the zcorpan/scrollintoview-undefined-like-true branch from 21c997e to 763f7e7 Compare June 8, 2017 19:22

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

CSSOM View: Expand on scrollIntoView test

a31d4f3

Follows w3c/csswg-drafts#1505

zcorpan mentioned this pull request Jun 15, 2017

CSSOM View: Expand on scrollIntoView test web-platform-tests/wpt#6253

Merged

zcorpan force-pushed the zcorpan/scrollintoview-undefined-like-true branch from 763f7e7 to 0c667c7 Compare June 21, 2017 06:22

syncbot force-pushed the zcorpan/scrollintoview-undefined-like-true branch from 0c667c7 to 763f7e7 Compare July 13, 2017 14:20

zcorpan requested a review from bzbarsky August 25, 2017 08:50

zcorpan mentioned this pull request Sep 8, 2017

[cssom-view] scrollIntoView({}) dictionary default should match scrollIntoView(true) #1720

Closed

annevk reviewed Sep 8, 2017

View reviewed changes

zcorpan added 2 commits September 8, 2017 12:29

[cssom-view] Change scrollIntoView(undefined) to be like true

27591aa

This aligns with the behavior of Gecko/WebKit/Chromium; EdgeHTML treats explicit undefined like false. Fixes #1367. Tests: web-platform-tests/wpt#6253

[cssom-view] Change defaults for ScrollIntoViewOptions

73f06e5

Fixes #1720.

zcorpan force-pushed the zcorpan/scrollintoview-undefined-like-true branch from 763f7e7 to 73f06e5 Compare September 8, 2017 10:38

zcorpan added the waiting for tests label Sep 8, 2017

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Sep 8, 2017

CSSOM View: Expand on scrollIntoView test

4ff9317

Follows w3c/csswg-drafts#1505

zcorpan removed the waiting for tests label Sep 8, 2017

annevk approved these changes Sep 8, 2017

View reviewed changes

zcorpan added 2 commits September 8, 2017 14:29

Address comments. This changes the input to 'scroll an element into v…

f02a15d

…iew'

Fixup

51eef11

annevk approved these changes Sep 8, 2017

View reviewed changes

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

CSSOM View: scrollIntoView tests for changed defaults of ScrollIntoVi…

4f74ec7

…ewOptions Follows w3c/csswg-drafts#1505 w3c/csswg-drafts#1720

zcorpan merged commit 232957f into master Sep 11, 2017

zcorpan deleted the zcorpan/scrollintoview-undefined-like-true branch September 11, 2017 14:25

zcorpan mentioned this pull request Sep 12, 2017

[cssom-view] Default values of ScrollIntoViewOptions contradicts in spec #1353

Closed

zcorpan mentioned this pull request Sep 12, 2017

Change invocation of cssom-view "scroll an element into view" whatwg/html#464

Closed

rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this pull request Nov 8, 2017

CSSOM View: scrollIntoView tests for changed defaults of ScrollIntoVi…

5ef42a5

…ewOptions Follows w3c/csswg-drafts#1505 w3c/csswg-drafts#1720

jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017

CSSOM View: scrollIntoView tests for changed defaults of ScrollIntoVi…

1ba5675

…ewOptions Follows w3c/csswg-drafts#1505 w3c/csswg-drafts#1720

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cssom-view] Change scrollIntoView(undefined) to be like true #1505

[cssom-view] Change scrollIntoView(undefined) to be like true #1505

zcorpan commented Jun 7, 2017 •

edited

Loading

zcorpan commented Jun 21, 2017

zcorpan commented Aug 23, 2017

annevk Sep 8, 2017

annevk Sep 8, 2017

annevk Sep 8, 2017

zcorpan commented Sep 8, 2017

annevk commented Sep 8, 2017

zcorpan commented Sep 8, 2017

zcorpan commented Sep 8, 2017

annevk left a comment

annevk Sep 8, 2017

annevk Sep 8, 2017

annevk Sep 8, 2017

annevk Sep 8, 2017

annevk Sep 8, 2017

zcorpan Sep 8, 2017

annevk Sep 8, 2017

zcorpan Sep 11, 2017

annevk Sep 11, 2017

zcorpan commented Sep 11, 2017 •

edited

Loading

[cssom-view] Change scrollIntoView(undefined) to be like true #1505

[cssom-view] Change scrollIntoView(undefined) to be like true #1505

Conversation

zcorpan commented Jun 7, 2017 • edited Loading

zcorpan commented Jun 21, 2017

zcorpan commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcorpan commented Sep 8, 2017

annevk commented Sep 8, 2017

zcorpan commented Sep 8, 2017

zcorpan commented Sep 8, 2017

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcorpan commented Sep 11, 2017 • edited Loading

zcorpan commented Jun 7, 2017 •

edited

Loading

zcorpan commented Sep 11, 2017 •

edited

Loading