Page MenuHomePhabricator

Main menu should slide over content
Closed, ResolvedPublic5 Story Points

Description

Description

The main menu does not act like a drawer. It is not opened. Instead, the content is re-positioned around the drawer. This makes the drawer feel foundational, as if the website was literally built on top of the menu, not the other way around.

This task posits that the content is what's most important and should carry the most weight. The menus, drawers, and dialogs should be lightweight peripheral views that glide over the top and invite dismissal, not heavyweight hitters that shove or shift the content out of the way to show secondary information and require beckoning the content back into view instead of drawer dismissal. Animating drawers in and out over the content is far less distracting than animating the content and primary focus in and out.

A lightweight drawer feels much more appropriate. E.g:

Designs

Developer Notes

If we don't support the swiping action mentioned in the above section and given that the .primary-navigation-enabled class is toggled on the body element when the hamburger menu is toggled, it should theoretically be possible to make this change only with CSS.

For mobile, the main menu is currently set to a relative width using a percentage

.animations #mw-mf-page-left {
    width: 75%;
}

It appears that doing a transform with a translate percentage i.e. transform: translate(75%,0); is compositor thread unfriendly and will cause the animation to stay on the main thread (at least in chrome) and this can cause unnecessary jank. When animating the main menu, it might be worth using vw units instead i.e. transform: translate(75vw, 0);.Consider using a feature query with a fallback for the browsers that don't support vw) as vw units appear to be compositor thread friendly [1].

Acceptance Criteria

[1] https://stackoverflow.com/questions/50409008/css-transform-transition-using-%C2%B4px%C2%B4-more-smooth-performant-than-percentage
[2] https://phabricator.wikimedia.org/T206354#5209466

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
alexhollender updated the task description. (Show Details)Apr 3 2019, 6:29 PM

Olga please advise how important this is in relation to the history page work. I'd rather focus our analysis on one of these for the time being to avoid burn out. My guess from our conversations is that history is more important than menu but I may be wrong. It also seems like it will be more straightforward given the troubles we've been hitting with our Javascript in recent weeks.

Lower in relation to history page work. I think normal priority makes sense here (with all history page tasks being high as of now). From a priority perspective, I think it would be best to work on this once we've finished the other portions of the navigation.

ovasileva removed ovasileva as the assignee of this task.Apr 9 2019, 3:46 PM
ovasileva added a subscriber: ovasileva.
nray added a subscriber: nray.EditedMay 23 2019, 11:29 PM

I am very much for this change, and I think there are important performance benefits here as well.

In an attempt to make these main menu/notification animations smooth we promote the .mw-mf-page-center element which encapsulates all of the content to its own compositor layer (through the -webkit-backface-visibility CSS property).

This uses up memory on any device but is particularly expensive on a mobile device since it is such a limited resource. With multiple sections open on the Obama page, I've seen reported memory from this layer alone to be upwards of 2,700 MB (although I'm not entirely sure how accurate this is as desktop reports this layer to be somewhere around 300 MB). Either way, it eats up memory. See screenshot below:

Furthermore, despite these efforts, this animation is still very much running on the main thread. The .mw-mf-page-center element's left property is set when the menu is opened in addition to other styles that are compositor thread unfriendly. This makes this animation both influenced by jank and cause jank since it will run on the main thread. On my phone (Pixel 2), it rarely comes close to hitting 60 fps [1] as a result.

I think it would be much easier to make this animation compositor thread friendly and limit the amount of memory consumption if we made the menu slide over the content as this ticket instructs since we could rely solely on css transforms and the menu would be a much smaller layer than the mw-mf-page-center element. If we do this, we have a much better chance at minimizing the jank [2] and memory consumption!

[1] https://practical-pasteur-ded885.netlify.com/#/22
[2] https://practical-pasteur-ded885.netlify.com/#/10

Edit: I mentioned above that mw-mf-page-center element's left property was compositor thread unfriendly for the animation, but I think it actually might be the fact that we use a percentage in .mw-mf-page-center's animation transform: translate(75%,0);. The left property is not being animated.

