Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Page MenuHomePhabricator

Add configurable scroll behavior to the Menu component
Closed, ResolvedPublic3 Estimated Story Points

Description

Summary

Currently, the menu component displays all menu items provided to it at once and does not support showing some of the menu items initially, then allowing the user to scroll to the rest of them (this would require the menu container having a max-height)

This behavior is critical to components with menus, which may have any number of menu items.

Current behavior

General menu behavior

If a menu component with many items is inside a container with limited height that becomes scrollable when it's too tall, it look like this:

Screenshot from 2022-03-16 21-58-22.png (429×772 px, 27 KB)
Screenshot from 2022-03-16 21-58-36.png (440×763 px, 27 KB)
Screenshot from 2022-03-16 22-03-09.png (431×761 px, 23 KB)

If it's not inside such a container, it expands over the rest of the page endlessly, and you have to scroll down the page to get to the item you're looking for (then back up once you've chosen an item).

Screenshot from 2022-03-16 22-04-24.png (990×804 px, 87 KB)
Screenshot from 2022-03-16 22-04-40.png (726×428 px, 59 KB)
Screenshot from 2022-03-16 22-04-54.png (629×313 px, 41 KB)
TypeaheadSearch

The default search client in Vector sets a default limit of 10 results. We need to make sure TypeaheadSearch continues to display up to 10 results without scroll in the existing implementation in Vector. There are 2 ways to achieve this, depending on what we define as default behavior:

  • If the default behavior is to show all menu items at once, nothing needs to be done to keep TypeaheadSearch working as it currently works
  • If another default number of results shown initially is set, we will need to configure this limit to be 10 for TypeaheadSearch

In a future task (T306933), we will make the number of results initially shown configurable within CdxTypeaheadSearch in order to enable scroll there, but for this task, we should maintain the status quo.

Design

This Figma file contains the specifications for the configurable scroll of the Menu component.

Considerations

  1. By default, Menu displays all available options at once (i.e. the current behavior).
  1. Allow setting a custom max number of visible menu items: Implementers of the component should be able to define the maximum number of items that menu will display by default. Recommendations regarding the minimum (3) and the maximum (10) number of options will be provided in the Menu's demo page.
  1. Item selection behavior in “scrollable” menus: Given that users have selected an item from a scrollable menu, when the menu is reopened, the scroll position will be maintained, and the item will display a highlighted state (selected + keyboard focus/hover). Headsup: this only applies to selection menus, since selection is not preserved in navigation menus (such as the TypeaheadSearch’s).
  1. Footer selection behavior in menus with custom scroll: Given a scrollable menu with a footer, when users scroll, the menu’s footer keeps a fixed position at the bottom.
Users can use their mouse or keyboard to access available options that are not on sight. Keyboard users will be able to access the footer from the last option in the menu using one more down arrow (⬇) stroke or the end key.

Implementation

Number of items initially displayed

This could be configurable via a prop, or we could instruct library users to limit the number of menu items they provide to the Menu component in order to set a specific limit. If we do add a prop, it should probably default to null, or no limit.

This configuration will need to be part of the menuConfig object, so it can be provided to parent components like Lookup or Select, then passed down to Menu.

Add scroll behavior

We will need to:

  • Set a max height of the menu, which may need to be calculated on the fly
  • Test out and improve scrolling behavior so it meets UX standards
  • Ensure keyboard navigation still works, and that an interactive footer (e.g. the search footer in TypeaheadSearch) can still be reached easily
  • Ensure that, when the menu is closed then reopened, the resulting UX is as expected. We should especially consider this when a menu item is selected—should the menu open to that item?
Demonstrate this behavior

We should add the following demos:

  • Menu: simple demo with an explanation of how this all works
  • Lookup, Select: realistic demos showing how this can be configured, with a reference to the Menu page if needed (should we add one for Combobox, too?)

Acceptance criteria

  • The Menu component can be configured to show an initial set of items, then allow scrolling through the entire list of menu items
  • This behavior can be configured in the Combobox, Lookup, and Select components and there are demos of at least the latter 2 present to show this behavior (demos for components that use a menu will be done later)
  • A demo is added to the Menu demo page displaying and explaining this behavior
  • No changes are made to the current UX of TypeaheadSearch, as this will be implemented separately

Event Timeline

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

@DAbad for visibility: the behavior described in this task is a requirement for TypeaheadSearch in the context of Wikidata. This is why we assumed and documented (see backlog) that this task could be a good candidate to be handled by our kind WMDE contributors.

Ensure that, when the menu is closed then reopened, the resulting UX is as expected. We should especially consider this when a menu item is selected—should the menu open to that item?

Currently, it seems to be the case that a selected menu item also has the keyboard focus when a menu is reopened. In that case, scrolling to that item seems necessary.

However, a more open question: Should we scroll far enough that the element can just be seen, or should we scroll to a point where it is in the middle of the menu (if possible)?

(Not sure what to do with the Combobox though if two or more MenuItems are selected 🤔 .)

Michael changed the task status from Stalled to Open.Sep 12 2022, 12:09 PM

Also, I don't think this is stalled anymore.

ldelench_wmf set the point value for this task to 3.Sep 12 2022, 4:35 PM

This estimate (medium) represents the simplest implementation

@Michael FWIW, we have not yet implemented multiselect in any menu components. For this initial iteration, we could find a solution for scrolling to the single selection, then worry about multiselect when we implement it in the future.

Change 831887 had a related patch set uploaded (by Michael Große; author: Michael Große):

[design/codex@main] WIP: Add configurable scroll behavior to the Menu component

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

Change 833329 had a related patch set uploaded (by Noa wmde; author: Noa wmde):

[design/codex@main] Lookup: Limit default number of visible menu items to 6

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

AnneT changed the task status from Open to In Progress.Sep 20 2022, 8:18 PM

Change 833853 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] docs, Menu: Separate menu scroll into its own demo

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

