Page MenuHomePhabricator

Main Menu should work without JavaScript to be more accessible and a better user experience
Closed, ResolvedPublic5 Estimated Story Points

Assigned To
Authored By
pmiazga
Jun 6 2019, 1:38 PM
Referenced Files
F31511999: T225213-3.gif
Jan 14 2020, 9:45 AM
F31511997: T225213-2.gif
Jan 14 2020, 9:45 AM
F31511994: T225213-1.gif
Jan 14 2020, 9:45 AM
F31502467: Screen Recording 2020-01-06 at 10.15.15 AM.mov
Jan 6 2020, 3:21 PM
F30734811: mainmenu.gif
Oct 15 2019, 11:46 PM
F29989183: Screen Shot 2019-08-07 at 11.20.19.png
Aug 7 2019, 9:36 AM
Tokens
"Like" token, awarded by Jdlrobson."Grey Medal" token, awarded by Volker_E."Like" token, awarded by phuedx."100" token, awarded by Niedzielski.

Description

On slow connections it is possible to click the main menu before JS has loaded and be taken to the "Special:MobileMenu" page throwing you out of the mobile experience. We no longer have to do this making sure that clicking the menu always reveals the menu no matter whether the page has fully loaded or not.

The current experience:

mainmenu.gif (394×370 px, 173 KB)

When we developed the Overflow menu @Niedzielski used a pretty nice technique to make the Overflow menu work without Javascript. The existing Main Menu is still using an old way, the Menu hamburger icon links to /wiki/Special:MobileMenu page.

If user doesn't have JS enabled, the browser instead of showing menu (as it does with Overflow menu) will load the Special:MobileMenu page.

Acceptance criteria

  • If JS is disabled and I tap the hamburger icon - the menu appears on the screen
  • the skins.minerva.scripts.menu should be simplified

Developer notes

We can do this using the :target CSS selector and keep the animation.
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/539974 demonstrates how this is possible.
This ships an additional 0.4kb of CSS (after gzip) to end users but means we remove the Special:MobileMenu page and associated experience.

Sign off steps

  • Measure the impact of our main menu changes with help from performance team
  • A card has been made for removing Special:MobileMenu - T241179
  • We have reviewed the accessibility of the new menu
  • Put it on the release timeline.

QA steps

Test on production as logged in user, amc user and logged out user. Click the menu click the random button and check the main menu can be accessed via all pages. Test for at about 10 pages for each scenario.

QA Results

ACStatusDetails
1T225213#5801055
2T225213#5801055
3T225213#5801055

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Adding Accessibility tag as the main menu trigger (currently a link, not a button!) doesn't work flawlessly in assistive technology.

Jdlrobson renamed this task from Main Menu should work without JavaScript to Main Menu should work without JavaScript to be more accessible and a better user experience.Oct 24 2019, 6:11 PM

@Jdlrobson Is this 1.1kb before or after gzip?

It's 0.4kb after gzip (I optimised it). I don't think this is a big problem.

Jdlrobson set the point value for this task to 5.Oct 29 2019, 4:31 PM

The issue with the notifications menu is also happening on master, so I've spun out https://phabricator.wikimedia.org/T237677 to look at that separately. My initial guess is that it might relate to recent changes to the OverlayManager.

TODO: Investigate if shield behaviour can be improved for notification overlay

  • load page
  • click notifications button
  • click shield
  • click back button

expected: get page before
actual: get notification overlay

FYI: I've finally got around to measure the CPU time when clicking on the navigation button on mobile. These seems like a perfect opportunity to measure before and after the switch. I haven't added that to our test servers yet so it happens automatically but I can do that. But the best way to measure it is on a real phone. I got a Alcatel One that is perfect for this. When I get back home I'll collect some metrics + look at measure it continuously.

@Peter that's great. Let me know if you want to chat it through. During offset we also reviewed the accessibility of mobile pages. As a follow up I plan to move the menu below the article.

