Page MenuHomePhabricator

AMC Navigation - overflow menu
Closed, ResolvedPublic8 Story Points

Description

Background

It is now time for the overflow menu. For background on navigation overall, see parent task.

User Story

User story: As an advanced editor, I want the ability to access useful pages quickly

The contents in the overflow menu will differ slightly per namespace (a complete list will be provided below). Generally the overflow menu includes the same links/contents as the “Tools” menu in the left sidebar in Vector, however there may be times where it doesn’t match exactly.

Each item on the menu will be tracked in the same schema where main menu tracking occurs

Main namespace

The overflow menu will contain:

action nameOOUI icon name
Download PDFdownload
Page informationinfo
Permanent linklink
What links herearticleRedirect
Cite this pagequotes
Wikidata itemlogoWikidata
In other projects*logoWikimedia

*= will add this item the future, not for first iteration

User namespace

The overflow menu will contain:

action nameOOUI icon name
Uploadsupload
User rightsuserAvatar
LogslistBullet
Page informationinfo
Permanent linklink
What links hereredo

General criteria

  • If a page does not contain one of the above links, do not present the link to that item
  • If a page does not contain any of the above links, do not display the overflow menu icon

QA

  • The overflow menu should not be present on the beta cluster when not logged. When logged in, it should _still_ not be present when AMC is enabled OR disabled. For all cases, please check a main namespace page and a user page.
  • The overflow menu should not be present on readers-web-stephen when not logged. When logged in, it should _still_ not be present when AMC is disabled; when enabled, it should be present. For all cases, please check a main namespace page and a user page.
  • When the overflow menu is present:
    1. JS and no-JS should be navigable correctly with touchscreen (mobile Minerva) and mouse / keyboard (desktop Minerva) navigation.
    2. All links should function properly. The page download link should zap into the menu only when JS is enabled.
    3. Note: a fade animation should be present only when opening the menu. When the menu is dismissed, the menu immediately disappears.
    4. Note: "Wikidata item" and "in other projects" are not present currently.
    5. Note: if possible, please also test using a screen reader.

QA Results

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ovasileva renamed this task from AMC Navigation - overflow menu to [Research] AMC Navigation - overflow menu.Mar 26 2019, 9:56 AM
ovasileva raised the priority of this task from Normal to High.
ovasileva added a project: Readers-Web-Backlog.
ovasileva moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
ovasileva updated the task description. (Show Details)Mar 26 2019, 5:22 PM
  • Add server side render
    • Does it use MenuBuilder? Are three any changes to menubuilder needed?
    • How will we show / hide for specific pages? Is that covered by menubuilder entirely?
    • Is there any client side code needed?
  • Add overflow button to toolbar
  • Add styling
    • Do not worry about scrolling, my friend
  • Add accessibility attributes

Question for Alex: what are the expectations for scrolling?

Outcome is to update the task, maybe prototype.

@alexhollender - discussed this during grooming and noted that we would like to add the scrolling behavior for the overflow menu to this task as well

ovasileva renamed this task from [Research] AMC Navigation - overflow menu to [Research 20hrs] AMC Navigation - overflow menu.Mar 26 2019, 5:39 PM
ovasileva updated the task description. (Show Details)
phuedx added a subscriber: phuedx.Mar 26 2019, 5:44 PM

Each item on the menu will be tracked in the same schema where main menu tracking occurs

T216152: AMC Navigation - add new links to main menu with click tracking currently contains a good explanation of wiring up items in the MainMenu to the MobileWebMainMenuClickTracking instrumentation:

Analytics

  • MainMenuClickTracking schema ID exists in MobileFrontend. We should move that to Minerva as this is confusing but apart from that all code is in Minerva.
  • All the code for communicating with EventLogging lives in Minerva in resources/skins.minerva.scripts/menu/MainMenu.js and resources/skins.minerva.scripts/menu/schema.js
  • 50% of clicks in the main menu are sampled (per wgMinervaSchemaMainMenuClickTrackingSampleRate)
  • Every link inside the MainMenu will be logged provided it has a data-event-name attribute

https://adoring-clarke-7bb0cd.netlify.com/demo-new.html

To expand on that a little bit, I did some preliminary research on the front-end implementation of this feature. The parts of this feature that I focused on were: the HTML/CSS & accessibility.

The HTML in the above demo is essentially a nested list:

<ul class="page-actions">
  <li>languages</li>
  <li>watchstar</li> 
  <li>edit</li> 
  <li> 
    <button>overflow menu icon</button>
    <ul>
      <li>first overflow menu item</li>
      <li>second overflow menu item</li>
    </ul>
  </li>
</ul>

