User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36 Steps to reproduce: See spec test result here: http://wpt.fyi/cssom-view/scrollIntoView-empty-args.html Actual results: The scroll goes to "nearest" or "end". I am not sure but either way it is incorrect. Expected results: In particular the tests fail when input is either "null" or {}. In this case according to the spec [1] the method should use default values in ScrollIntoViewOptions which is "center". dictionary ScrollIntoViewOptions : ScrollOptions { ScrollLogicalPosition block = "center"; ScrollLogicalPosition inline = "center"; }; [1] https://drafts.csswg.org/cssom-view/#dictdef-scrollintoviewoptions

FYI, I found that the idl used by FF [1] is different from spec which may explain this: // http://dev.w3.org/csswg/cssom-view/ enum ScrollLogicalPosition { "start", "end" }; dictionary ScrollIntoViewOptions : ScrollOptions { ScrollLogicalPosition block = "start"; }; http://searchfox.org/mozilla-central/source/dom/webidl/Element.webidl#174

bz, I'm not sure how we can proceed here without changing our webidl binding generators. The IDL is supposed to be this [1]: >enum ScrollLogicalPosition { "start" , "center", "end", "nearest" }; >dictionary ScrollIntoViewOptions : ScrollOptions { > ScrollLogicalPosition block = "center"; > ScrollLogicalPosition inline = "center"; >}; So if an empty object/dict is passed in as a parameter, it will default to using center/center for the dict's entries. Unfortunately, when nothing is passed in, it's supposed to instead default to start/end [2]. Unfortunately, we generate code that doesn't pass in an optional dict, but rather always passes in a dict with default center/center values, whether the user passed anything in or passed in an empty dict (that is, it calls ScrollIntoView({block:"center",inline:"center"}) when the user calls scrollIntoView()). As such it's impossible to distinguish the cases. Is there a way to tweak the definitions to deal with this, or would it require more drastic changes? Note that I have no problem making a patch which partially fixes the problem and passes more WPTs, but fails to default to "center" if a dict is passed without one of the values, but this strikes me as something we might as well fix properly the first time. Thoughts/insights? [1] https://drafts.csswg.org/cssom-view/#dictdef-scrollintoviewoptions [2] https://drafts.csswg.org/cssom-view/#dom-element-scrollintoview

> I'm not sure how we can proceed here without changing our webidl binding generators We don't need to change the generator. We do need to change the IDL involved. Our IDL says: void scrollIntoView(boolean top); void scrollIntoView(optional ScrollIntoViewOptions options); but the spec's says: void scrollIntoView(); void scrollIntoView((boolean or object) arg); precisely because the spec wants `scrollIntoView()` and `scrollIntoView({})` to have different behavior, which is not something dictionaries normally do. Then the algorithm prose (aka C++ code in our case), if arg is an object, converts it to a ScrollIntoViewOptions dictionary by hand.

Ah, thanks for that, bz. I'll get the hang of IDL someday! One of the reasons I was thrown off is that the web platform test cssom-view/scrollIntoView-empty-args.html is telling me to treat scrollIntoView(null) as I would treat scrollIntoView({}), and scrollIntoView((undefined) as I would scrollIntoView(). However, given that IDL definitions, they should both be treated as scrollIntoView(false), unless I'm again misreading the spec. As such I'm just going to have to change that test. I don't see any browsers fully passing the test yet anyway, even if Chrome passes the null case.

Alright, I have a try-run for this here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a6d28817c494ebf7e0ff512cc4b175d818ca4bc bkelly, since bz is out on vacation, I'm hoping you're a suitable reviewer for the IDL stuff. annevk, I hope the web platform test changes are suitable. As I mentioned in comment 4, they weren't doing what the IDL spec seems to imply they should be doing for two of the cases, and I also updated the Shadow DOM one to be more robust (since the test seems ok to pass if Shadow DOM isn't supported, and wasn't expecting v1, just v0). Note that I also had to update one call in the toolkit code to specify which type of scrollIntoView it did, since the IDL changed the default that it was expecting. I didn't see any other callsites in the codebase where a default wasn't provided or would affect the outcome.