Tiniest nitpick about the demo, but the width of the input to enter the number of visible items should be increased, so it can take two digits:

Screenshot 2022-09-23 at 20.39.29.png (360×898 px, 24 KB)

I think 2.5em in width would work fine.

On the other hand, it'd be great if the label and the input were aligned in the center of their flex container:

Screenshot 2022-09-23 at 20.45.20.png (106×976 px, 20 KB)

Change 831887 merged by jenkins-bot:

[design/codex@main] Menu: Add configurable scroll behavior

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

Change 836241 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] docs, Menu: Increase width of number input

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

Tiniest nitpick about the demo, but the width of the input to enter the number of visible items should be increased, so it can take two digits:
On the other hand, it'd be great if the label and the input were aligned in the center of their flex container

@Sarai-WMDE I've included both of these changes in the latest patch!

Change 833853 merged by jenkins-bot:

[design/codex@main] docs, Menu: Separate menu scroll into its own demo

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

Change 836241 merged by jenkins-bot:

[design/codex@main] docs, Menu: Improve display of number input

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

@Sarai-WMDE and @EUdoh-WMF, you can test this new functionality via the "menu with scroll enabled" demo on the docs site. No other demos should be affected since the default behavior is to show all menu items at once.

Change 832271 had a related patch set uploaded (by Michael Große; author: Michael Große):

[design/codex@main] Add cypress browser test for Menu component scroll functionality

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

Change 832271 merged by jenkins-bot:

[design/codex@main] Add cypress browser test for Menu component scroll functionality

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

This task can be signed-off from a design perspective. Following the ACs, users of the Menu component can now define the number of visible items that the menu will display in context. Further options are available on scroll. As specified, the scroll position is maintained when the menu is reopened after a selection has been made, so the current selection is always visible/accessible regardless of its placement within the menu.

No alterations of the visual or default interactive features of the Menu component were detected as a consequence of these changes.

Tested in Chrome v105, Firefox v105 and Safari v15.6 on macOS Monterey.

Change 838268 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] docs: Add more scrolling demos and load-more docs

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

Change 838268 merged by jenkins-bot:

[design/codex@main] docs: Add more scrolling demos and load-more docs

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

@Catrope @SaraiSan

1.Chrome v106: At 175% zoom, there is a faint blue border at the top of the first menu item.

image.png (956×2 px, 116 KB)

  1. Chrome v106: At 125% zoom and with a number of visible items as 1, there appears to be a bottom border with some markings or text reflections. This shows up as an white space on Firefox v105
image.png (796×2 px, 205 KB)
image.png (1×2 px, 327 KB)
  1. All browsers: when the text input has focus, its blue border outline should be of equal thickness all over the component, as designed. In the demo page, the bottom border outline is thinner.
image.png (1×1 px, 210 KB)
image.png (1×1 px, 149 KB)

@Catrope @SaraiSan

1.Chrome v106: At 175% zoom, there is a faint blue border at the top of the first menu item.

image.png (956×2 px, 116 KB)

@EUdoh-WMF that blue border also appears to me just when the menu is scrollable.

Captura de Pantalla 2022-10-24 a las 9.55.34.png (1×2 px, 241 KB)
Captura de Pantalla 2022-10-24 a las 9.56.45.png (952×2 px, 89 KB)
scrollable menu (with blue border)not scrollable menu (without blue border)

Although this blue border appears when the screen is 175% zoom, it doesn't appear when the screen is 100% zoom or other zoom than 175%. So I don't see this blue border as a blocker since it only affects the 175% zoom which I don't think is very common while users browse.


  1. Chrome v106: At 125% zoom and with a number of visible items as 1, there appears to be a bottom border with some markings or text reflections. This shows up as an white space on Firefox v105
image.png (796×2 px, 205 KB)
image.png (1×2 px, 327 KB)

I agree, when the number of visible items is 1 there is 1px with no white background and the text "Number of visible items in Menu" below the menu component is visible between that 1px transparent.


  1. All browsers: when the text input has focus, its blue border outline should be of equal thickness all over the component, as designed. In the demo page, the bottom border outline is thinner.