I think this HTML structure is beneficial because it allows us to achieve the show/hide behaviour with CSS and maintain semantically correct DOM order.

Placing the overflow menu immediately after the overflow icon/button allows us to use the adjacent sibling selector the change the visibility of the menu, based on the focus state of the button (similar to the "checkbox hack"):

.overflow-menu { display: none; }
.overflow-button:focus + .overflow-menu { display: block; }

As far as I can tell, this seems to work on mobile devices well. The item is initially hidden, then when clicking the button it’s made visible, and remains visible while scrolling, and then when clicking elsewhere on the screen, it’s made hidden again.

The area where this CSS approach falls down though, is accessibility. Since screen-reader/keyboard-users depend on focus-state to navigate, so hiding the menu when the button looses focus makes it impossible to navigate via keyboard. To rectify this, javascript is indeed required to keep the menu open (and maybe on a blur event, close the menu?).

Javascript is also required to improve screen-reader accessibility. The aria-expanded attribute should be placed on the button to announce the state of the menu (expanded/collapsed).

So in terms of HTML/CSS & a11y, those are pretty much my findings from that prototype.

Change 501418 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] WIP: add secondary page actions menu in AMC mode

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

@alexhollender, still working through this but the following would help:

  • Please provide the assets wanted.
  • Do we need a vertical scrollbar in the menu itself? I explored this briefly and I have an idea for how we could do it but it's not a great experience. Basically, I set the maximum height to the viewport height and if the menu exceeds it, it adds a scroll bar. It's not a great experience because the maximum height is not adjusted with the scroll offset. That is, the menu initially fits on screen but once you start scrolling, it no longer can fit, which feels really inconsistent. I think it's fine to let this overflow without scrollbar. I can show you what I mean if it's helpful.
  • Where does the download link go?
    1. Is it the same link shown on the production mobile site's toolbar? If so, this can only be shown for the JavaScript render, not the server render.
    2. Or is the Vector download PDF link (E.g., https://en.wikipedia.org/w/index.php?title=Special:ElectronPdf&page=Dog&action=show-download-screen)?
    3. The Vector print link (E.g., https://en.wikipedia.org/w/index.php?title=Dog&printable=yes)?
    4. Or something else?
  • Where does the uploads link go? https://en.wikipedia.org/wiki/Special:Log?type=upload&user=Jdlrobson&page=&wpdate=&tagfilter=&subtype=?
  • I'm not planning to explore "in other projects" for this task since it will require new UI.

Per our discussion:

Please provide the assets wanted.

Alex to provide. Please also include the Wikibase, citation, and user page icons if possible.

Do we need a vertical scrollbar in the menu itself?

Alex is still experimenting.

Where does the download link go?

The existing download button moves from the toolbar into the overflow menu. However, given that there is a visible delay between when the page is initially rendered on device and when the download button appears, progressively enhancing the desktop download link seems preferable. That is, rendering the desktop's "Download as PDF" link on the server and replacing this with the client download link when supported. Unfortunately, this data appears to come from the ElectronPdfService and Collections extensions and the API isn't very clean. I was kind of hoping this would be a drop-in but I don't think it is. @pmiazga, is this something we should use?

There's another link on desktop, "Printable version", that also sounded like a promising backup. It's easy to access in the code too, $tpl->data['nav_urls']['print']. This basically adds a &printable=yes parameter to the URL like https://en.wikipedia.org/w/index.php?title=Dog&printable=yes. Unfortunately, the presentation on mobile is identical because it doesn't magically kick change the CSS media type to print rendering.

@alexhollender, ...So I'm leaning more towards zapping the download button into place like we currently do even though the experience isn't great :[

Where does the uploads link go?

This is in now: https://en.m.wikipedia.org/wiki/Special:Uploads/Jdlrobson


Alex's demo here: https://mobile-contributions.firebaseapp.com/nav4.html
Current patch here (be sure to enable AMC):

alexhollender updated the task description. (Show Details)Apr 9 2019, 5:31 PM

Following up on the remaining open topics:

1. Menu scrolling behavior

Regarding when there is not enough vertical space to display the entire menu, either because the phone is in landscape mode, or has a shorter screen.

For JS users we will do the following:

set a max-height on the menu, taking into account the offset from top of screenallow the menu to scroll internallyset a min-height of 220px so that the menu never gets too short

For non-JS users we will do the following:

Not set a max-heightdefer to scrolling the page to access the additional menu items
2. Menu item icons

I've updated the task description to indicate which OOUI icon to use for each menu item

@Niedzielski the Minerva print button is handled uniquely. First, after injecting button, it logs shownPrintButton (printing is not always available, there is no need to inject the button if you cannot print). Then when clicked - it doesn't add printable=yes to the URL, what it does, after it's clicked first it logs the click, then tries to load images on the page (the one that is lazyloaded), then calls window.print().

The Menu instrumentation will not be able to track that stuff as it is too complicated. What we can do is to track when a user clicks a menu item, nothing else. In this case, you'll need to add an entry that is not visible by default, then bind all actions on the frontend, and make it visible. MenuElement already knows how to handle those elements - when MenuElement is created with isJSOnly - the menu element will have jsonly class, which makes it hidden on non-js clients.

IMHO some menu entries should be added/shipped only with JS module, that makes those menu elements interactive (deciding whether to show the button, tracking button actions). Same will happen with the Share button, it's not available on all browsers, also it has special instrumentation that cannot be handled by the instrumentation built into the Menu system.

Change 501418 abandoned by Niedzielski:
WIP: add secondary page actions submenu in AMC mode

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

Change 501418 restored by Niedzielski:
WIP: add secondary page actions submenu in AMC mode

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

I've revised the images and scrolling behavior. The changes are most visible on user pages. E.g., https://readers-web-stephen.wmflabs.org/wiki/User:Hello.

All feedback is addressed. Latest changes are up https://readers-web-stephen.wmflabs.org/wiki/User:Hello. Open questions on download icon behavior and toolbar layout to be discussed tomorrow.

Change 503092 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Fix: don't overwrite additional icon classes

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

ovasileva renamed this task from [Research 20hrs] AMC Navigation - overflow menu to AMC Navigation - overflow menu.Apr 16 2019, 5:15 PM
ovasileva set the point value for this task to 8.Apr 17 2019, 5:08 PM
nray added a subscriber: nray.Apr 17 2019, 11:03 PM

Following up on the remaining open topics:

1. Menu scrolling behavior

Regarding when there is not enough vertical space to display the entire menu, either because the phone is in landscape mode, or has a shorter screen.
For JS users we will do the following:

set a max-height on the menu, taking into account the offset from top of screenallow the menu to scroll internallyset a min-height of 220px so that the menu never gets too short

For non-JS users we will do the following:

Not set a max-heightdefer to scrolling the page to access the additional menu items
2. Menu item icons

I've updated the task description to indicate which OOUI icon to use for each menu item

@alexhollender Regarding the "set min-height of 220px so that the menu never gets too short" spec, can you please clarify what we should do if the height of the menu fits within the viewport but is less than 220px? Should we still apply the min-height: 220px CSS rule or let the menu fit to its contents in that scenario? I ask this because if we apply the CSS rule in this scenario, there will can be white space at the bottom of the menu as pictured below; is that okay?

Change 503092 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix: don't overwrite additional icon classes

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

@nray well that height was set based on the assumption of the minimum number of items that would appear in that menu. Do you know if the case in the screenshot you posted above is a real one, or something that would only occur on staging. I also notice that there is no history icon in that screenshot, which seems peculiar.

nray added a comment.Apr 19 2019, 4:43 PM

@nray well that height was set based on the assumption of the minimum number of items that would appear in that menu. Do you know if the case in the screenshot you posted above is a real one, or something that would only occur on staging. I also notice that there is no history icon in that screenshot, which seems peculiar.

Sorry, yeah Stephen filled me in on this info after I posted this question. This screenshot was of my local machine where I try to keep the extensions down to a minimum so production would have more menu items.

Change 501418 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Update: add secondary page actions submenu in AMC mode

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

When testing I noticed couple things:

when AMC mode is on, a user signed up for the AMC experience, and the MinervaOverflowInPageActions is set to false, icons get organized in a different way:

As a non-amc user I see:


As AMC user with disabled MinervaOverflowInPageActions I see:

If I set MinervaOverflowInPageActions to be enabled as Beta user, and I join Beta mode, leave the AMC mode (only beta mode enabled) I see:

User Pages are bit messy. It would be awesome to clean it up:

User does not exist view:

User exists, but doesn't have a user page:

User exists, has a user page:

When download icon gets injected, FOUC is a bit more visible now

There is something we should reconsider (it's not possible to avoid), the FOUC when we inject download icon is a bit more visible now. There is a script that injects a new Icon if the print feature is available, that has to be done on the JS side. Previously the icon was just added before the "Watch", it didn't cause any reflow - a new icon jumped into empty space. Now when the AMC mode is on, all icons are nicely spaced (same distance between each icon), when we add a new icon (Download button) only far left and far right icon stay in the same place, all icons in the middle have to "readjust" and that is a bit more visible.

@pmiazga thanks for the feedback. I think it makes sense for @Niedzielski to respond to the first few points because I don't know which of those cases are unique to the staging environment, and which might actually occur in production.

User does not exist view

I would expect you to see something like what renders here: https://en.m.wikipedia.org/wiki/User:Asdas2382138asdasd

User exists, but doesn't have a user page:

if you are pointing out that there is no toolbar in this case, see T220114

When download icon gets injected, FOUC is a bit more visible now

We decided to put the download icon at the bottom of the menu, I'm not sure if that change was live yet when you tested.

@Niedzielski I've only been able to find one bug thus far. Sometimes when navigating to the Permanent link page from the overflow menu I see a flash of the menu on the Permanent link page. I can't reproduce this consistently, and I have not yet seen it on iPhone/Safari (only desktop & Android/Chrome).

video of menu flashscreenshot from 0:03 seconds

Regarding the discussion/talk page - would it be possible to include the History page icon/action in the toolbar? Perhaps I should make a subtask for T213352 for that. Apologies for not catching this earlier.

Change 506526 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Fix: AMC overflow menu initial CSS and SSR icons

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

I've only been able to find one bug thus far. Sometimes when navigating to the Permanent link page from the overflow menu I see a flash of the menu on the Permanent link page. I can't reproduce this consistently, and I have not yet seen it on iPhone/Safari (only desktop & Android/Chrome).

Thank you for spotting this bug. This was a flaw (actually two) in my original code.

Regarding the discussion/talk page - would it be possible to include the History page icon/action in the toolbar? Perhaps I should make a subtask for T213352 for that. Apologies for not catching this earlier.

I think a new task would be best. Work has already started to transition the overflow menu to Piotr's new menu system and I don't want to make more changes that will need to be migrated or wait until that work is done to resolve this task.

nray added a comment.Apr 25 2019, 9:03 PM

@Niedzielski I've only been able to find one bug thus far. Sometimes when navigating to the Permanent link page from the overflow menu I see a flash of the menu on the Permanent link page. I can't reproduce this consistently, and I have not yet seen it on iPhone/Safari (only desktop & Android/Chrome).

video of menu flashscreenshot from 0:03 seconds

Regarding the discussion/talk page - would it be possible to include the History page icon/action in the toolbar? Perhaps I should make a subtask for T213352 for that. Apologies for not catching this earlier.

@alexhollender regarding the menu flash bug, what chrome version are you using (desktop) just out of curiousity? No matter how hard I try, I cannot reproduce the issue you are seeing, but I did notice this 5 year old chrome bug ticket here (still active!) that seems related to the issue https://bugs.chromium.org/p/chromium/issues/detail?id=332189

nray added a comment.Apr 25 2019, 9:08 PM

^^ fyi, I'm on chrome Version 74.0.3729.108

@nray Version 74.0.3729.108 (Official Build) (64-bit)

nray added a comment.Apr 25 2019, 11:13 PM

Update: I've successfully replicated the flash that alex was seeing on my android phone (Pixel 2)! Ironically, it happened for me on the page "Nothing to Hide" (http://readers-web-staging.wmflabs.org/w/index.php?title=Nothing_to_Hide_(book)&mobileaction=toggle_view_mobile)

Change 506526 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Fix: AMC overflow menu initial CSS and SSR icons

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

Stephen to add!

@Edtadros - just wanted to note that in addition to beta we should also QA this in production once it's out

Edtadros reassigned this task from Edtadros to ovasileva.May 8 2019, 11:51 PM
Edtadros added a subscriber: Edtadros.

Test Result

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

Test Artifact(s):
✅ AC1: The overflow menu should not be present on the beta cluster when not logged.


When logged in, it should _still_ not be present when AMC is enabled

OR disabled.

✅ AC2: The overflow menu should not be present on readers-web-stephen when not logged.

When logged in, it should _still_ not be present when AMC is disabled;

when enabled, it should be present.

✅ AC3: When the overflow menu is present:
JS and no-JS should be navigable correctly with touchscreen (mobile Minerva) and mouse / keyboard (desktop Minerva) navigation.
JS: See AC1 and AC2

no-JS: I turned JS off and confirmed that it behaved the same.

All links should function properly. The page download link should zap into the menu only when JS is enabled.

✅ AC4: a fade animation should be present only when opening the menu. When the menu is dismissed, the menu immediately disappears.

✅ AC5: if possible, please also test using a screen reader. Tested with ChromeVox.

Edtadros updated the task description. (Show Details)May 8 2019, 11:52 PM
ovasileva closed this task as Resolved.May 20 2019, 9:36 AM

All done, thanks @Niedzielski and @Edtadros! Created T223883 for turning it on. All done here.