I think the specification is wrong. Why can't it be: void scrollIntoView(optional (boolean or ScrollToOptions) arg = false); And the dictionary having different defaults than false also seems wrong. Passing {} shouldn't have different results from passing undefined. Filed https://github.com/w3c/csswg-drafts/issues/1720.

See also https://github.com/w3c/csswg-drafts/pull/1505 (apparently the default is true).

Comment on attachment 8896774 [details] Bug 1389274 - Correct the behavior of Element.scrollIntoView to match the draft spec and pass web platform tests; https://reviewboard.mozilla.org/r/168066/#review173532 r=me with comments addressed. ::: dom/base/Element.h:1033 (Diff revision 1) > return slots ? slots->mShadowRoot.get() : nullptr; > } > > void ScrollIntoView(); > - void ScrollIntoView(bool aTop); > + void ScrollIntoView(JSContext* aCx, const BooleanOrObject& aObject); > void ScrollIntoView(const ScrollIntoViewOptions &aOptions); You can make this private and "internal" now, right? AFAICT the binding code should always be calling into the union version of the method above. Correct? ::: dom/base/Element.cpp:748 (Diff revision 1) > + JS::RootedValue value(aCx, JS::ObjectValue(*aObject.GetAsObject())); > + if (NS_WARN_IF(!options.Init(aCx, value))) { > + return; > + } > + } > + if (aObject.IsBoolean()) { It would be a bit clearer here to use "shortcut style" conditionals. So just do `return ScrollIntoView(options)` immediately in the first object block. The you can `MOZ_DIAGNOSTIC_ASSERT(aObject.IsBoolean())` and perform the boolean logic without nesting. ::: dom/base/Element.cpp:774 (Diff revision 1) > nsCOMPtr<nsIPresShell> presShell = document->GetShell(); > if (!presShell) { > return; > } > > - int16_t vpercent = (aOptions.mBlock == ScrollLogicalPosition::Start) > + int16_t vpercent; Please initialize this to somthing. I know it will get set below, but its preferable to always initialize. ::: dom/base/Element.cpp:776 (Diff revision 1) > return; > } > > - int16_t vpercent = (aOptions.mBlock == ScrollLogicalPosition::Start) > - ? nsIPresShell::SCROLL_TOP > - : nsIPresShell::SCROLL_BOTTOM; > + int16_t vpercent; > + switch (aOptions.mBlock) { > + case ScrollLogicalPosition::Nearest: Please order case statements in the switch in the same order the enumeration is defined. This can help the compiler to generate a more efficient jump table. Also, I think it would be better to explicitly specify the `ScrollLogicalPosition::Center` case and then do MOZ_ASSERT_UNREACHED() in the default case. ::: dom/base/Element.cpp:789 (Diff revision 1) > + break; > + default: > + vpercent = nsIPresShell::SCROLL_CENTER; > + } > + > + int16_t hpercent; Same comment about initializing this variable.

Thanks guys! Anne, based on your suggested IDL, this is what will happen for each case: scrollIntoView(true) -> block=start, inline=nearest scrollIntoView() -> same as (true) scrollIntoView(null) -> same as (true) scrollIntoView({}) -> same as (true) scrollIntoView(false) -> block=end, inline=nearest scrollIntoView(undefined) -> same as (false) scrollIntoView({block:x}) -> block=x, inline=nearest scrollIntoView({inline:x}) -> block=start, inline=x This seems fine to me, but thought I'd confirm in case any of them seem off to you. If it's fine then I'll update the patch while we wait for spec feedback.

> scrollIntoView(undefined) -> same as (false) This should be the same as true. Since undefined and omitting the argument are supposed to be identical (only a couple of exceptions to this rule and this does not need to be one of them). Looks fine otherwise.

Comment on attachment 8896774 [details] Bug 1389274 - Correct the behavior of Element.scrollIntoView to match the draft spec and pass web platform tests; Clearing review flag for now.

