Page MenuHomePhabricator

Notification count appears misaligned in all mobile web
Closed, ResolvedPublic8 Estimated Story Points

Description

Due to new header changes, notification count appears misaligned on ALL wikis.

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptFeb 24 2017, 12:36 PM

This is what I see:

Can you provide more info about the browser? We should probably test it in different browsers and identify the ones that are affected. Needs reproduction?

Android 6.0.1
Chrome 56.0.2924.87


Firefox seems to strangely not have the new header, same for Opera Mini.

@bmansurov - try it on your phone, it looks aligned for me on desktop

bmansurov moved this task from Needs Analysis to Doing on the Reading-Web-Sprint-92-๐Ÿœ board.

Change 339629 had a related patch set uploaded (by Bmansurov):
Fix notification count vertical alignment

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

@bmansurov
Chrome: new header + notifications appearing


Firefox: new header not appearing

ovasileva added a comment.EditedFeb 24 2017, 2:03 PM

Opera mini beta mode - non-beta header appears
expected behavior: beta mode header should appear


Opera mini regular - new header not present
expected behavior: new header should appear (without branding)

@Nirzar - it seems I was logged in to firefox as two different users. For the new header, is the empty bell icon expected behavior for user with no notifications?

Jhernandez added a subscriber: Jhernandez.

Attached image for review on patch.

MBinder_WMF set the point value for this task to 8.Feb 27 2017, 6:26 PM

Change 340308 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
Fix vertical alignment of notifications label

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

I'm sorry to be a pain but I've also spent time and the best solution I could find that wouldn't change the HTML is center using flexbox. I've submitted https://gerrit.wikimedia.org/r/340308 as an alternative to Baha's patch.

Browserscreenshot
Chrome mobile
Chrome
Firefox 51
Safari 9
Safari 8

QA on Baha's take https://gerrit.wikimedia.org/r/#/c/339629

Summary: Old iOS <= 8 aligns properly, Chrome mobile misaligns a bit to the top

BrowserScreenshot
chrome mobile
chrome 56
firefox 51
iOS8 safari 8
safari 9

T158957#3061586 and T158957#3060917 contain local QA on several browsers and the different tradeoffs on the solutions. I don't mind which one to choose right now, but we should do something.

@Nirzar, you can test @Jhernandez 's solution at reading-web-staging.wmflabs.org

Looks pretty good on Staging. Here are the screenshots:

ScreenshotsAndroid Device OS - Browser VersioniOS Device OS - Browser Version
Nexus 6P Android 7 - Firefox 50iPhone 6 iOS 8.1 - Safari 8.0
Galaxy Note 3 Android 4.4 - Firefox 50iPhone 6S+ iOS 9.0 - Chrome 47
Nexus 9 Tablet Android 6.0 - Chrome 55iPad Air 2 iOS 9.0 - Safari 9.0
Galaxy S6 Android 5.0 - Android Browser 5.0iPhone 5s iOS 7.0 - Safari 7.0
Nexus 6 Android 5 - Maxthon 4.5iPhone 6s iOS 9.0 - Chrome 47

@ABorbaWMF, @Nirzar - the iPhone 5s iOS 7.0 - Safari 7.0 combination still seems a bit misaligned to me.

Summary of tradeoffs:

  • Table-cell approach (Baha): All well aligned but Chrome mobile (aligned but a bit misplaced to the top)
  • Flexbox approach: All well aligned but iOS <= 8 (aligned but a bit misplaced to the bottom)

The patch deployed is the Flexbox approach, which is why you see old iOS a bit misaligned.

@ABorbaWMF, @Nirzar - the iPhone 5s iOS 7.0 - Safari 7.0 combination still seems a bit misaligned to me.

True, it's 1-2px down.

@Jhernandez - not perfect, but I think it's okay. @Nirzar?

okay from my side

ovasileva closed this task as Resolved.Mar 1 2017, 4:29 PM
bmansurov reopened this task as Open.Mar 1 2017, 4:56 PM

The patch hasn't been merged yet.

Change 339629 abandoned by Bmansurov:
Fix notification count vertical alignment

Reason:
See an alternative approach: I3aa1beb9b12d99f4b08dd7f98c4455156a0dc458.

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

Change 340308 merged by jenkins-bot:
Fix vertical alignment of notifications label

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

phuedx added a subscriber: phuedx.Mar 2 2017, 6:48 AM

rEMFR26343111b51e: Fix vertical alignment of notifications label was merged by @bmansurov (after it was deployed to the staging server and signed off by @Nirzar?).

phuedx assigned this task to Jhernandez.Mar 2 2017, 6:48 AM

Over to you @Jhernandez since you're the author of the fix!

Summary of tradeoffs:

  • Table-cell approach (Baha): All well aligned but Chrome mobile (aligned but a bit misplaced to the top)
  • Flexbox approach: All well aligned but iOS <= 8 (aligned but a bit misplaced to the bottom)

The patch deployed is the Flexbox approach, which is why you see old iOS a bit misaligned.

For the record, the Flexbox approach was chosen.

ovasileva closed this task as Resolved.Mar 2 2017, 11:36 AM

Based on the results from staging, signing this off.