image.png (1×1 px, 210 KB)
image.png (1×1 px, 149 KB)

@EUdoh-WMF I agree, as specified in the Figma spec the focus should border should be 2px if possible on all sides.

Captura de Pantalla 2022-10-24 a las 10.05.00.png (880×964 px, 308 KB)

Change 849191 had a related patch set uploaded (by VolkerE; author: VolkerE):

[mediawiki/core@master] Update Codex from v0.2.1 to v0.2.2

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

Change 849191 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.2.1 to v0.2.2

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

Change 850261 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] Menu: Remove -1px margin-top from menu

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

@EUdoh-WMF @bmartinezcalvo I've opened a patch that I think should resolve all of those things, although the bug that happens in Chrome at 175% zoom is still a bit of an issue. Check it out here.

The 175% bug, where you can see a tiny bit of the underlying text, is probably an unfortunate rendering thing in Chrome. The calculations for the Menu border and height are correct, and everything looks right at other zooms and in other browsers, so like Bárbara said, I don't think we should spend time trying to cater to this.

The patch resolves these issues by removing the -1px border-top that used to be added to the menu element. The designs show the input and menu sitting next to each other with no overlap, so that's what I've tried to achieve here. Please be sure to test all components that have menus (Combobox, Lookup, Select, and TypeaheadSearch).

cc @Sarai-WMDE on the message above ^ if you have a second, could you please confirm if this is the correct style now, especially for TypeaheadSearch? I had thought there was supposed to be a -1px margin-top on the menu, but it seems like that's not indicated in the designs.

@bmartinezcalvo The reason for the -1px negative margin in past design decision in OOUI was to make sure the menu is overlaying the calling element, to also support the elevated layering with this visual detail (I vaguely remember some technical reason as well, but don't have it on top of my mind right now).
If we do so, we have to do so in all OOUI widgets as well! Are we sure that's “better”?

Hey all. Regarding the removal of that -1px border-top. The change (which I still support) was suggested a while back (see original task, T307767): me and Alex noticed that cdx-TypeaheadSearch was misaligned with the original TahS – and other components – in production. After consulting the Product design team (via Slack), some members also expressed their preference for this new visual approach.

I decided to capture this desired new direction in the designs (also, at the time, Figma didn't allow negative autolayout!). The task was brought up for discussion with the DST, and we decided to put it on hold due to issues mentioned by @Volker_E in this comment: https://phabricator.wikimedia.org/T307767#8026555". (I believe he meant to write "Issues with adjacent (Option B)", btw). So, this is blocked by an internal disagreement that we should try to resolve.

The reason for the -1px negative margin in past design decision in OOUI was to make sure the menu is overlaying the calling element, to also support the elevated layering with this visual detail

It's very common for menus to not touch (or even have a 1px margin separation with) their inputs, and rely on box-shadow to convey elevation. I don't think that the border overlap is essential to reinforce that, and that on the contrary, the result of having active inputs that present even borders is very desirable. Again, I'd support moving ahead in this direction.

Nevertheless, I understand the logic behind your concern. A way to reduce that "Escher effect" potentially caused by adjacent menus could be to apply 2px to all of the menu's border-radius. We can discuss this in a follow-up design round. I think that this approach would 1. allow us to convey elevation, while 2. keeping the menu from overlapping with the input, and 3. removing the need to flip border radius for upward-opening menus. It would also make dropdown and contextual menus consistent. I know it's quite a different direction, but had to share as it seems to be convenient.

Screenshot 2022-10-28 at 15.56.15.png (466×902 px, 49 KB)

Hi Team,

Thank you for the perspective and comments. @AnneT 's patch works just fine on the major browsers: Chrome v106, Safari v16, Firefox v106. The issues I stated are not occurring anymore, even the blue line I mentioned in point 1 appears to have been fixed!

Could we accept such a slight divergence between Codex and OOUI? I would hate to hold back Codex because of the old system, and I'm not convinced that both versions must be completely in sync, especially when it comes to a single-pixel difference.

Could we accept such a slight divergence between Codex and OOUI? I would hate to hold back Codex because of the old system, and I'm not convinced that both versions must be completely in sync, especially when it comes to a single-pixel difference.

Agreed

Could we accept such a slight divergence between Codex and OOUI? I would hate to hold back Codex because of the old system, and I'm not convinced that both versions must be completely in sync, especially when it comes to a single-pixel difference.

@AnneT I agree with this small divergence between Codex and OOUI in this case.

@Volker_E can we move forward with this patch and accept that we will not make a similar change in OOUI, per the previous comments?

Moving to Design Sign-Off and assigning to @Volker_E per the previous comment

Change 850261 merged by jenkins-bot:

[design/codex@main] Menu: Remove -1px margin-top from menu

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

Change 865151 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[mediawiki/core@master] Update Codex from v0.3.0 to v0.4.0

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

Change 865151 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.3.0 to v0.4.0

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