Closed Bug 1516722 Opened 1 year ago Closed 1 year ago

scrollIntoView() should scroll the element into the visual viewport

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Keywords: correctness)

Attachments

(4 files)

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.
Keywords: correctness
OS: Unspecified → Android
Hardware: Unspecified → All
Assignee: nobody → botond
Attachment #9061127 - Attachment description: Bug 1516722 - Also scroll the visual viewport in ScrollToShowRect() if necessary. → Bug 1516722 - Also scroll the visual viewport in ScrollToShowRect() if necessary. r=kats
Pushed by bballo@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Backout by ncsoregi@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/e8766f96041a
Backed out 2 changesets for wpt failures on scroll-restoration-fragment-scrolling-samedoc.html. a=backout

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

Status: RESOLVED → REOPENED
Flags: needinfo?(botond)
Resolution: FIXED → ---
Target Milestone: mozilla68 → ---
Regressions: 1547808

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).

Flags: needinfo?(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.

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

Pushed by bballo@mozilla.com:
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
Duplicate of this bug: 1545096
You need to log in before you can comment on or make changes to this bug.