Page MenuHomePhabricator

[AMC] Menu handle has no active state
Closed, ResolvedPublic2 Estimated Story Points

Description

Description

We should add an active state to the user icon and more icon that remains while the menus are open to help ensure that people know what they tapped on.

Design

Maybe we can just use the current active/touch state here? I'm not aware of other places on mobile where we do this. It's okay if the treatment differs slightly among browsers.

QA Steps

Perform those steps as AMC user on the beta cluster

  • Verify that User Menu trigger (user icon in the header) has a darker background when the user menu is visible
  • Verify that Overflow Menu trigger (three dots icon in the toolbar) has a darker background when the overflow menu is visible

Please verify that behavior is correct on popular mobile devices (iOS, Android, etc).

QA Results

ACStatusDetails
1T230034#5522682
2T230034#5522682

Event Timeline

Esanders created this task.Aug 7 2019, 3:52 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 7 2019, 3:52 PM
alexhollender updated the task description. (Show Details)
alexhollender renamed this task from Menu handle has no active state to [AMC] Menu handle has no active state.Aug 18 2019, 3:27 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

This functionality is currently disabled for the user and overflow menus as it was for other elements on mobile:

// ToggleList.less
.touch-events .toggle-list__checkbox:focus + .toggle-list__toggle {
	// Buttons have no focus outline on mobile.
	outline: 0;
}

By removing this code, the Chromium emulator presentation looks like:

This appearance is set according to the following code:

.toggle-list__checkbox:focus + .toggle-list__toggle {
	// The toggle button / label itself cannot receive focus but the underlying checkbox can. Keep
	// the button and checkbox focus presentation in sync. From
	// resources/src/mediawiki.toc.styles/screen.less.
	outline: dotted 1px; /* Firefox style for focus */
	outline: auto @colorProgressiveHighlight; /* Webkit style for focus */
}

I think we just need a design decision.

I think we just need a design decision.

@Nirzar - any thoughts from your side? (since @alexhollender is out this week)

Jdlrobson added subscribers: ovasileva, Jdlrobson.

needs analysis. Assign to Olga when ready for estimation.

@Niedzielski currently when I tap on the menu handles they have a highlighted touch state, so my hope was that we could persist that same treatment while the menu is open. I.e. is it possible to have gray background highlights instead of the blue box?

Ok, I think we want something like this then:

.toggle-list__checkbox:checked + .toggle-list__toggle {
	background: @colorGray14;
}
alexhollender removed alexhollender as the assignee of this task.Aug 26 2019, 7:02 PM

Ok, I think we want something like this then:

.toggle-list__checkbox:checked + .toggle-list__toggle {
	background: @colorGray14;
}

if you can post a screenshot of how that would look that'd be awesome.

.toggle-list__checkbox:checked + .toggle-list__toggle {
	outline: 0;
	background: @colorGray14;
}

@Niedzielski looks great. Does it also work for the User menu handle?

@alexhollender, hm, since the toolbar is already grey maybe we need to use a translucent background. Something like:

.toggle-list__checkbox:checked + .toggle-list__toggle {
	outline: 0;
	background: rgba(1,1,1,.1);
}

I was thinking that we can work out the actual changes needed when this enters the doing column. This is looking pretty ready to me.

@Niedzielski looks good to me as well. Sorry, I thought we were already "doing" :)

ovasileva set the point value for this task to 3.
ovasileva changed the point value for this task from 3 to 2.
Volker_E added a comment.EditedSep 4 2019, 8:48 PM

Note, that in the default interface treatment we go for increasing contrast of foreground elements (icons, text) to black when active background color sets in.

Change 534622 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Show darker background when toggle list is open

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

pmiazga removed pmiazga as the assignee of this task.Sep 5 2019, 2:23 PM
pmiazga added a subscriber: pmiazga.
Jdlrobson assigned this task to pmiazga.Sep 9 2019, 5:09 PM
pmiazga removed pmiazga as the assignee of this task.Sep 10 2019, 12:11 PM
nray added a subscriber: nray.Sep 10 2019, 5:38 PM

@pmiazga I moved this to QA, but I'm not sure if this is more design QA. Either way, can you add the appropriate QA steps and ensure this is in the right column?

Jdlrobson added a subscriber: Edtadros.

thx @nray, @alexhollender this thing will be available to verify on beta cluster/staging env

Change 534622 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Show darker background when toggle list is open

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

Looks good on iOS/Safari

On Android/Chrome the active state is darker than the focus state, which creates a minor flash. Is there any easy way to fix this?

initial touchmenu open

after icon changes you get a square, hurraz.

after icon changes you get a square, hurraz.

This is extremely beautiful and satisfying.

Change 537179 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] DoNotMerge: remove highliht for ToggleList trigger tap

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

The highlight color can be modified by -webkit-tap-highlight-color,

  • we can disable the highlight color for menu handles, but it will bring inconsistency between user menu and notifications menu, same it will bring inconsistency between icons on toolbar and the overflow menu
  • set the highlight to given color - we will need two CSS rules, one for user menu (it's on dark background), and a second one for overflow menu (it's on white background). The ToggleList supposed to be a generic component,

but if we want to make it look nice we need specific styles based on where the menu trigger is located.

Per hangout conversation with @alexhollender we decided to leave it as it is (keep the default Android behavior).

Change 537179 abandoned by Pmiazga:
DoNotMerge: remove highliht for ToggleList trigger tap

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

pmiazga updated the task description. (Show Details)Sep 17 2019, 9:50 AM
pmiazga updated the task description. (Show Details)
Edtadros reassigned this task from Edtadros to ovasileva.Sep 25 2019, 2:03 PM

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX, Galaxy S5, iPad Pro, Google Pixel 2, Galaxy Note 3

Test Artifact(s):

Perform those steps as AMC user on the beta cluster

✅ AC1: Verify that User Menu trigger (user icon in the header) has a darker background when the user menu is visible

✅ AC2: Verify that Overflow Menu trigger (three dots icon in the toolbar) has a darker background when the overflow menu is visible

Edtadros updated the task description. (Show Details)Sep 25 2019, 2:03 PM
ovasileva closed this task as Resolved.Sep 26 2019, 4:35 PM

Looks good!