Page MenuHomePhabricator

Visual Editor overlays do not work in night theme
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
Color contrast isuses

Screenshot 2024-04-30 at 12.02.41 PM.png (1×3 px, 362 KB)

What should have happened instead?:
No color contrast issues

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):
Adding notheme to oo-ui-windowManager should fix this.

QA Results - Beta

ACStatusDetails
1T363861#9802574 is a pass per T363861#9807142

QA Results - Prod

ACStatusDetails
1T363861#9823094

Event Timeline

ovasileva set the point value for this task to 3.May 2 2024, 5:50 PM

Change #1028941 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/Vector@master] feature(WIP:vector): Adjust CSS properties for Visual Editor overlays

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

Change #1028942 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[VisualEditor/VisualEditor@master] fix(VisualEditor): Adjust VE overlays for dark mode

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

Change #1028941 abandoned by Mabualruz:

[mediawiki/skins/Vector@master] feature(vector): Enhance CSS for Visual Editor overlays

Reason:

replaced by https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/1028942

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

Earlier last week @Mabualruz and I explored different approaches to this problem:

  • adding the .notheme class to VisualEditor
  • adding the .ve-ui-overlay-global selector to the .notheme ruleset in Vector.

We realized that this application of the .notheme class is temporary by design and that adding it to multiple repos is a form of technical debt that should be avoided (because after OOUI gets support for CSS variables, we'll have to go back into all the different repos and remove the .notheme class).

That led us to explore more generalized (and contained) solutions that could target not just VE but all OOUI interfaces.
patchdemo here: https://patchdemo.wmflabs.org/wikis/d7552cee1b/w/index.php?title=Main_Page&veaction=edit
gerrit patch here: https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/1029674
We're going to pursue this idea of targeting generic OOUI selectors with the .notheme class in an upcoming ticket.

For the work described here, we'll test that approach by first adding the .oo-ui-window selector to the .notheme ruleset in Vector.

We realized that this application of the .notheme class is temporary by design and that adding it to multiple repos is a form of technical debt that should be avoided (because after OOUI gets support for CSS variables, we'll have to go back into all the different repos and remove the .notheme class).

I recommend steering away from the term "technical debt" - it means different things to different people and we don't have a shared understanding of what that means. Instead could you rephrase this concern more explicitly in terms of how it effects ability to ship code/build new productions?

Another perspective is that this is a a kind of "Exclude list"/audit that allows us to clearly identify UIs that are not supported by the night theme and mitigate risk when we switch OOUI to support a night theme by being able to test and fix them individually.

The challenge with a generalized solution is that when T363849 is fixed we may find ourselves limited in where we can remove the notheme class. Consider a scenario where VisualEditor overlays are now working but an unmaintained workflow in Echo, BetaFeatures and MultimediaViewer isn't - is that a scenario we want to find ourselves in? Would there not be benefits about being able to remove one notheme class at a time? Also what would your test plan be for the generalized solution given that this task is only about VisualEditor - if we are adding notheme class globally we should probably test every OOUI dialog to check we are not inadvertently breaking something else..?

Anyway, no strong opinions on this - just want to make sure we've documented this approach and have thought about the long term impacts on our work here.

Change #1031043 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/Vector@master] Override VE overlays in night-mode

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

I recommend steering away from the term "technical debt" - it means different things to different people and we don't have a shared understanding of what that means. Instead could you rephrase this concern more explicitly in terms of how it effects ability to ship code/build new productions?

Sure thing. Applying the .notheme class to many OOUI interface is a temporary measure that will have to be undone when T363849 is fixed. Going through repo by repo to remove these classes is considerably more work than removing a single selector, (or even removing them individually in a single file).

I would agree that the idea is kind of like an "exclude" list, the benefit would be to avoid making these temporary changes across many repos in favour of making them in a single place. The hope is that this will make removing these classes easier in the future.

The degree to which these classes should be generic is debatable, but it's something I'm keen to explore in a follow-up task. For the purposes of this task, the patch above sets up a general approach for doing this (with considerable comments) but only targets .ve-ui-overlay-global.

Change #1028942 abandoned by Jdlrobson:

[VisualEditor/VisualEditor@master] fix(VisualEditor): Adjust VE overlays for dark mode

Reason:

While this would probably be the most preferable solution, I had some trouble getting it working locally.

Given this is hopefully temporary, after talking with Jan we decided to go with https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/1031043

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

Change #1031043 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Override VE overlays in night-mode

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

Edtadros subscribed.

Test Result - Beta

Status: ❌ FAIL
Environment: patchdemo
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

Test Steps

Visit https://patchdemo.wmflabs.org/wikis/abfa33da2d/w/index.php?title=Dog&veaction=edit making sure VE loads.
Click insert and then template
❌ AC1: There should be no color contrast issues
Note: I needed to add the vectornightmode paramter to the url to see dark mode.

screenshot 325.png (1×1 px, 171 KB)

screenshot 324.png (1×1 px, 170 KB)

Jdlrobson updated the task description. (Show Details)

@Edtadros sorry for the confusion - the patchdemo demonstrates the issue. I've updated the URL to beta cluster - that should show the fix.

Change #1031466 had a related patch set uploaded (by Jdlrobson; author: Jdrewniak):

[mediawiki/skins/Vector@wmf/1.43.0-wmf.4] Override VE overlays in night-mode

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

Change #1031535 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/Vector@master] [Follow-up] Override VE overlays in night-mode

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

Change #1031541 had a related patch set uploaded (by Jdlrobson; author: Jdrewniak):

[mediawiki/skins/Vector@wmf/1.43.0-wmf.4] Override VE overlays in night-mode

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

Change #1031541 abandoned by Jdlrobson:

[mediawiki/skins/Vector@wmf/1.43.0-wmf.4] Override VE overlays in night-mode

Reason:

See https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/1031466

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

Change #1031535 merged by jenkins-bot:

[mediawiki/skins/Vector@master] [Follow-up] Override VE overlays in night-mode

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

Change #1031466 merged by jenkins-bot:

[mediawiki/skins/Vector@wmf/1.43.0-wmf.4] Override VE overlays in night-mode

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

Mentioned in SAL (#wikimedia-operations) [2024-05-14T20:42:01Z] <cjming@deploy1002> Started scap: Backport for [[gerrit:1031466|Override VE overlays in night-mode (T363861)]]

Mentioned in SAL (#wikimedia-operations) [2024-05-14T20:44:43Z] <cjming@deploy1002> cjming and jdlrobson: Backport for [[gerrit:1031466|Override VE overlays in night-mode (T363861)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Change #1031477 had a related patch set uploaded (by Jdlrobson; author: Jdrewniak):

[mediawiki/skins/Vector@wmf/1.43.0-wmf.5] [Follow-up] Override VE overlays in night-mode

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

Mentioned in SAL (#wikimedia-operations) [2024-05-14T21:00:46Z] <cjming@deploy1002> Finished scap: Backport for [[gerrit:1031466|Override VE overlays in night-mode (T363861)]] (duration: 18m 44s)

Change #1031477 merged by jenkins-bot:

[mediawiki/skins/Vector@wmf/1.43.0-wmf.5] [Follow-up] Override VE overlays in night-mode

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

Mentioned in SAL (#wikimedia-operations) [2024-05-15T14:48:27Z] <jsn@deploy1002> Started scap: Backport for [[gerrit:1031477|[Follow-up] Override VE overlays in night-mode (T363861)]], [[gerrit:1031479|Mark night mode as a valid beta feature (T363814)]], [[gerrit:1031478|Mark night mode as a valid beta feature (T363814)]]

Mentioned in SAL (#wikimedia-operations) [2024-05-15T14:51:08Z] <jsn@deploy1002> jsn and jdlrobson: Backport for [[gerrit:1031477|[Follow-up] Override VE overlays in night-mode (T363861)]], [[gerrit:1031479|Mark night mode as a valid beta feature (T363814)]], [[gerrit:1031478|Mark night mode as a valid beta feature (T363814)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-05-15T15:06:54Z] <jsn@deploy1002> Finished scap: Backport for [[gerrit:1031477|[Follow-up] Override VE overlays in night-mode (T363861)]], [[gerrit:1031479|Mark night mode as a valid beta feature (T363814)]], [[gerrit:1031478|Mark night mode as a valid beta feature (T363814)]] (duration: 18m 26s)

Test Result - Beta

Status: ❓Need More Info
Environment: beta
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

Test Steps

Visit https://en.wikipedia.beta.wmflabs.org/w/index.php?title=T352930 making sure VE loads.
Click insert and then template
❓ AC1: There should be no color contrast issues
@Jdlrobson, I am not sure if the disabled Insert and Add buttons are out of scope for this

screenshot 348.png (993×1 px, 246 KB)

@Edtadros this is a pass - these buttons have color contrast issues in the light mode theme as well.

Looks good, resolving!

Test Result - PROD

Status: ✅ PASS
Environment: PROD
OS: macOS Sonoma 14.5
Browser: Chrome 125
Device: MBA
Emulated Device:NA

Test Artifact(s):

Test Steps

Visit https://en.wikipedia.org/wiki/Lightsaber making sure VE loads.
Click insert and then template
✅ AC1: There should be no color contrast issues
As stated in T363861#9807142, Insert and Add are fine

2024-05-22_12-37-13.png (1×3 px, 346 KB)