Page MenuHomePhabricator

[Bug] Horizontal scrollbar shown when browser window is between 1000px and 1107px
Closed, ResolvedPublic

Description

Steps to reproduce

  1. (if you are on a Mac, you need to go to System Preferences > General > and change "Show Scrollbars" to "always")
  2. Visit https://en.m.wikipedia.org/wiki/Barack_Obama
  3. Open developer tools to easily eye the screen dimensions
  4. Change the browser window size to 999px
  5. Increase the width to 1000px
  6. Increase the width to 1107px
  7. Increase the width to 1108px

Expected results

  • No horizontal scrollbar is necessary for a large window

Actual results

  • The horizontal scrollbar pops in and out as different CSS breakpoints are hit

Environments observed

  • enwiki

Browser Version:

  • Chromium v65.0.3325.181

OS Version:

  • Ubuntu v17.10 64b

Device Model:

  • Desktop

Device Language:

  • English

Notes

When I scroll a little ways past #Cultural_and_political_image, the horizontal scrollbar disappears. Maybe this is an issue with lazy loading placeholders?

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 10 2018, 1:26 PM
Niedzielski updated the task description. (Show Details)Apr 10 2018, 1:29 PM

The only thresholds we officially support are 320px (mobile), 720px (tablet) and 1200px (desktop). Anything in between can and is likely to break. Looking into this i think the problem is our fixed max width and margins are not compatible at this resolution.

@alexhollender which thresholds should we be caring about more? Should we adjust our max width and margins (at tablet/desktop) to counter for this?

To clarify: this issue only appears when resizing from browser size ≤999px to ≥1000px, correct? I'm not sure I understand your question of which thresholds we should be caring about more?

@alexhollender right now we have 3 notable breakpoints where we adapt the layout. These are mobile (320px), tablet (720px) and desktop 1000px.

Thus as you approach these breakpoints the design starts to suffer.

https://vimeo.com/235428198 is a great talk that I saw yesterday that gives an idea of how you might solve this but hints at the added complexity cost of trying to solve this problem.

We should be cautious and mindful about supporting a completely fluid layout but it is possible...

We notice the screen pops back into place when content near "Post-presidency (2017–present)" loads so probably a local wiki issue.

TheDJ added a subscriber: TheDJ.Apr 23 2018, 8:33 PM

This is caused by .content .figure, .content .thumb {width: 320px} , while the [[ Job_Growth_by_U.S._President_-_v1.png | included image ]] is 370px wide, causing it to overflow on the right side. You can easily spot this when you disable javascript.

TheDJ added a comment.EditedApr 23 2018, 8:37 PM

Additionally, when you scroll down with JS enabled, the horizontal scrollbars disappear, because the image is lazy loaded and suddenly

.content a > img {
    max-width: 100% !important;
    height: auto !important;
}

becomes active. guess we need a .content a > noscript > img
and

.lazy-image-loader {
    max-width: 100%;
}

Change 428511 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Size restrict images inside noscript tags

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

Change 428511 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Size restrict images inside noscript tags

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

Restricted Application added a project: Readers-Web-Backlog. · View Herald TranscriptApr 23 2018, 9:03 PM

@alexhollender I think this can skip QA but I wanted to give you the opportunity to verify this is fixed: http://reading-web-staging.wmflabs.org/w/index.php?title=Barack_Obama&mobileaction=toggle_view_mobile and that we haven't broken anything relating to image styling. Be sure to test with JS disabled and enabled.

Sorry, there's some confusion. I left a comment on the patch when I merged it that it does indeed fix the no-JS case but does not fix the lazily loaded case originally reported.

Niedzielski removed alexhollender as the assignee of this task.Apr 24 2018, 1:15 AM
TheDJ added a comment.EditedApr 24 2018, 11:55 AM

I also note that this forced 320px, breaks anything that is larger than that, even if it would have the noresize class set... ( See also: T181756 and T185562 )

@Jdlrobson assuming I should wait on checking this out based on the comments that came in after yours

@alexhollender you can take a look at this now - it's on staging.

Change 429848 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Regression: Avoid reflows on lazy image placeholders

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

Jdlrobson reopened this task as Open.Apr 30 2018, 5:54 PM

We've caused reflows in this change which is very bad for performance so reopening.

Change 429848 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Regression: Avoid reflows on lazy image placeholders

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

ovasileva triaged this task as High priority.Apr 30 2018, 7:33 PM
alexhollender added a comment.EditedMay 1 2018, 1:47 PM

@Jdlrobson just checked this out with and without js. Not seeing anything weird. What specifically should I be looking for re:

We've caused reflows in this change which is very bad for performance so reopening.

@alexhollender, if you set your browser to emulate a slow connection or offline and load a page, all the grey placeholders had short heights. When the placeholders were lazily replaced with images, they are given proportional heights which caused the page to reflow for nearly each image loaded. With the new patch Jon made, there should _generally_ be no difference between grey placeholder height and loaded image height.

@Niedzielski - should we test on a slower connection then or are we good to go?

@ovasileva I tested this fix before merging and it looked good to me!

ovasileva closed this task as Resolved.May 2 2018, 2:39 PM

sounds good then. resolving!