Page MenuHomePhabricator

Mobile header shows both bell icon and red notification circle in Internet Explorer 9-11
Closed, ResolvedPublic3 Story Points

Description

Windows 7(64) - IE9
Windows 8 - IE10
Windows 8.1 - IE11
Windows 10 - Edge

Expected: Bell should be hidden and circle should be centered as it is in Edge on Windows 10.
DO NOT SPEND MORE THAN 2HRS TRYING TO FIX THIS
DO NOT RE-ARCHITECT MOBILEFRONTEND JUST TO FIX THIS BUG

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 10 2017, 11:35 PM
Jdlrobson updated the task description. (Show Details)

This was discovered during QA of T160471.
Might want to address this now. Let's talk about it in standup on Tuesday.

ovasileva triaged this task as Normal priority.Apr 11 2017, 10:41 AM
ovasileva moved this task from To Triage to 2016-17 Q3 on the Readers-Web-Backlog board.

Does this task need story points? :)

Jdlrobson lowered the priority of this task from Normal to Low.Apr 11 2017, 5:26 PM
Jdlrobson added a subscriber: ovasileva.

Does this task need story points? :)

It does, but we need to estimate it. @ovasileva has said this should be done in the current sprint but with low priority. I haven't had time since the bug was raised to think about how many ponts this would entail.

@Jdlrobson Thanks for clarifying my question. When I ask if a task "needs story points" I am assuming that that is interpreted as "needs the team to estimate as a group" but can see how it could be instead interpreted as "add however many points you or someone else feels" :)

Jdlrobson updated the task description. (Show Details)Apr 12 2017, 5:17 PM

Looking into this, the problem is the combination of position absolute and the display table.

I'm wondering why we use position absolute at all here. If we drop from .notification-count the following:

/* position: absolute; */
/* top: 0; */
/* right: 0; */
/* bottom: 0; */
/* left: 0; */

and hide #secondary-button
we get the same result.

Is there a reason we use position absolute?

@Jdlrobson here is the background info: T158957.

Change 348518 had a related patch set uploaded (by Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Notification icon tweak for better IE support

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

I looked at the discussion and my proposal doesn't seem controversial. I'm keeping use of flex, just dropping absolute positioning.

Jdlrobson moved this task from To Do to Needs Code Review on the Reading-Web-Sprint-95 board.
phuedx added a subscriber: phuedx.

@Jdlrobson to fix a very minor issue with the secondaryButton.mustache template.

@phuedx that's now fixed. I'm actually working on a follow up with your feedback in mind as this is a good opportunity to clean up some code here.

I've proposed a refactor: https://gerrit.wikimedia.org/r/348795 Hygiene: Refactor Notification code - add NotificationBadge class
but it should not block addressing this problem.

MBinder_WMF set the point value for this task to 3.Apr 19 2017, 5:20 PM
phuedx added a comment.EditedApr 20 2017, 9:05 AM

Here's my test plan:

  1. Create a new user account
  2. Observe that there's a new notification, i.e. the notification counter (RHS of the header) is a solid red circle
  3. Open the notification drawer by tapping the notification counter
  4. Observe that the notification counter turns to a hollow grey circle
  5. Read the notification (tap the blue dot)
  6. Observe that the notification counter turns to a bell icon
  7. Refresh the page
  8. Observe that the notification counter is still a bell icon
phuedx added a comment.EditedApr 20 2017, 9:40 AM

Results:

Note well that I'm only providing screenshots of the notification counter in the "new notification" state – step #2 from T162647#3196881 – as that's what triggered this bug report.

macOS Sierra (10.12.3) – Chrome (57.0.2987.133)
macOS Sierra (10.12.3) – Safari (10.0.3)
macOS Sierra (10.12.3) – Firefox (53.0)
Windows 7 (x64) – IE9
Windows 8 – IE10
Windows 8.1 – IE11
Windows 10 – Edge

👍


All browsers passed the test plan, I'd note that:

  • I had to refresh the page between steps #5 and #6 from T162647#3196881. This behavior is noted in the commit message of rEMFR87e9916f7f08: Notification icon tweak for better IE support; and
  • I observed that in IE9 only the spinner that should replace the notification counter when you click it sometimes didn't replace it but instead appeared on a new line, immediately below the hamburger icon:

This wasn't always reproducible.

This only happens when there are no notifications. Moreover, this currently happens on MobileFrontend@HEAD.

Change 348518 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Notification icon tweak for better IE support

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

phuedx reassigned this task from Jdlrobson to ABorbaWMF.Apr 20 2017, 11:41 AM
phuedx moved this task from Needs Code Review to Needs QA on the Reading-Web-Sprint-96 board.

Over to you for general QA @ABorbaWMF!

Looks fixed!
Tested against https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page
Systems:
Windows 7(64) - IE9
Windows 8 - IE10
Windows 8 - Opera 44
Windows 8.1 - IE11
Windows 10 - IE11
Windows 10 - Edge 14
Mac OSX 10.12 - Safari 10
Mac OSX 10.11 - Firefox 52
Mac OSX 10.7 - Chrome 48

Jdlrobson closed this task as Resolved.Apr 20 2017, 6:57 PM

this is a bug and we've shown it to be fix. Hurrah!