Just a quick update, I note that the IDL is changing in https://github.com/w3c/csswg-drafts/pull/1505, to: > dictionary ScrollIntoViewOptions : ScrollOptions { > ScrollLogicalPosition block = "start"; > ScrollLogicalPosition inline = "nearest"; > }; > void scrollIntoView(optional (boolean or ScrollIntoViewOptions) arg); I have a revised patch waiting which implements that version, but will wait for the web platform tests to be updated as well (to reflect that the {}, null and undefined cases will all act as the true case does; right now the in-tree WPTs expect {} and null to center instead). A try run of the revised patch shows only unrelated intermittents failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba2c8946956d169328a02d0db0b4cbd761643a72

https://github.com/w3c/csswg-drafts/pull/1505 is now merged FYI.

Anne, whom should I ping to update our in-tree tests to pick up the update that landed 15 days ago? (Or should I just land this patch with the version of the test that's in git/master, and things will work out?)

Actually, disregard. The test seems to be up-to-date, but it's behaving differently from what I thought we agreed on (or there's a bug in my patch). I'll investigate.

Normally jgraham syncs the tests. He's on leave right now, though. And in general, if you do land things with the same version as in git/master things will work out when the merge happens.

@zcorpan, what's the current scoop with scrollIntoView? I thought the scrollIntoView({}) and (null) cases were now meant behave like true/undefined, as of https://github.com/w3c/csswg-drafts/issues/1720 and https://github.com/w3c/csswg-drafts/issues/1505 ? Yet scrollIntoView-empty-args.html is still expecting center/center for those values: https://github.com/w3c/web-platform-tests/blob/master/cssom-view/scrollIntoView-empty-args.html Did some wires just get crossed? If so, I don't mind submitting those changes as part of my patch here, if that's alright.

I missed updating that test it seems. https://github.com/w3c/web-platform-tests/blob/master/cssom-view/scrollintoview.html should be correct. The scrollIntoView-empty-args.html test could either be folded in to scrollintoview.html (if it covers something not already covered in the latter), or fixed, or removed.

Ah, makes sense. No, I don't think that test is doing anything that isn't covered by the new one, so we can just remove it outright. I'll just update my patch to do that, since it's otherwise ready for check-in. (I'm just giving it a final look).

Given that annevk and bkelly were fine with the changes, and all I've altered since then is to remove a web platform test (which zcorpan agrees with), I'm going to carry over the r+ and request checkin.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 0346ef159156 -d 8bfe747029e1: rebasing 422779:0346ef159156 "Bug 1389274 - Correct the behavior of Element.scrollIntoView to match the draft spec and pass web platform tests; r=bkelly" (tip) merging dom/base/Element.cpp merging testing/web-platform/meta/MANIFEST.json warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Love hitting those long odds and getting a merge clash. I've just rebased the patch... let's try again, shall we?

Comment on attachment 8896774 [details] Bug 1389274 - Correct the behavior of Element.scrollIntoView to match the draft spec and pass web platform tests; https://reviewboard.mozilla.org/r/168066/#review189114 Not super happy with how shadow trees are tested, but I'm guessing that's not your fault. ::: dom/base/Element.cpp:734 (Diff revision 3) > > void > -Element::ScrollIntoView() > -{ > - ScrollIntoView(ScrollIntoViewOptions()); > +Element::ScrollIntoView(const BooleanOrScrollIntoViewOptions& aObject) { > + if (aObject.IsScrollIntoViewOptions()) { > + return ScrollIntoView(aObject.GetAsScrollIntoViewOptions()); > -} > + } Looks like you missed something during rebasing? Oh, this also happens below. Why would we display whitespace using >? ::: testing/web-platform/tests/cssom-view/scrollIntoView-shadow.html:38 (Diff revision 3) > - shadowDiv.scrollIntoView({block: "start", inline: "start"}); > + shadowDiv.scrollIntoView({block: "start", inline: "start"}); > - assert_approx_equals(window.scrollX, expected_x, 1); > + assert_approx_equals(window.scrollX, expected_x, 1); > - assert_approx_equals(window.scrollY, expected_y, 1); > + assert_approx_equals(window.scrollY, expected_y, 1); > + } else { > + assert_true(true, "No Shadow DOM, no fault"); > + } Making shadow trees optional is dubious and createShadowRoot is even more dubious (it's a proprietary API). But I guess this is from upstream? Hmm.

Anne, I don't mind reverting the shadow-DOM test's changes, since it doesn't look like it has been checked in yet. Just let me know!

So where are those changes coming from then?

I think in hindsight I'm just going a bit too far with this patch, and changing a test that I should just leave alone. I'll take checkin-needed off for a moment while I remove those changes from the patch. I just was not sure why a browser that has no shadow DOM support should fail that test, given that I recalled other tests being ignored when a browser does not yet support a feature (like ReadableStreams when testing XHR code).

I see. In the future please consider any such instances bugs to be fixed. Tests shouldn't have optional features. (There might be exceptions, but I can't think of any offhand.)