Patch expectations while I'm out: Feel free to amend it/review it. Please +2 it to signal it is done and ready for merging.

I've intentionally tagged it WIP as once it's ready I'd like to sync with the performance team to measure the change before and after. This can wait till I get back. So once it has a +2 please move to "blocked on others". a +2 while I'm out would be really appreciated as I think this is an important change!

Change 553210 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Do not use data URI on main menu icons

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

I've made some tests so we have some data to compare with. I've accessed https://en.m.wikipedia.org/wiki/Barack_Obama on my Alcatel One (using my office wifi) and then clicking on the navigation to open it, and measure that input delay using the Chrome API First Input Delay. The click always happens after onload.

The runs:

32,368,384,8,16,8,536,24,536,16,8,16,416,8,8,24,392,8,24,384,416.

These where wrong since measured it wrong, I'll do new set of runs, the difference then will be smaller,

Assigned to @Jdrewniak to refine his CSS checkbox patch.

Change 553210 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Do not use data URI on main menu icons

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

Change 556830 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Echo@master] Navigating to notifications on desktop/tablet screens doesn't change address bar

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

Change 559179 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Main menu button works without JS

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

Change 539974 abandoned by Jdlrobson:
Main menu works without JavaScript using :target selector

Reason:
Approach in https://gerrit.wikimedia.org/r/559179 is now being used.

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

Change 559179 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Main menu button works without JS

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

Change 559583 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Simplify menu code

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

Change 559583 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Simplify menu code

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

Change 556830 abandoned by Jdlrobson:
Navigating to notifications on desktop/tablet screens doesn't change address bar

Reason:
Abandoning for now.folded into requirements of https://phabricator.wikimedia.org/T226125

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

Change 560535 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Remove primary-navigation-enabled

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

Change 560535 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove primary-navigation-enabled

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

where and how would we like this to be reviewed?

@alexhollender the change is live on the beta cluster. It'll go live whenever the train rolls around (I think it's this week).
https://drive.google.com/file/d/1GqQ8ZeXyxFoJP1SlfLrj0Py4e7E1Iyhr/view?usp=sharing
I think it looks pretty nice myself :D

I don't know why but the new version seems more smooth on the iPad (and only on the iPad) even though I didn't change the easing function, still ease-in-out.

I did add an opacity transition on the menu as it flies out, as well as a drop-shadow (same one as the drop-down menus).

Jdrewniak assigned this task to alexhollender_WMF.

@Jdrewniak the menu looks great. With javascript turned off I am not seeing any kind of transition (just wanted to confirm that's expected):

With javascript turned off I am not seeing any kind of transition (just wanted to confirm that's expected):

I think this is because of the .animations class which is added via JS for historical reasons. I think we have a bug somewhere to remove (and if we haven't got a bug we should create a bug to do that!)

I think this is because of the .animations class which is added via JS for historical reasons. I think we have a bug somewhere to remove (and if we haven't got a bug we should create a bug to do that!)

There is already a ticket for this at T234570, but removing the .animations class surfaces a nasty chrome bug (see https://phabricator.wikimedia.org/T234570#5772701)

Note this code will be live on mediawiki.org tomorrow.

Adding @N3rsti for context on recent changes to the HTML structure of the main menu that are currently not present on mediawiki.org but will be tomorrow.

@alexhollender we talked about this in standup - we can't easily enable animations for non-js users because of the Chrome bug.
I checked in with performance team and the plan is to review the dashboard thursday/friday post deploy to see how this change impacts things globally. This is blocked until then.

Edtadros subscribed.

Test Result

Status: ✅ PASS
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA steps

Click the menu click the random button and check the main menu can be accessed via all pages. Test for at about 10 pages for each scenario.

✅ AC1: Test on production as logged in user,

T225213-1.gif (480×221 px, 1 MB)

✅ AC2: AMC user

T225213-2.gif (480×221 px, 1 MB)

✅ AC3: and logged out user.

T225213-3.gif (480×221 px, 1 MB)