Page MenuHomePhabricator

[SPIKE 8hr] Main Menu should work without javascript
Open, NormalPublic

Description

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
  • Special:MobileMenu special class is removed from code
  • the skins.minerva.scripts.menu should be simplified

Event Timeline

pmiazga created this task.Jun 6 2019, 1:38 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 6 2019, 1:38 PM
pmiazga triaged this task as Normal priority.Jun 6 2019, 3:01 PM
pmiazga moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
ovasileva renamed this task from [SPIKE] Main Menu should work without javascript to [SPIKE 8hr] Main Menu should work without javascript.Jun 18 2019, 4:28 PM
ovasileva moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.

The issue we had with the CSS checkbox hack was that the closing animation doesn't work and had to be removed (see discussion on overflow patch)

https://codepen.io/erikterwan/pen/EVzeRP

phuedx added a subscriber: phuedx.EditedJun 18 2019, 4:45 PM

As I mentioned in today's grooming, there's a whole bunch of (now overly-complicated) machinery that we created in order to defer the parsing/DOM construction for the main menu as it's the very top element in #mw-mf-viewport. I think that reevaluating this machinery is well worth it!

If there's a chance that we could move the container element for the hamburger menu to below the main content, then that'd be neat.

Could we also consider moving the Wikimedia-specific instrumentation for the main menu into the WikimediaEvents extension?

Could we also consider moving the Wikimedia-specific instrumentation for the main menu into the WikimediaEvents extension?

that would be a nice touch!

Jdlrobson lowered the priority of this task from Normal to Low.Jul 31 2019, 6:48 PM

Change 528264 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] [UI] [menu] slide the main menu over the page

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

What is the problem statement here ie. can qe articulate what is wrong with the Special:MobileMenu which reduces HTML/CSS bloat by deferring the loading of the menu? Have we considered the trade offs?

I think 1) the usability win and 2) reduced tech debt are more than justification enough:

  1. Clicking before the menu before JavaScript loads confusingly goes to a giant menu-only page. It looks awful and for non-JS users requires an extra page load.
  2. Maintaining an extra special page is non-trivial as its an entirely different configuration to consider for the LESS, ResourceLoader module structure, and just extra code.

The only tradeoff I can think of is if someone links to Special:MobileMenu this change would break it. Otherwise, this is a terrible page and should be burninated.

Clicking before the menu before JavaScript loads confusingly goes to a giant menu-only page.

The JavaScript load is a separate issue that never got addressed and is orthogonal to this. It would be trivial to disable the click handler for JS users... This page is meant for users such as those using Opera Mini.

for non-JS users requires an extra page load.

For non-JS users this however this considerably reduces the cost of every other page as we don't need to download all the icons, nor the main menu CSS, not the HTML for including them in the payload. Remember menu usage is very low (https://grafana.wikimedia.org/d/000000566/overview?panelId=22&fullscreen&orgId=1 logs events for 50% of all page views). Most people don't interact with the main menu. They want to read the article.

Maintaining an extra special page is non-trivial as its an entirely different configuration to consider for the LESS, ResourceLoader module structure, and just extra code.

I don't think we've ever had any issues with this special page. It's not exactly complicated or a maintenance burden.

  1. the usability win

It seems to me like the main win here is we're trading usability for performance.

How does the new CSS and experience fair on legacy browsers e.g. Opera Mini and Android 2? We should remember this page is mostly for those users.

Or is the idea that we'd minimise visits to Special:MobileMenu for JS users? If you have a good connection and a good browser, there's absolutely no reason to hit Special:MobileMenu and I can't argue with that.

The JavaScript load is a separate issue that never got addressed and is orthogonal to this. It would be trivial to disable the click handler for JS users... This page is meant for users such as those using Opera Mini.

Yes, I see this page on slow connections when I tap the menu so that's part of this issue too.

It's not exactly complicated or a maintenance burden.

