Page MenuHomePhabricator

New Vector breaks OO.ui.Element.static.scrollIntoView and sticky layouts
Closed, ResolvedPublic

Description

  1. Go to https://test.wikipedia.org/wiki/Main_Page?useskinversion=2
  2. Run OO.ui.Element.static.scrollIntoView( $('#Bureaucrats')[0] )
  3. Nothing happens
  4. Go to https://test.wikipedia.org/wiki/Main_Page?useskinversion=1 and try the same thing
  5. The "Bureaucrats" heading is scrolled into view, as expected

This happens because <div class="mw-page-container"> has overflow-y: auto. This confuses OO.ui.Element.static.getClosestScrollableContainer into thinking that mw-page-container, rather than the body, is the element it needs to scroll. Disabling this rule in the style inspector fixes the issue.

I'm not sure if the right approach here is to remove the overflow-y: auto rule from mw-page-container (presumably it's there for a reason, but it's probably not intended to make that div a scrollable container), or to make getClosestScrollableContainer smarter and have it look at other signs of scrollability (e.g. el.scrollHeight > el.clientHeight)

See also T255162: OO.ui.Element.static.scrollIntoView is broken for the previous time scrollIntoView was broken.

Update

The overflow-y also causes the issues described in T271868.

QA Results - Beta

ACStatusDetails
1T270146#6752345

QA Results - Prod

ACStatusDetails
1T270146#6773300

Event Timeline

For the product owners wanting to prioritize this, can we update the description with how this impacts products? I'm also like us to add a test for this given the times this has broken, so that would be helpful for working out where this should go.

I'm not sure if the right approach here is to remove the overflow-y: auto rule from mw-page-container

I think that is part of the correct approach. I added that rule to get around the header's top margin collapsing, but I think a better solution would be to remove the overflow-y rule and use top padding for the header instead of using a top margin.

For the product owners wanting to prioritize this, can we update the description with how this impacts products? I'm also like us to add a test for this given the times this has broken, so that would be helpful for working out where this should go.

The impact is that all features involving programmatically scrolling something into view don't work in new Vector. In VisualEditor alone, this includes scrolling down the editor to display the highlighted text when doing find and replace, and scrolling such that the entire inspector is visible when clicking a link or reference near the bottom of the screen. I discovered this issue when working on T261398, where navigating to the next link recommendation may require scrolling down the editor to make it visible.

Jdlrobson added a subscriber: ovasileva.

Thanks @Catrope I assume reading web's assistance here is required per T270146#6693580 (if not please let me know) so tagging with our sprint so @ovasileva prioritizes.

ovasileva triaged this task as Medium priority.Jan 5 2021, 5:24 PM

Copied from Slack:

I think the near-zero padding hack is better for margin collapse, as used in VE: https://github.com/wikimedia/VisualEditor/blob/89187bb6910c6a3e98fa0fda0b6a3cfa9820641f/src/ui/styles/ve.ui.Surface.css#L30

There's an argument that scrollIntoView should keep scrolling parent containers until the element is visible, but that seems lower priority. Eventual parent scrolling is how mouse wheel scrolling works. It wouldn't work so well with animation though.

Change 654471 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/skins/Vector@master] Use padding margin-collapse hack instead of overflow

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

Jdlrobson renamed this task from New Vector breaks OO.ui.Element.static.scrollIntoView to New Vector breaks OO.ui.Element.static.scrollIntoView and sticky layouts.Jan 14 2021, 10:15 PM
Jdlrobson updated the task description. (Show Details)

Change 654471 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use padding margin-collapse hack instead of overflow

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

Change 656489 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@master] Don't override vertical padding on .mw-page-container on tablet+ viewport width

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

Change 656489 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Only override .mw-page-container horizontal padding on tablet+ viewport width

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

Verified on https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page using OO.ui.Element.static.scrollIntoView( $('#footer-info-credits')[0] )

Edtadros added a subscriber: Edtadros.

Test Result - Prod

Status: ✅ PASS
Environment: beta/xyzwiki
OS: macOS Big Sur
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps
  1. Go to https://test.wikipedia.org/wiki/Main_Page?useskinversion=2
  2. Run OO.ui.Element.static.scrollIntoView( $('#Bureaucrats')[0] )
  3. Nothing happens
  4. Go to https://test.wikipedia.org/wiki/Main_Page?useskinversion=1 and try the same thing
  5. The "Bureaucrats" heading is scrolled into view, as expected

✅ AC1:

Screen Recording 2021-01-25 at 5.43.42 AM.mov.gif (1×1 px, 484 KB)

Edtadros updated the task description. (Show Details)

Looks good, resolving.