1516722 - scrollIntoView() should scroll the element into the visual viewport

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Botond Ballo [:botond]]

Currently, Element.scrollIntoView() makes an attempt to scroll the element into the visual viewport, but it's limited to modifying the layout scroll position.

This means that if e.g. you're zoomed in, and the element is near the bottom-right corner of the scrollable rect, the element will not actually be scrolled into the visual viewport.

To properly ensure the element is scrolled into the visual viewport, scrollIntoView() needs to be able to modify the visual scroll position as well. It will need to use the mechanism tracked in bug 1507279 to do so.

Comment 1

[Botond Ballo [:botond]]

Attachment: Bug 1516722 - Also scroll the visual viewport in ScrollToShowRect() if necessary. r=kats

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1516722 - Mochitest. r=kats

Depends on D29079

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55c8e6f3e522
Also scroll the visual viewport in ScrollToShowRect() if necessary. r=kats
https://hg.mozilla.org/integration/autoland/rev/8f2db95f0610
Mochitest. r=kats

Comment 4

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/55c8e6f3e522
https://hg.mozilla.org/mozilla-central/rev/8f2db95f0610

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/e8766f96041a
Backed out 2 changesets for wpt failures on scroll-restoration-fragment-scrolling-samedoc.html. a=backout

Comment 6

[Natalia Csoregi [:nataliaCs]]

Backed out for wpt failures on scroll-restoration-fragment-scrolling-samedoc.html.

backout: https://hg.mozilla.org/mozilla-central/rev/e8766f96041a7f5a56aecf2a9dc94787a9e297ce

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8f2db95f0610ed8c51419bb94ed97ab25a2e3c8c&searchStr=wpt

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243551553&repo=autoland&lineNumber=63108

08:46:38 INFO - PID 3144 | => mAdoptCount: 556
08:46:38 INFO - PID 3144 | => mAdoptFreeCount: 572
08:46:38 INFO - PID 3144 | => Process ID: 3151, Thread ID: 140735247889152
08:46:38 INFO - PID 3144 | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /var/folders/rw/g2g0vvbj6sz13g4_cgzjs8fc00000x/T/tmpsk7aVA.mozrunner/runtests_leaks_1831_tab_pid3153.log
08:46:38 INFO - PID 3144 | ++DOMWINDOW == 3 (0x1214a7000) [pid = 3152] [serial = 3] [outer = 0x121435020]
08:46:38 INFO - PID 3144 | ++DOMWINDOW == 4 (0x1214ad800) [pid = 3152] [serial = 4] [outer = 0x121435020]
08:46:39 INFO - PID 3144 | --DOMWINDOW == 3 (0x11f68b000) [pid = 3147] [serial = 2] [outer = 0x0] [url = about:blank]
08:46:39 INFO -
08:46:39 INFO - TEST-UNEXPECTED-FAIL | /html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html | Manual scroll restoration should take precedent over scrolling to fragment in cross doc navigation - assert_equals: should not scroll to fragment expected 0 but got 800
08:46:39 INFO - @http://web-platform.test:8000/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html:47:9
08:46:39 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1587:25
08:46:39 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1611:35
08:46:39 INFO - EventListener.handleEvent*@http://web-platform.test:8000/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html:42:14
08:46:39 INFO - setTimeout handler*@http://web-platform.test:8000/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html:40:5
08:46:39 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1587:25
08:46:39 INFO - async_test@http://web-platform.test:8000/resources/testharness.js:576:22
08:46:39 INFO - @http://web-platform.test:8000/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html:28:3
08:46:39 INFO - TEST-OK | /html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html | took 1817ms

Comment 7

[Botond Ballo [:botond]]

The issue in scroll-restoration-fragment-scrolling-samedoc.html is that ScrollToVisual() is clobbering a later main-thread scroll in the same transaction (a problem which Kats foresaw back when we added ScrollToVisual).

Comment 8

[Botond Ballo [:botond]]

WIP fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba9f0a0503ca07e68341f3808e7632936129cc0f

Comment 9

[Intermittent Failures Robot]

3 failures in 4131 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1
• mozilla-inbound: 2

Platform breakdown:

• osx-10-10: 1
• osx-10-10-shippable: 1
• windows7-32-shippable: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1516722&startday=2019-04-29&endday=2019-05-05&tree=all

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)

WIP fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba9f0a0503ca07e68341f3808e7632936129cc0f

I verified with some retriggers that this patch fixes the intermittent failure of scroll-to-the-fragment-in-shadow-tree.html (bug 1547808) as well.

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1516722 - Don't let ScrollToVisual clobber further layout scrolling in the same transaction. r=kats

Test coverage for this is provided in the web platform test
html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html.

Depends on D29093

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1516722 - Update usage notes for nsIPresShell::ScrollToVisual(). r=kats

Depends on D30105

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6348ca2145d9
Also scroll the visual viewport in ScrollToShowRect() if necessary. r=kats
https://hg.mozilla.org/integration/autoland/rev/5e9833abf181
Mochitest. r=kats
https://hg.mozilla.org/integration/autoland/rev/9e64b394951d
Don't let ScrollToVisual clobber further layout scrolling in the same transaction. r=kats
https://hg.mozilla.org/integration/autoland/rev/f1d9dca026c3
Update usage notes for nsIPresShell::ScrollToVisual(). r=kats

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6348ca2145d9
https://hg.mozilla.org/mozilla-central/rev/5e9833abf181
https://hg.mozilla.org/mozilla-central/rev/9e64b394951d
https://hg.mozilla.org/mozilla-central/rev/f1d9dca026c3