Change 596533 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [dev][Legacy] split sidebar Mustache and Less

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

Chatted with Alex. Here are the initial interests for a button: grey hover, blue focus, no checked state.

Assuming https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/584694/ and the core patch are the only changes to the only changes left to review, I've left some comments.

Here are the initial interests for a button: grey hover, blue focus, no checked state.

Will be new patch. Not done yet.

All other feedback addressed. Thanks, @Jdlrobson and @Volker_E!

Here are the initial interests for a button: grey hover, blue focus, no checked state.

Not done yet.

All other feedback addressed.

Change 597312 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [button] add hover state to menu button

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

@alexhollender, latest is now on labs. Patch is pending your approval.

In T246419#6150323, @Niedzielski wrote:

@alexhollender, latest is now on labs. Patch is pending your approval.

My apologies for not bringing this up when we chatted the other day but the icons are rendering too large. Currently they are 24x24px but should be 20x20px. Also the hover area doesn't quite match what we have in Minerva (highlight color and shape).

Minerva	staging

@alexhollender all icons in desktop are 24x24 as we never upstreamed the changes we made for Minerva to Vector. Doing that is a little risky as it impacts everything using icons in Vector. Let's check in with @Volker_E but I suggest tackling the 24x24 > 20x20 icon change as a separate patchset.

In T246419#6152861, @Jdlrobson wrote:

@alexhollender all icons in desktop are 24x24 as we never upstreamed the changes we made for Minerva to Vector. Doing that is a little risky as it impacts everything using icons in Vector. Let's check in with @Volker_E but I suggest tackling the 24x24 > 20x20 icon change as a separate patchset.

I'm seeing various sizes, including 20x20px, in Vector

In T246419#6152946, @alexhollender wrote:

In T246419#6152861, @Jdlrobson wrote:

@alexhollender all icons in desktop are 24x24 as we never upstreamed the changes we made for Minerva to Vector. Doing that is a little risky as it impacts everything using icons in Vector. Let's check in with @Volker_E but I suggest tackling the 24x24 > 20x20 icon change as a separate patchset.

I'm seeing various sizes, including 20x20px, in Vector

Sorry I should have qualified that better - OOUI icons and custom icons are not impacted, however icons with the mw-ui-icon class are 24x24. We can't use ooui here (too much of a performance penalty) and we're trying to be good mediawiki citizens by not adding to technical debt by making yet another custom icon.

In T246419#6153009, @Jdlrobson wrote:

Sorry I should have qualified that better - OOUI icons and custom icons are not impacted, however icons with the mw-ui-icon class are 24x24. We can't use ooui here (too much of a performance penalty) and we're trying to be good mediawiki citizens by not adding to technical debt by making yet another custom icon.

I see. Can you show or tell me where those 24x24px icons are in Vector? I'm not able to find them.

Thankfully doesn't look like too many of the icons are inconsistent, given low usage of this library outside mobile. Looks like the following are inconsistent.

Hover in Popups:

https://en.wikipedia.org/wiki/File:Flag_of_Spain.svg

Change 589692 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.page.ready: add checkbox hack JavaScript

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

Collapse icon with lowered precision, yet perfect rendering:

<?xml version="1.0" encoding="UTF-8"?>
<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 20 20">
	<title>
		collapse
	</title>
	<path d="M9 2l1.3 1.3L3.7 10l6.6 6.7L9 18l-8-8 8-8zm8.5 0L19 3.3 12.2 10l6.7 6.7-1.4 1.3-8-8 8-8z"/>
</svg>

Change 597312 abandoned by Niedzielski:
[button] add hover state to menu button

Reason:
Folded into preceding commit Ic9d54de7e19ef8d5dfd703d95a45b78c0aaf791a

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

Latest is up on Labs.

@Niedzielski wow, looking awesome! Nice job with the spacing : ) Where are you getting the background color for the hover state from? In Minerva it looks like we're using background-color: rgba(0,0,0,0.03);, which looks good to me.

background-color: rgba(0,0,0,0.03);

This is from OOUI / wikimedia-ui-base, @background-color-frameless--hover / @wmui-color-base90 and is rgba( 0, 24, 73, 7/255 ).

Latest is up on Labs.

Change 584694 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [feature] add menu button to toggle panel visibility

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

This is the status quo in positioning:

Change 599151 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Vector@master] Vertically align menu icon with sidebar contents

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

With updated patch:

