Oriol Brufau [:Oriol] Assignee
	Description • 7 years ago

If the element is bigger than the scrolling box, scrollIntoView in "nearest" mode scrolls to the start edge.

This is wrong, from https://drafts.csswg.org/cssom-view/#element-scrolling-members
> - If element edge A and element edge B are both outside
>   scrolling box edge A and scrolling box edge B
>     Do nothing.
> - If element edge A is outside scrolling box edge A
>   and element width is less than scrolling box width
> - If element edge B is outside scrolling box edge B
>   and element width is greater than scrolling box width
>     Align element edge A with scrolling box edge A.
> - If element edge A is outside scrolling box edge A
>   and element width is greater than scrolling box width
> - If element edge B is outside scrolling box edge B
>   and element width is less than scrolling box width
>     Align element edge B with scrolling box edge B.

Load the attached testcase.
Result: red.
Expected: green, like Chrome

	Oriol Brufau [:Oriol] Assignee
	Comment 3 • 7 years ago

Hopefully some web platform test will unexpectedly pass and that will be enough.
Otherwise what should I do, use my testcase as a reftest?

	Oriol Brufau [:Oriol] Assignee
	Comment 5 • 7 years ago

I fixed test_scroll_selection_into_view.html, is this enough as a test?
Also modified various comments.

	Anne (:annevk)
	Comment 6 • 7 years ago

Comment on attachment 8927517 [details]
Bug 1416391 - Fix Element.scrollIntoView in "nearest" mode to behave according to CSSOM spec.

I only reviewed web-platform-tests changes for this method. I cannot review code or layout tests.

	Thomas Wisniewski
	Comment 7 • 7 years ago

Comment on attachment 8927517 [details]
Bug 1416391 - Fix Element.scrollIntoView in "nearest" mode to behave according to CSSOM spec.

I don't think my r+ counts for actually landing patches, so I'm changing the request over to bkelly. That said, this seems fine to me in general, so I'll feedback+ at least.

Anne, was there a web platform test for this that I'm not quite seeing, or should we file a follow-up bug to add one?

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 7 years ago

Comment on attachment 8927517 [details]
Bug 1416391 - Fix Element.scrollIntoView in "nearest" mode to behave according to CSSOM spec.

Sorry, but I'm going to hot potato this over to a layout peer.  I don't feel qualified to review it.

My one comment, though, is it would be nice to have web-platform-tests.

	Anne (:annevk)
	Comment 9 • 7 years ago

Thomas, not sure. https://github.com/w3c/web-platform-tests/tree/master/css/cssom-view has various test files for this method that we could presumably extend if needed.

	Timothy Nikkel (:tnikkel)
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8927517 [details]
Bug 1416391 - Fix Element.scrollIntoView in "nearest" mode to behave according to CSSOM spec.

https://reviewboard.mozilla.org/r/198828/#review207702

Makes sense. Might be some unexpected fallout from this though.

	Oriol Brufau [:Oriol] Assignee
	Comment 11 • 7 years ago

The "nearest" was added in firefox58. Should the patch be uplifted, to avoid landing a wrong implementation, or is it too risky?

	Pulsebot
	Comment 12 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/184697371b6b
Fix Element.scrollIntoView in "nearest" mode to behave according to CSSOM spec. r=tnikkel

	Thomas Wisniewski
	Comment 13 • 7 years ago

(In reply to Oriol Brufau [:Oriol] from comment #11)
> The "nearest" was added in firefox58. Should the patch be uplifted, to avoid
> landing a wrong implementation, or is it too risky?

FWIW, I'm not against an uplift it (assuming our tests pass of course), but I'd only bother if it definitely improves our interop with current browsers (the spec itself has been clarifying this kind of behavior lately, and I'm not sure whether it's worth it unless everyone else is already doing these measurements correctly).

	Oriol Brufau [:Oriol] Assignee
	Comment 14 • 7 years ago

(In reply to Thomas Wisniewski from comment #13)
> I'd only bother if it definitely improves our interop with current browsers

It improves interop with Chrome. Edge does not seem to support the scrollIntoViewOptions parameter and just scrolls to top.

	Tiberius Oros[:tiberius_oros]
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/184697371b6b

	Oriol Brufau [:Oriol] Assignee
	Comment 16 • 7 years ago

Comment on attachment 8927517 [details]
Bug 1416391 - Fix Element.scrollIntoView in "nearest" mode to behave according to CSSOM spec.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1389274.
[User impact if declined]: a wrong implementation of scrollIntoView's "nearest" mode will land in firefox58. The current behavior does not follow CSSOM spec and is not compatible with Chrome, which does it right.
[Is this code covered by automated tests?]: yes.
[Has the fix been verified in Nightly?]: I have manually verified this in
Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: I guess the automated tests should be enough, but if you want to test manually see comment #0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not much risky.
[Why is the change risky/not risky?]: It only changes how an element is scrolled into view in nearest mode. The top edge is no longer forced to become visible. This behavior is already implemented by Chrome, so it shouldn't be a problem.
[String changes made/needed]: None.

	Gerry Chang [:gchang]
	Comment 17 • 7 years ago

Comment on attachment 8927517 [details]
Bug 1416391 - Fix Element.scrollIntoView in "nearest" mode to behave according to CSSOM spec.

Fix a wrong scrollIntoView "nearest" implementation. Beta58+.

	Stefan Hindli [:stefan_hindli]
	Comment 18 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/71956196206b

	Ciprian Georgiu, Desktop QA
	Comment 19 • 7 years ago

I have reproduced this bug using the testcase from comment 1, on an affected Nightly build (2017-11-10).

This is verified fixed on latest Nightly 59.0a1 (2017-12-04) and Beta 58.0b8 (20171130160223) under the following OSes: 
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64