Page MenuHomePhabricator

OO.ui.Element.static.scrollIntoView is broken
Closed, ResolvedPublic

Description

It does nothing, e.g. OO.ui.Element.static.scrollIntoView($('#footer')[0])

I tested against the demos which blames https://gerrit.wikimedia.org/r/#/c/oojs/ui/+/525634/, which is a commit only for the demos.

Edit: Bisecting Vector found a similar commit: https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/551672/

Event Timeline

Esanders created this task.Jun 11 2020, 4:07 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 11 2020, 4:07 PM
Esanders updated the task description. (Show Details)Jun 11 2020, 4:10 PM
Esanders triaged this task as Unbreak Now! priority.Jun 11 2020, 4:30 PM
Esanders updated the task description. (Show Details)
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJun 11 2020, 4:30 PM

Change 604786 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/skins/Vector@master] Revert "Set overflow-y to scroll to prevent reflow"

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

Change 604787 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/skins/MinervaNeue@master] Revert "Set overflow-y to scroll to avoid reflow"

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

The correct fix will be to patch scrollIntoView, but for now we should revert the offending patches. That method being broken seems more critical than T204084.

Change 604793 had a related patch set uploaded (by Esanders; owner: Esanders):
[oojs/ui@master] Revert "demos: Make forced scrolling rule compatible with disabling scroll for dialogs"

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

Jdlrobson added a subscriber: Jdlrobson.EditedJun 11 2020, 4:45 PM

Can you expand the description with some reproduction steps. I have no idea what this is about right now (and why it should be considered UBN).

Volker_E added a comment.EditedJun 11 2020, 4:55 PM

Ahem, we would need to expect as lib to be in an environment where overflow-y: scroll is set…?!
Or did I misread your comment above @Esanders and you mean patching scrollIntoView to take overflow-y into account?

Esanders added a comment.EditedJun 11 2020, 5:05 PM

Or did I misread your comment above @Esanders and you mean patching scrollIntoView to take overflow-y into account?

That's correct. Edit: To be clear I would happily abandon these reverts if there was a fix for scrollIntoView, but given that is of unknown complexity right now we should revert the regression while we figure it out.

Can you expand the description with some reproduction steps.

Sure, a quick command line test is OO.ui.Element.static.scrollIntoView($('#footer')[0])

Esanders updated the task description. (Show Details)Jun 11 2020, 5:05 PM

Anywhere you see scrollIntoView in our code would be broken. We use to it keep templates and inspectors in the viewport, amongst other things.

Change 604936 had a related patch set uploaded (by Catrope; owner: Catrope):
[oojs/ui@master] Element: Deal with scrollable <body> in scrollIntoView()

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

Change 605190 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/core@master] Add temporary workaround for scrollIntoView bug when OOUI is loaded

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

Change 604786 abandoned by Esanders:
Revert "Set overflow-y to scroll to prevent reflow"

Reason:
See I34c05c512d731eb5977c51f503f7f29ebcdfbdbf

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

Change 604787 abandoned by Esanders:
Revert "Set overflow-y to scroll to avoid reflow"

Reason:
See I34c05c512d731eb5977c51f503f7f29ebcdfbdbf

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

Per @Jdlrobson 's suggestion, https://gerrit.wikimedia.org/r/605190 only applies the revert when OOUI is loaded, so it doesn't affect most readers on Minerva, which I think was the initial case for the fix.

I now also see Roan's upstream fix, so hopefully the hack won't be necessary.

Change 605207 had a related patch set uploaded (by Esanders; owner: Esanders):
[oojs/ui@master] Element: Fix getClosestScrollableContainer when body has overflow

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

Change 604936 abandoned by Bartosz Dziewoński:
Element: Deal with scrollable <body> in scrollIntoView()

Reason:
Merging Ed's patch instead.

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

Change 605207 merged by jenkins-bot:
[oojs/ui@master] Element: Fix getClosestScrollableContainer when body has overflow

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

Change 604793 abandoned by Bartosz Dziewoński:
Revert "demos: Make forced scrolling rule compatible with disabling scroll for dialogs"

Reason:
No longer needed.

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

That should fix the underlying issue.

Do we still want to merge the various workarounds, or just wait for the next OOUI release?

I'd prefer an OOUI release tomorrow.

Change 605190 abandoned by Esanders:
Add temporary workaround for scrollIntoView bug when OOUI is loaded

Reason:
Ib6bda230f3ab4f0a12454cc7eafc220ce5f7f700 has been merged into OOUI

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

Jdforrester-WMF closed this task as Resolved.Wed, Jun 17, 3:01 PM
Jdforrester-WMF assigned this task to Esanders.
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

This is now fixed in the master of OOUI, right? It's just waiting for a release?

@Jdforrester-WMF Yes, OOUI v0.39.2 – upcoming.

Volker_E moved this task from Backlog to OOUI-0.39.2 on the OOUI board.Wed, Jun 24, 12:58 AM
Volker_E edited projects, added OOUI (OOUI-0.39.2); removed OOUI.

Change 607443 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@master] Update OOUI to v0.39.2

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

Change 607443 merged by jenkins-bot:
[mediawiki/core@master] Update OOUI to v0.39.2

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