Change 599151 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Horizontally and vertically align menu icon to design templates

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

Change 599431 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [fix][Less] disable sidebar animations on page load

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

Last merged patch https://gerrit.wikimedia.org/r/599151 does not compile:
variable @margin-top-header is undefined in file .../Sidebar.less on line 24, column 8

http://patchdemo.wmflabs.org/wikis/9ef4f38b8e25402c3e47e15f8fdcfee1/w/load.php?lang=en&modules=ext.flaggedRevs.basic%2Cicons%7Cext.visualEditor.desktopArticleTarget.noscript%7Cmediawiki.ui.icon%7Cmediawiki.widgets.styles%7Coojs-ui-core.icons%2Cstyles%7Coojs-ui.styles.indicators%7Cskins.vector.icons%2Cstyles&>

The Beta-Cluster has the patch and I think it's working ok?

Sorry, my bad. Went missing in an unrelated merge.

@margin-top-header comes from @import './layout.less'; – we make a difference in these two paths, comparing @import 'checkboxHack.less'; We use @margin-top-header and @height-logo-icon from Sidebar.less

@Volker_E Yes, I've renamed it to @padding-top-header in the personal menu vertical alignment patch, that better suits theming (user styles), which issue I've explained in great length. Different file (Sidebar.less) was changed, so there was no hint of the inconsistency and I didn't notice in the diff.

@Niedzielski for accessibility reasons a few users set the browser font-size to big values, like 150% or even 200% of the default 16px. For them the sidebar icon looks like this:

@Niedzielski - following up on the defaults - so currently, the default is sidebar off for both logged-in and logged-out users. We'd want it to be default on, as per the acceptance criteria (with the possibility for different defaults when logged-in/logged-out). Maybe we should pull this out into a separate task?

Change 599938 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [fix][a11y][Less] use pixel dimensions for sidebar button

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

Change 599939 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [config] Change the sidebar's default state to open

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

Thanks for updating the AC. Just to make sure I got this right, on = visible, off = hidden?

Legacy mode:

• No change

Latest mode:

• INITIAL STATE: sidebar is default visible for all users
• EVENTUAL STATE: sidebar is default hidden for anons, default visible for all other users

If so, I need to quickly flip the default state. Patch is here.

(with the possibility for different defaults when logged-in/logged-out). Maybe we should pull this out into a separate task?

Yes, please.

For the broader Legacy vs Latest mode, I'm aware of this detail on a per wiki basis:

