Page MenuHomePhabricator

Use OO.ui.Element.static.getRootScrollableElement when setting/reading scrollTop in VE
Closed, ResolvedPublic

Description

We use window but OOUI does this better and has handing for browser bugs. Also using window is not compatible with $.animate.

In ce.surface we cache $window. Most uses of that should probably be changed.

Event Timeline

Change 756179 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Consistently use target/surface $scrollContainer to set/get scrollTop

https://gerrit.wikimedia.org/r/756179

Change 756723 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] VE-MW: Consistently use target/surface $scrollContainer to set/get scrollTop

https://gerrit.wikimedia.org/r/756723

Change 756179 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Consistently use target/surface $scrollContainer to set/get scrollTop

https://gerrit.wikimedia.org/r/756179

Change 757112 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (c3f56a0b9)

https://gerrit.wikimedia.org/r/757112

Change 757112 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (c3f56a0b9)

https://gerrit.wikimedia.org/r/757112

Change 756723 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] VE-MW: Consistently use target/surface $scrollContainer to set/get scrollTop

https://gerrit.wikimedia.org/r/756723

matmarex moved this task from Incoming to QA on the Editing-team (FY2021-22 Kanban Board) board.
matmarex added a subscriber: matmarex.

I tested a bunch locally, but we're making a lot of changes here and we might have broken something and not noticed.

Things to test: everything where we scroll the browser viewport, or react to the user scrolling, particularly:

  • copy-pasting shouldn't scroll unexpectedly
  • toolbar sticking to the top of the viewport when you scroll
  • opening the keyboard and toolbar dropdowns shouldn't scroll unexpectedly (on mobile)
  • find-and-replace dialog should scroll to the matches
  • context menu (when you e.g. focus a large image) should stay visible as you scroll

I checked the things listed in @matmarex 's comment and more. Ran regression for core features while focusing on the user scrolling.

All seems good.

ppelberg claimed this task.