Yes but I just broke it trying to make changes to the menu so all these extra specialized configurations are costing us. In this case, we will providing a single experience for both JS and non-JS users as a replacement to Special:MobileMenu \o/

Or is the idea that we'd minimise visits to Special:MobileMenu for JS users? If you have a good connection and a good browser, there's absolutely no reason to hit Special:MobileMenu and I can't argue with that.

Yes, let's delete Special:MobileMenu as part of this task.

Jdlrobson added a comment.EditedAug 5 2019, 10:26 PM

Repeating as I consider this the most important thing I said and I didn't see a reply:

It seems to me like the main win here is we're trading usability for performance.
How does the new CSS and experience fair on legacy browsers e.g. Opera Mini and Android 2? We should remember this page is mostly for those users.

The point I'm trying to make on the ticket is that the page is mainly aimed at legacy browsers such as Opera Mini/Android 2. Any change here needs to consider the performance and CSS support of those devices and needs special QA attention on those browsers.

I think you can improve the experience of modern browsers without removing the Special:MobileMenu page by using what you have and something like this snippet executing at the top of the page.

var menuBtn = document.getElementById( 'mw-mf-main-menu-button' );
var menuLink = menuBtn.getAttribute('href');
menuBtn.setAttribute( 'href', '#' );
(window.NORLQ||[]).push( function ( ) {
  menuBtn.setAttribute( 'href', menuLink );
} );

@Jdlrobson, after re-reading my comment, I realized I could have worded it much better. I apologize as I didn't intend my response to come off as flippant. I've added some more detail below and we can dig into this in our 1:1 tomorrow as needed.


The point I'm trying to make on the ticket is that the page is mainly aimed at legacy browsers such as Opera Mini/Android 2. Any change here needs to consider the performance and CSS support of those devices and needs special QA attention on those browsers.

Honestly, I'm uncertain and haven't looked into it. The reason is that according to the compatibility matrix, Opera Mini is grade C (emphasis mine):

In the front-end this means content is presented in a readable manner, and to some extent user actions can be performed...

And Android 2 falls into grade B. Grade B is nebulous and doesn't have a definition but one assumes it's better than grade C and less than grade A. However, I would guess that supporting Android 2 will soon be grade C or lower as the matrix was last evaluated in March 2017 and on 2018-08-26 it was at 0.2% share and as of 2019-07-21 it's 0.0%. I've pinged wikitech-l to get community feedback on updating the matrix.

In truth, I was a little taken aback by the interest. I don't know how well the current solution works on Android 2 and Opera Mini or if Edward has ever tested it. I kind of hope not but given your concerns, I realized I'm unclear how we should prioritize continuing to support these configurations. If my perceptions are wrong, I'd like to know as it'll change the way I approach development from here on out. My opinion is that maintaining this configuration is a poor investment and keeping old code around like this hoping it still works only slows us down and is unsustainable give our team size and product asks.

I think you can improve the experience of modern browsers without removing the Special:MobileMenu page

This looks promising. If the concern you've identified in this task is a priority, we would also need to rework our entire approach to the page actions overflow and user menus as they leverage the same mechanism proposed for this task and have no special page correspondences.

Could we also consider moving the Wikimedia-specific instrumentation for the main menu into the WikimediaEvents extension?

I think this was done in T228681 but could be mistaken.

The issue we had with the CSS checkbox hack was that the closing animation doesn't work and had to be removed

Workaround uncovered in T206354.

Could we also consider moving the Wikimedia-specific instrumentation for the main menu into the WikimediaEvents extension

That's already done ;)

In terms of browser support, the mechanism employed in the overflow menu & user menu is called the checkbox hack.
It was pioneered in 2010 and it depends on a few CCS level 3 features:

  • the :checked pseudo-selector
  • the ~ general sibling combinator

According to caniuse the browser support for these features is practically universal.

