Closed Bug 1416391 Opened 2 years ago Closed 2 years ago

scrollIntoView "nearest" is wrong when the element is bigger than the scrolling box

Categories

(Core :: DOM: CSS Object Model, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- verified
firefox59 --- verified

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(2 files)

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
Attached file testcase.htm
Blocks: 1416394
Hopefully some web platform test will unexpectedly pass and that will be enough.
Otherwise what should I do, use my testcase as a reftest?
I fixed test_scroll_selection_into_view.html, is this enough as a test?
Also modified various comments.
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.
Attachment #8927517 - Flags: review?(annevk) → review?(wisniewskit)
Priority: -- → P3
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?
Flags: needinfo?(annevk)
Attachment #8927517 - Flags: review?(wisniewskit)
Attachment #8927517 - Flags: review?(bkelly)
Attachment #8927517 - Flags: feedback+
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.
Attachment #8927517 - Flags: review?(bkelly) → review?(tnikkel)
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.
Flags: needinfo?(annevk)
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.
Attachment #8927517 - Flags: review?(tnikkel) → review+
The "nearest" was added in firefox58. Should the patch be uplifted, to avoid landing a wrong implementation, or is it too risky?
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
Keywords: checkin-needed
(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).
(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.
https://hg.mozilla.org/mozilla-central/rev/184697371b6b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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.
Attachment #8927517 - Flags: approval-mozilla-beta?
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+.
Attachment #8927517 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.