nray added a subscriber: Krinkle.Jun 7 2019, 8:24 PM

@Krinkle Are there any other points you'd like to add to my read of the menu situation performance? I'd love to hear any insights your team has on it as well!

@nray Looks good to me.

Animations like this should indeed use transforms instead of positional animations so that it can be offloaded to the composite thread instead of the main thread.

Regarding the layer memory; Layers do come with memory overhead indeed. I often see memory issues when there are many sub trees on a page with their own composite layer. Or when a single structure each each inner child be its own layer. I haven't seen in the past problems from a single chunk that's very big as 1 layer. It will have most of the graphics memory associated with it, but I would expect most of that to remain without the layering as well. The pixels would instead be associate with the default layer (minus the overhead of having a separate layer, which for 1 layer should be minimal).

Having said that, I'm seeing at least in Chrome/desktop, that on mobile the default layer seems to be the same size as the mw-mf-page-center layer. And removing the latter, doesn't change the size of the former. This might have to do with how Chrome measures/shows the information. Or, it may've changed recently to not be less efficient. (Or maybe I misunderstood how it worked before).

@Niedzielski questioning whether or not this drawer pattern is appropriate for Notifications. We don't have this well defined but here is an attempt at articulating the differences between drawers and modals/overlays.

Modal

  • takes up the entire screen
  • has a unique URL
  • has a close button in the top left
  • appears on top of content
modal examples

Drawer

  • does not take up the entire screen
  • does not have a unique URL
  • can be closed by tapping outside of the drawer area (can also include a close button)
  • appears on top of content
drawer examples

It seems like Notifications should be rendered in a modal/overlay. I wonder if the appropriate thing to do here (or via a separate task) would be to modify the transition/animation on the Notifications modal so that it appears like other modals do, rather than pushing the content and sliding in from the side?

Niedzielski renamed this task from Menu and notifications drawers should slide over content to Main menu should slide over content.Jun 19 2019, 8:12 PM
Niedzielski updated the task description. (Show Details)

@alexhollender, we chatted about this yesterday in grooming. As I understand it, we will focus on the main menu initially for AMC so a new task seems appropriate. I've taken a stab at splitting the notification work off into T226125 and integrating your thoughts. Modal or drawer both sound nice to me. The former may make more sense currently since notifications are only loaded upon request. Edits welcome!

@Niedzielski thanks for making the updates and the new task

Jdlrobson assigned this task to nray.Jul 9 2019, 4:34 PM
Jdlrobson moved this task from Upcoming to Needs Prioritization on the Readers-Web-Backlog board.

Do we expect this to be a CSS only change or require some JS changes? Needs a summary from a developer to make this something we can estimate.
Seems like Nick has a good grasp of this so can I ask that you summarise T206354#5209466 in the description so it has acceptance criteria and/or developer notes?

nray updated the task description. (Show Details)Jul 16 2019, 12:34 AM

Do we expect this to be a CSS only change or require some JS changes? Needs a summary from a developer to make this something we can estimate.
Seems like Nick has a good grasp of this so can I ask that you summarise T206354#5209466 in the description so it has acceptance criteria and/or developer notes?

@Jdlrobson I added AC/dev notes to the ticket for as much as I know currently

nray updated the task description. (Show Details)Jul 16 2019, 12:40 AM
nray updated the task description. (Show Details)
nray updated the task description. (Show Details)
Volker_E added a comment.EditedJul 16 2019, 12:51 PM

@nray If I were able to, I'd add another gold medal token for your dev notes. Excellent!

nray removed nray as the assignee of this task.Jul 16 2019, 3:52 PM
ovasileva updated the task description. (Show Details)Jul 16 2019, 4:30 PM
Jdlrobson updated the task description. (Show Details)Jul 16 2019, 4:31 PM
ovasileva set the point value for this task to 5.Jul 16 2019, 4:45 PM

Change 527215 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] [dev] divide main menu LESS

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

Change 528184 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] [fix] hide root scrollbars when showing overlay

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

Change 528185 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] [fix] notification drawer closing animation

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

Change 528189 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] [menu] [UI] [notifications] make notifications slide over top

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

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

