Page MenuHomePhabricator

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

Description

Header-Win7(64) - IE9.png (768×1 px, 128 KB)
Windows 7(64) - IE9
Header-Win8 - IE10.png (768×1 px, 129 KB)
Windows 8 - IE10
Header-Win 8.1 - IE11.png (768×1 px, 134 KB)
Windows 8.1 - IE11
Header-Win 10 - Edge.png (768×1 px, 173 KB)
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

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 Medium priority.Apr 11 2017, 10:41 AM
ovasileva moved this task from Incoming to 2016-17 Q3 on the Web-Team-Backlog board.
Jdlrobson lowered the priority of this task from Medium 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" :)

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?

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.

phuedx subscribed.

@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

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

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.

Screen Shot 2017-04-20 at 10.16.52.png (87×103 px, 6 KB)
macOS Sierra (10.12.3) – Chrome (57.0.2987.133)
Screen Shot 2017-04-20 at 10.32.22.png (66×72 px, 1 KB)
macOS Sierra (10.12.3) – Safari (10.0.3)
Screen Shot 2017-04-20 at 10.33.41.png (71×82 px, 1 KB)
macOS Sierra (10.12.3) – Firefox (53.0)
Screen Shot 2017-04-20 at 10.21.04.png (89×98 px, 4 KB)
Windows 7 (x64) – IE9
Screen Shot 2017-04-20 at 10.16.52.png (87×103 px, 6 KB)
Windows 8 – IE10
Screen Shot 2017-04-20 at 10.26.04.png (75×76 px, 3 KB)
Windows 8.1 – IE11
Screen Shot 2017-04-20 at 10.31.06.png (77×94 px, 4 KB)
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:

Screen Shot 2017-04-20 at 10.20.01.png (91×187 px, 8 KB)

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 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

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