Page MenuHomePhabricator

Regression: Mobile gallery loading spinner appears on left instead of centered
Closed, ResolvedPublic1 Story Points

Description

Steps to reproduce

  1. Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama on the MinervaNeue mobile site.
  2. Tap the lead image.
  3. Advance the image.

Expected results

  • Loading spinner is centered.

Actual results

  • Loading spinner is off centered.

Environments observed

  • Browser version: Chrome v72.0.3626.49
  • OS version: Chrome OS v72.0.3626.49
  • Device model: Pixel Slate
  • Device language: English

Check any additional observations

Developer notes

Introduced by I4d703eef68d51bbe0b03579c5cca0845e17b8c9d

New rule should fix it:

.media-viewer .loading { margin: 8px auto; }

QA steps

QA Results

StatusDetails
✅ PassedTest results

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 9 2019, 8:25 PM
Jdlrobson added subscribers: jan, Jdlrobson.

I believe this is a regression, so @jan would you mind investigating the patch that introduced this problem (using git blame) and add a developer note? I think if this proves to be a regression that should be enough for an estimation.

Jdlrobson triaged this task as High priority.Jan 11 2019, 12:53 AM
Jdlrobson lowered the priority of this task from High to Normal.
nray added a subscriber: nray.Jan 14 2019, 10:28 PM

When I briefly looked into this, I thought it was https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/472368/ that introduced the regression. I couldn't reproduce before that commit

Jdlrobson renamed this task from [Bug] Mobile gallery loading spinner appears on left instead of centered to Regression: Mobile gallery loading spinner appears on left instead of centered.Jan 14 2019, 11:52 PM
Jdlrobson removed Jdrewniak as the assignee of this task.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: Jdrewniak.

Got it!

ovasileva set the point value for this task to 1.Jan 16 2019, 5:10 PM

Change 485734 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Add MediaViewer-specific sekector to loading-spinner CSS

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

Change 485734 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add MediaViewer-specific selector to loading-spinner CSS

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

Jdlrobson updated the task description. (Show Details)

Edward - you should have everything you need in QA steps. Let me know if that's not the case. This is an interesting kind of bug we hit often- UI regressions. We'd love to automate the testing of these. (bookmark T107588 for a rainy day on that subject:-))

Edtadros reassigned this task from Edtadros to Jdlrobson.Jan 29 2019, 4:46 AM
Edtadros added a subscriber: Edtadros.

I tested this following the QA steps (very helpful @Jdlrobson! ). The spinner always appeared in the center horizontally. However, it appears to be off-centered vertically. I have attached a screen video. I put some crude red lines to show more clearly.

Change 487869 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Aligning ImageOverlay prev/next buttons with flexbox.

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

Change 487870 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Align ImageOverlay prev/next buttons with top of viewport.

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

thanks for spotting that @Edtadros!

In the above patches I've aligned the prev/next buttons to the spinner (not the other way around).
@alexhollender I found the prev/next buttons were moved down the page by 3.35em on account of a @headerHeight value here. Since I didn't see a header anywhere, I removed that so they align to top: 0;

This moves the prev/next buttons up a bit, closer to the vertical centre of the page.

beforeafter

The alignment to the close button could use some work, but modifying that is pretty difficult so I left it as is.

Change 487869 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Aligning ImageOverlay prev/next buttons with loading spinner.

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

Change 487870 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Align ImageOverlay prev/next buttons with top of viewport.

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

Jdlrobson reassigned this task from Jdrewniak to Edtadros.Feb 6 2019, 7:48 PM

Over to Edward for a second round of QA (same steps apply) but probably best to wait for another 10 minutes to ensure the change is live (Minerva one has not yet been deployed).

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Safari
Test Artifact:

ovasileva closed this task as Resolved.Feb 7 2019, 10:25 AM

Looks like we're all done here.