@alexhollender, visible changes are here: https://readers-web-stephen.wmflabs.org/wiki/Foobarbaz. I'm looking into a test failure but need your blessing on the visual changes.

Change 528284 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] MainMenu no longer manages classes on the body tag

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

@Niedzielski wow, this is rad man. It feels so good : )

Two things:

  1. Should we give the menu a max-width?
desktopAndroid in landscape mode
  1. I wonder if we need to disable page scrolling while the menu is open to avoid this situation?

Regarding additional design review: are there things I should specifically look at/check? I looked at the menu on iOS, Android, and desktop. I also tried it logged-out, logged-in, and logged-in+AMC.

+1 to max-width. Please take into account that the logout button is right aligned aside of users with possibly a very long user name.

Change 527215 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [dev] divide main menu LESS

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

Change 528189 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [menu] [UI] [notifications] make notifications slide over top

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

Change 528284 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] MainMenu no longer manages classes on the body tag

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

Change 528184 abandoned by Niedzielski:
[fix] [menu] [UI] hide scrollbars when showing overlay

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

Should we give the menu a max-width?
+1 to max-width. Please take into account that the logout button is right aligned aside of users with possibly a very long user name.

I've changed the max-width from tablet (600px) to a min-width of 250px that feels a lot lighter to me. Let me know what you think.

I wonder if we need to disable page scrolling while the menu is open to avoid this situation?

This was a bug. Fixed.

Latest changes posted to https://readers-web-stephen.wmflabs.org/wiki/Foobarbaz. Please clear your cache.

Change 528185 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] [fix] [menu] [UI] [notifications] notification drawer closing animation

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

Change 528264 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [UI] [menu] slide the main menu over the page

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

ovasileva reassigned this task from Edtadros to phuedx.Aug 20 2019, 5:08 PM
ovasileva added a subscriber: Edtadros.
phuedx updated the task description. (Show Details)Aug 21 2019, 5:35 PM

This LGTM. I have a question for @alexhollender but I don't believe that the answer (and the associated follow-up change) could invalidate the results below.

Results

I tested the menu while logged in (with an overly long username) and logged out using:

  • Chrome's mobile viewport simulator feature for a small number of devices
  • A physical Nexus S (Android Browser 4.0 on Android 4.1.2)

Despite viewport units being reported as supported by Android Browser 4.4+, I saw no issues on this device.

Questions

  1. The design in the description (currently F29605664) shows the menu having a drop shadow. @alexhollender: Is it still the case that the menu should have a drop shadow (i.e. that it's above the content like a drawer)?

Artefacts

Logged in
DeviceOrientationScreenshot
iPhone 5 SEPortrait
iPhone 5 SELandscape
iPadLandscape
Nexus SPortrait
Nexus SLandscape
Logged out
DeviceOrientationScreenshot
iPadLandscape
Nexus SPortrait
phuedx removed phuedx as the assignee of this task.Aug 22 2019, 11:32 AM
phuedx added a subscriber: phuedx.

I think I missed this. Do we want to make the notification drawer shadow match while we're at it? (Currently it's a hard outline.)

nray added a comment.Aug 22 2019, 8:00 PM

I checked this out today (per Sam's comment in standup) and looks great.

Try to make the menu animation stay on the compositor thread using compositor thread friendly properties. See https://developers.google.com/web/fundamentals/performance/rendering/stick-to-compositor-only-properties-and-manage-layer-count for more info.

The animation is still occurring on the main thread due to using a percentage in translate (e.g. transform: translate(-100%,0)), but I'm not that concerned about that, and Stephen mentioned foregoing this AC in a slack thread so it shouldn't be a blocker for sign off 👍

nray updated the task description. (Show Details)Aug 22 2019, 8:01 PM

@phuedx great catch on the drop shadow. I actually noticed that as well today and created T231205.

phuedx closed this task as Resolved.Aug 27 2019, 11:23 AM

Thanks for creating that task, @alexhollender. Given T206354#5431259, T206354#5432542, and the existence of T231205: Add shadow to nav menu drawer, I think this task is OK to sign off.