• Logged in
  • New account initialization state (saved as a preference, once it is set for a given account, we can't unset it)
  • Existing account defaults (not saved as a preference, just a default at runtime)
• Anonymous

Additionally:

• No support built in for bucketing users into different default states.
• If a user sets a global preference, I believe it will have effect on all wikis whether the experience is intended to be deployed there or not.
• The presentation of the user preference for skin version can be controlled per wiki independently of the user's preference state.

For the collapsible sidebar specifically:

• If the default skin is vector and the default skin version is Latest on a given wiki, the user preference for skin version doesn't matter but the collapsible sidebar state still does depending on the persistence approach taken.
• Collapsible sidebar only works in Latest mode. If the default state for a wiki isn't Latest, the sidebar is unavailable for anons regardless of the persistence approach taken.
• The sidebar impacts page content width behavior (T246420) depending on screen size but screen size never impacts sidebar state.
• I think everything else is currently accounted for in @Jdrewniak's deck.

Is any of this surprising or incorrect?

Other work:

• There's a patch to fix a known animation bug in Chrome we had hoped wouldn't occur but Nick, Volker, and Edward can all repro.
• The icon sizing is incorrect for different browser font sizes (T246419#6175184). I've pushed a patch to use pixel values instead. There's history here so I'll need @Volker_E or @Jdrewniak to advise.

Have replied on patch, we want to ensure that icons are user-scalable. Have to provide further guidance on MediaWiki and the Design Style Guide, but it's a pretty clear guidance when to rely on ems and when not – everything that leads to recognizability should be scalable, distances etc. can be set in px.

With the patch overhauled the result

Thank you, @Volker_E and @Demian. I didn't notice it before but I think the dropdown mehr arrow should be larger too.

This is outside of the current ticket but I just want to say that I think describing what not to do and why can be invaluable context, especially given the status quo. I don't know if you agree but I think it could be added as supplemental information to the Design Style Guide.

Change 600012 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Vector@master] Fix misconception on icon coloring

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

Change 599938 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [fix][a11y][Less] use percentage for sidebar button icon size

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

Change 599939 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [config] Change the sidebar's default state to open

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

Test Result - Beta

Status: ⬜ In Progress
*Environment: beta
OS: macOS Catalina
Browser: Chrome, Safari, Firefox
Device: MBP
Emulated Device:** NA

Test Artifact(s):

QA steps

Execute the tests for each of the following scenarios

• a variety of screen sizes: Screen size didn't appear to have an effect.
• a variety of browsers: Tested on Safari, Chrome, and Firefox
• at least one rtl language
• the history page
• the recent changes page
• an article page

AC1: For all users, sidebar state should persist across pageviews (i.e. if I go from one article/page to another, in the same browser tab)
✅ Screen size didn't appear to have an effect.
✅ Tested on Safari, Chrome, and Firefox with no noticeable difference.
❌ Tested on Hebrew. RTL failed when Sidebar was collapsed.

AC2: For logged-in users sidebar state should also persist across sessions (i.e. if I close the browser tab, and then open a new one)
In progress

AC3: For logged-out users every session should start with the sidebar closed
In progress

Change 601548 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [fix][RTL] flip menu collapse button icon

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

❌ Tested on Hebrew. RTL failed when Sidebar was collapsed.

That was a big bug on my part. Thank you for catching this before it became an issue I've proposed an RTL icon collapse icon fix in this patch.

❌ The History page did not work for when the Sidebar was collapsed

I was able to reproduce this page on Dog. It was a caching issue caused by the recent flip of the default sidebar state (see T246419#6177744). I resolved it by purging https://en.wikipedia.beta.wmflabs.org/wiki/Dog?action=purge. Can you try once more, please? If you see it on another page, you may need to purge it too. It is a very good reminder that we don't want to change the default state in prod for anons. Persistence is covered by T246427.

❌ The recent changes page

Is this the persistence issue you're noting? That's T246427.

AC2: For logged-in users sidebar state should also persist across sessions (i.e. if I close the browser tab, and then open a new one)

That's T246427 and not part of this task AFAIK.

AC3: For logged-out users every session should start with the sidebar closed

I think this is for a future task? The task also says "INITIAL STATE: sidebar is default visible for all users".

Thank you, @Edward!

@Niedzielski regarding your note on the History page above I tried the following (all as anon) on the Hooded Skunk page.

Navigating with the sidebar expanded without purging.

• The sidebar collapses (as you expected)
• The sidebar expands when I view history

Navigating with the sidebar expanded and with purging.

• sidebar stays expanded when I view the article
• sidebar stays expanded with I view History

Navigating with the sidebar collapsed and with purging.

• sidebar gets expanded at the purge prompt
• sidebar stays expanded when I view the article
• sidebar stays expanded with I view History

As far as AC2 and AC3, I went based on the task description. @ovasileva thoughts?

As far as AC2 and AC3, I went based on the task description. @ovasileva thoughts?

What Stephen said - there's a note for this in the description, but reading it again, I can see that it's a bit confusing. I think we're good for now. Thank you!

@Niedzielski - sorry for the late reply - yes, this is all correct - details/thoughts per line below. TLDR: the only use case we want to account for is logged-in/logged-out at this time with the caveat for new accounts that we can use later on if necessary.

In T246419#6177744, @Niedzielski wrote:

Thanks for updating the AC. Just to make sure I got this right, on = visible, off = hidden?

Legacy mode:

• No change

Latest mode:

• INITIAL STATE: sidebar is default visible for all users
• EVENTUAL STATE: sidebar is default hidden for anons, default visible for all other users

If so, I need to quickly flip the default state. Patch is here.

Yup - just needs a flip. This was actually always in the acceptance criteria, but I see that it might have been a bit unclear - I bolded it to make it more apparent and added the additional comment ;)

(with the possibility for different defaults when logged-in/logged-out). Maybe we should pull this out into a separate task?

Yes, please.

Will do.

For the broader Legacy vs Latest mode, I'm aware of this detail on a per wiki basis:

• Logged in
  • New account initialization state (saved as a preference, once it is set for a given account, we can't unset it)
  • Existing account defaults (not saved as a preference, just a default at runtime)
• Anonymous

Additionally:

• No support built in for bucketing users into different default states.
• If a user sets a global preference, I believe it will have effect on all wikis whether the experience is intended to be deployed there or not.

This is expected, but leaving a note that perhaps we should add to QA to verify that it is happening.

• The presentation of the user preference for skin version can be controlled per wiki independently of the user's preference state.

Yup. Noting that this is a bit confusing because theoretically you have a setting on every wiki but it actually sets it globally, but it's not a major concern.

For the collapsible sidebar specifically:

• If the default skin is vector and the default skin version is Latest on a given wiki, the user preference for skin version doesn't matter but the collapsible sidebar state still does depending on the persistence approach taken.
• Collapsible sidebar only works in Latest mode. If the default state for a wiki isn't Latest, the sidebar is unavailable for anons regardless of the persistence approach taken.
• The sidebar impacts page content width behavior (T246420) depending on screen size but screen size never impacts sidebar state.
• I think everything else is currently accounted for in @Jdrewniak's deck.

Is any of this surprising or incorrect?

Correct on all counts :)

@Niedzielski - here's the task for varying the default state: T254230: Sidebar should allow for different states for logged-in and anonymous users

Change 600012 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Fix misconception on icon coloring

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

Change 602192 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [fix][dev] use "skin" module for JavaScript, not "Vector"

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

@ovasileva is the testing in T246419#6183825 and T246419#6183578 sufficient for Beta? If so I'll document and test in a prod wiki.

Change 601548 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [fix][RTL] flip menu collapse button icon

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

In T246419#6191182, @Edtadros wrote:

@ovasileva is the testing in T246419#6183825 and T246419#6183578 sufficient for Beta? If so I'll document and test in a prod wiki.

Yup, sounds good.

So far as I know, this is the last patch needed for this task. The patch works around a six year old Chrome bug wherein the drawer flickers on every pageview (although I think this only happens when the default state is closed so it may not currently be visible on the Beta-Cluster).

@ovasileva, this patch has had thoughtful feedback from @nray, @Jdrewniak, @Volker_E, and @Jdlrobson each which so far as I know boils the options down to:

1. Merge this patch and commit to prioritizing the work identified in T254399.
2. Abandon the patch and accept the flicker issue.
3. Postpone the patch and start on T254399.

@Edtadros, please retest the RTL bug. E.g.: https://he.wikipedia.org/wiki/%D7%9E%D7%99%D7%A9%D7%9C_%D7%90%D7%95%D7%91%D7%9E%D7%94?useskinversion=2&uselang=he. According to https://versions.toolforge.org/, it will be available on hewiki on Wednesday.

In T246419#6194510, @Niedzielski wrote:

So far as I know, this is the last patch needed for this task. The patch works around a six year old Chrome bug wherein the drawer flickers on every pageview (although I think this only happens when the default state is closed so it may not currently be visible on the Beta-Cluster).

@ovasileva, this patch has had thoughtful feedback from @nray, @Jdrewniak, @Volker_E, and @Jdlrobson each which so far as I know boils the options down to:

1. Merge this patch and commit to prioritizing the work identified in T254399.

Since we already have the patch and the follow-up task created, it seems this is the most reasonable way forward that also gives us an intermediate fix

Change 599431 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [fix][Less] disable sidebar animations on page load

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

@Edtadros, remaining QA work so far as I know is:

• Validate RTL support in production: https://he.wikipedia.org/wiki/%D7%9E%D7%99%D7%A9%D7%9C_%D7%90%D7%95%D7%91%D7%9E%D7%94?useskinversion=2&uselang=he.
• Test for the Chrome flicker bug discussed in chat. You shouldn't see the menu flicker in on page reload.

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome, Firefox, Safari
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

✅ AC1: a variety of screen sizes: See T246419#6183578
✅ AC2: a variety of browsers: See T246419#6183578
✅ AC3: at least one rtl language

Test Result - Prod

Status: ✅ PASS
Environment: enwiki/hewiki
OS: macOS Catalina
Browser: Chrome, Firefox, Safari
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

✅ AC1: a variety of screen sizes
✅ AC2: a variety of browsers: Verified on Chrome, Safari, and Firefox
✅ AC3: at least one rtl language

Change 602192 abandoned by Jdlrobson:
[fix][dev] use "skin" module for JavaScript, not "Vector"

Reason:
I would prefer to go down the route of https://gerrit.wikimedia.org/r/#/c/mediawiki/core/ /605641 - if that's merged I'll do the required follow up in Vector.

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