Now, we might be doing other things to mess up browser support (like not including the vendor prefixes in some flexbox declarations :P ) but the core mechanism of this technique is compatible with browsers from grades A-C.

Nevertheless, as @Niedzielski mentions, this does raise the issue of browser support in general and to what extent we should be supporting ancient browsers.

My opinion is that maintaining this configuration is a poor investment and keeping old code around like this hoping it still works only slows us down and is unsustainable give our team size and product asks.

👆I second this opinion. Maintaining legacy browser support could easily be enough work for an entire team.

the browser support for these features is practically universal.

I guess, in theory... http://timpietrusky.com/advanced-checkbox-hack

I'm guessing the slide in animation won't work though right?
It would be helpful to have a proof of concept that we can test on old browsers but I would want Olga, Alex to sign off on the experience, Edward to QA and probably the performance team to advise about additional CSS bloat before pushing forward with this task to completion.

Making sure the browser matrix is up to date first is also important so thank you for pushing that.

For what it' worth, this same technique is used in Vector. On Readers Web we're fond of saying YAGNI (You Ain't Gonna Need It) to get rid of piles of code and complexity. Given our resourcing, we don't need this.

I would want Olga, Alex to sign off on the experience

Adding @ovasileva and @alexhollender. Per T225213#5395241, do we need to test on Opera Mini and Android 2?

Jdlrobson added a comment.EditedAug 7 2019, 3:30 PM

Im not disagreeing that we dont need it but Vector has never had to work on a mobile browser and has terrible performance compared to mobile so I wouldn't use that as a justification. All I'm asking let's slow down and consult the right people before jumping into a solution. I believe we have more pressing concerns right now such as making the menu work nicely for grade A browsers. Let's focus energy there not here and come back to this when it's done.

@phuedx posted some nice queries specific to the page in question. I've modified them, hopefully correctly and they show that Special:MobileMenu had ~15k total pageviews last year. Of those, ~4.7k pageviews were Opera Mini visits.

Firstly, forgive the lateness of my reply and also if I've missed any discussion of this task (or the more general topic) while I've been away.

My opinion is that maintaining this configuration is a poor investment and keeping old code around like this hoping it still works only slows us down and is unsustainable give our team size and product asks.

I share and third this opinion. Characterising this decision as trading usability for performance mistakenly misses out this vital part of the conversation.


Speaking of performance, if I'm understanding the system correctly (major caveat):

If we are to consider only performance and usability in the rubric, we should include the negative performance impact of the above.

for non-JS users requires an extra page load.

Remember menu usage is very low (https://grafana.wikimedia.org/d/000000566/overview?panelId=22&fullscreen&orgId=1 logs events for 50% of all page views). Most people don't interact with the main menu. They want to read the article.

I think this is the likely scenario.

However – and this is a very minor nitpick – the conclusion "menu usage is very low" doesn't necessarily follow from the data as we haven't yet instrumented opening of the menu (see T220016#5404014 onwards).

I've modified them, hopefully correctly and they show that Special:MobileMenu had ~15k total pageviews last year. Of those, ~4.7k pageviews were Opera Mini visits.

@phuedx, identified some flaws in these queries: no pageview filter and no accounting for 1/128th sampling. Updated queries:

I had a look at this and got quite far with using the "#target" css selector, but because we lock the body tag (overflow hidden/height:100%) I'm not sure how to achieve that without pointing the main menu link at the body tag itself and relying on child selectors.

It seems doable, but does add additional CSS to the critical path (around 1kb - 10.37kb to 11.21kb)
https://gerrit.wikimedia.org/r/539974

Jdlrobson raised the priority of this task from Low to Normal.Wed, Oct 2, 10:38 PM
Jdlrobson removed a project: Patch-For-Review.

Bumping visibility given interest above and now AMC is quieting down. Not sure if this is something we want to pursue. If we are going to park Minerva for a bit, I can't argue with having less code to maintain and it seems we feel the performance problems are worth the trade off.