Page MenuHomePhabricator

Regression: Border color of Vector action menu is black
Closed, ResolvedPublicBUG REPORT

Description

Since rSVECac069fbec122: [dev] Consolidate ResourceLoader LESS style files the border color of Vector action menu is black instead of gray.
Steps to Reproduce:

  • Load a page with skin Vector.
  • Expand the action menu.

Actual Results:

Expected Results:

QA

QA Results

ACStatusDetails
1T247537#5991301

Event Timeline

Fomafix created this task.Mar 12 2020, 4:25 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 12 2020, 4:25 PM
Niedzielski added a subscriber: ovasileva.

Thank you for identifying this @Fomafix and reporting! My apologies for the trouble.

Hey @ovasileva! It seems I've introduced a regression so I'm going to look at this now.

Change 579325 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [fix] [LESS] Move print style variables to query

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

Niedzielski removed Niedzielski as the assignee of this task.Mar 12 2020, 4:52 PM
ovasileva triaged this task as High priority.Mar 12 2020, 4:53 PM
Jdlrobson renamed this task from Border color of Vector action menu is black to Regression: Border color of Vector action menu is black.Mar 12 2020, 5:39 PM

Quick status update: this fix is ready to go but is blocked on further technical refactor discussion. As I understand it, there are a few options:

  • Merge the fix as it is. This maintains the status quo and eliminates the regression. This approach neither incurs nor eliminates technical debt.
  • Revert the change that introduced the fix. No problem with that approach. However, there may be conflicts.
  • Wait for resolution of the discussion and then implement the conclusion with the fix. No problem with this either.

I'm in tomorrow but out of office next week so I leave it to the team to decide.

Quick status update: this fix is ready to go but is blocked on further technical refactor discussion. As I understand it, there are a few options:

  • Merge the fix as it is. This maintains the status quo and eliminates the regression. This approach neither incurs nor eliminates technical debt.

I endorse the immediate fix. The 3rd resolution will add additional refactoring, thus risk and discussion. The immediate need is the fix, I think further technical debt reduction is best done in a follow-up patch.

Quick status update: this fix is ready to go but is blocked on further technical refactor discussion. As I understand it, there are a few options:

  • Merge the fix as it is. This maintains the status quo and eliminates the regression. This approach neither incurs nor eliminates technical debt.

+1 on this

Change 579603 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Don't use variables for values only ever used once

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

Jdlrobson lowered the priority of this task from High to Medium.Mar 13 2020, 5:59 PM
Jdlrobson added subscribers: Volker_E, Jdlrobson.

The issue is fixed so dropping to medium.
I am not 100% happy with the solution as I think this pattern can lead to further issues if it was intentional.

So to me the conversation with @Volker_E is super important and this remains blocked until it happens.
I've raised https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/579603 to capture the work I think needs to happen.

From team chat:

Jon Robson Yesterday at 11:35 AM
@volker_e a question about variables came up in code review with @stephen on https://gerrit.wikimedia.org/r/#/c/579325/1/resources/skins.vector.styles/print.less - my understanding is within a skin/repo we should never use the same variable name with different values more than once, right??
5 replies

Jon Robson  23 hours ago
It seems Vector uses @color-base: #222; in variables.less but then defines another variable for print styles with the same name but different value @color-base: #000;

Jon Robson  23 hours ago
My feeling is the latter should be called print-color-base or something different? the dangers of clobbering names with different values is scope can be ambiguous

Change 579325 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [fix] [LESS] Move print style variables to query

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

My take has been that print styles are never concatenated or merged into screen styles and assuming clear scope of variables for performance reasons.
If you think we can't assure that, we need to go on a different path. But I'm not sure if we're are going to fix the symptoms, not the cause.

Volker_E added a comment.EditedMar 16 2020, 7:15 PM

LESS compiler variable-scopes lies within @media rules. https://phabricator.wikimedia.org/rSVECac069fbec1222bcb196d3d33fe073b901748370f#change-N3Jq3vXFMwYg
I think that's sufficient. No print variable should be defined outside @media print.

What the other approach would lead to, is to redefine every single variable as -print variable as well, which seems counterproductive to me.

Let me be a bit clearer: One beauty of CSS (and LESS) variables is, that they can be contextually amended if necessary. If we define new ones, we're giving up this power.

As I understand you're saying:
@border-color-base within the @media print scope (pseudo-code: @media-print[@border-color-base]) will only apply for print media, thus it is enough distinction from the global @border-color-base which is inherited to @media screen.
These can be defined in variables.less by wrapping them into the @media print selector.

There's one benefit to this: the for print (darker) @border-color-base would be applied to normal (not print-specific) styles too in print if it was used (currently only in .menu which is hidden in print).

Let me be a bit clearer: One beauty of CSS (and LESS) variables is, that they can be contextually amended if necessary. If we define new ones, we're giving up this power.

Sure but now every time I use a LESS variable I now have to be aware of the scope it runs in and can't assume it has the value defined globally.

Up until now https://github.com/wikimedia/mediawiki-skins-Vector/blob/master/variables.less#L39 was a definition of global variables with the expectation they wouldn't be overriden. Now print.less is importing this file and then changing the value of one of them. Now if I wanted to use
an import statement print and import variables (as it currently our practice so that storybook can import them) what would I expect the value of base-color to be? The global or local value? If I wanted the local value I'd need to set it again...

A compromise might be prefixing these variable names with local/context so the reader knows that their values are dependent on other things and moving color-base out of the variables.less file.

Change 579603 abandoned by Jdlrobson:
Don't use variables for values only ever used once

Reason:
I talked with Volker some more and unable to convince him. I respect his preference here to keep as is so I'll leave the status quo.

That said I'm still not convinced about the pattern of clobbering variables with different values and I will bring it up again if I see it causing further problems in the codebase. Not a priority right now though.

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

Jdlrobson updated the task description. (Show Details)Mar 17 2020, 10:25 PM
Jdlrobson removed a project: Patch-For-Review.

Technical detail is not a hill I'm going to die on :) Let's QA the fix.

Edtadros reassigned this task from Edtadros to ovasileva.Mar 23 2020, 9:14 AM
Edtadros added a subscriber: Edtadros.

Test Result

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

Test Artifact(s):

QA

Visit https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page
Hover over more
✅ AC1: Confirm the border is not black (compare and contrast with production if this is not clear)

ProductionBeta
Edtadros updated the task description. (Show Details)Mar 23 2020, 9:15 AM
ovasileva closed this task as Resolved.Mar 23 2020, 10:11 AM

Looks good!