Alright, I've removed those un-needed shadow-related bits from the patch. Requesting checkin.

Autoland can't push this while there's still unresolved issues in MozReview.

Comment on attachment 8896774 [details] Bug 1389274 - Correct the behavior of Element.scrollIntoView to match the draft spec and pass web platform tests; https://reviewboard.mozilla.org/r/168066/#review189114 > Looks like you missed something during rebasing? Oh, this also happens below. Why would we display whitespace using >? Hmm, it looks like it's just a weirdly-formatted diff, presumably caused by my not having a { down on the next line. The code itself looks fine (I got rid of ScrollIntoView() and ScrollIntoView(bool), in favor of ScrollIntoView(BooleanOrScrollIntoViewOptions). I'll update the patch to move the { down; it should make this change clearer.

Hmm, strange that I missed those notes. Anne, if you're not too sick of this patch already, would you mind taking one last look to make sure I'm not accidentally missing something else that needs review?

Looks fine. (I attached a screenshot to demonstrate what I meant with the ">>" indicating whitespace. That's some kind of issue with the review tool.)

Thanks Anne. The patch still applies cleanly for me on central right now, so requesting checkin.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8a970e561fe1 Correct the behavior of Element.scrollIntoView to match the draft spec and pass web platform tests; r=annevk,bkelly

Backed out for unexpected passes of web-platform-test /cssom-view/scrollIntoView-shadow.html: https://hg.mozilla.org/integration/autoland/rev/489a4a560c60f2eb0f0a145ab01626f798247498 Push which ran unexpectedly passing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dd4eea6b8ae40bc9d304ebe8c33fb5f48e677360&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Test log: https://treeherder.mozilla.org/logviewer.html#?job_id=133869526&repo=autoland [task 2017-09-28T17:29:59.543Z] 17:29:59 INFO - TEST-START | /cssom-view/scrollIntoView-shadow.html [task 2017-09-28T17:29:59.604Z] 17:29:59 INFO - PID 3520 | 1506619799600 Marionette DEBUG Register listener.js for window 2147483734 [task 2017-09-28T17:29:59.701Z] 17:29:59 INFO - [task 2017-09-28T17:29:59.701Z] 17:29:59 INFO - TEST-UNEXPECTED-PASS | /cssom-view/scrollIntoView-shadow.html | scrollIntoView should behave correctly if applies to shadow dom elements - expected FAIL [task 2017-09-28T17:29:59.701Z] 17:29:59 INFO - TEST-INFO | expected FAIL

I have no idea what's going on here. My patch doesn't touch that file, and the test fails locally with "shadow.createShadowRoot is not a function". Why on earth would those try servers get past that point? Any ideas, bkelly?

Thomas, shadow DOM is disabled when stylo is enabled. Otherwise it's enabled in our test harnesses. The unexpected passes are on the stylo-disabled test jobs. The simplest thing to do would be to change "expected: FAIL" to: expected: if stylo: FAIL for the test. And maybe add "Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1293844" in there somewhere.

Ah, I see, thanks bz. I've updated the patchset as you suggested. Let's try again...

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2a91649c5285 Correct the behavior of Element.scrollIntoView to match the draft spec and pass web platform tests; r=annevk,bkelly