Page MenuHomePhabricator

styles: Get rid of some `:not()` selectors in favor of default styles
Closed, ResolvedPublic3 Estimated Story Points

Description

Problem

Throughout Codex, we have followed a pattern when writing styles for cases where there are different styles for different variants, e.g.:

  • enabled/disabled
  • default/error status
  • different types where there is a default (button action, button weight, message type, etc.)
  • different layouts (e.g. block/inline, framed/quiet)

There are two components to this pattern:

  1. We do not write styles for the default state and then override them with more specific selectors, e.g. setting styles for enabled buttons with the .cdx-button selector and then overriding those styles for .cdx-button:disabled. Instead, we set enabled styles on .cdx-button:enabled and disabled styles on .cdx-button:disabled. The original intent of this pattern was to make styles more readable by clearly separating mutually exclusive styles, and to ease maintainability by avoiding unexpected specificity or cascade issues.
  2. When there is a "default" state defined by a CSS class (i.e. not a native attribute), we do not actually use that class to set the default styles. Instead, we use a :not() selector, or a series of :not() selectors. We do this to make it easier to use CSS-only components: users do not have to include the default class, e.g. cdx-button--weight-normal or cdx-message--block. If we did require users to do this, they may forget and they will not get most of the styles needed for a given component.

This pattern works well in terms of code readability and maintainability, and easing the burden on CSS-only component users. However, there are 2 issues:

  1. The mutually exclusive pattern was new to some of us working on Codex—some developers are used to writing default styles on the root selector and then overriding those styles with a more specific selector.
  2. The :not() selectors are more specific than a simple class selector, especially when we end up with chains of them, e.g. &:not( .cdx-message--warning ):not( .cdx-message--error ):not( .cdx-message--success ) in place of &.cdx-message--notice.

The second issue has already caused bugs, where a style was unexpectedly overridden by one within a block of chained :not() selectors. It also caused great difficulty when updating the Tabs styles, which are already complex and now contain a significant amount of unwanted specificity.

Solution

The DST engineers discussed this and decided that:

  • We will continue to use the mutually exclusive styles pattern for things that are binary (enabled/disabled, block/inline)
  • We will not use that pattern for things with multiple values (button action and weight, message type). Instead, we will write default styles on the root selector and override them with more specific selectors.

:not() selector inventory

ComponentPurposeProposed action
Accordion:focus but not :focus-visibleKeep as is; in the future evaluate whether/how we want to use :focus-visible
ButtonIcons in non-icon-only fake buttonsKeep as is
ButtonNot applying :focus styles to :activeKeep as is
ButtonNon-quiet buttonsRemove :not(), make these styles default and make quiet buttons override them
ButtonNormal enabled buttons (not primary, not quiet, not disabled)Replace primary/quiet :not() with default+override ; keep disabled :not()
CheckboxNon-indeterminate checked stateKeep as is
CheckboxNot applying :focus styles to :active or :hoverKeep as is (maybe consider reordering styles?)
FieldNot disabledKeep as is
LabelNot disabledKeep as is
LabelNot visually hiddenKeep as is
MenuNot applying :last-of-type styles to an only childKeep as is
MessageInline as default (not block)Keep as is
MessageNotice as default (not warning, error or success)Consider replacing :not() with default + override (may be difficult) or requiring explicit class
ProgressBarBlock as default (not inline)Keep as is
ProgressBarNot disabledKeep as is
TabsNon-disabled tabKeep as is
TabsQuiet as default (not framed)Keep as is
TextAreaNot applying readonly styling when disabledKeep as is
ToggleSwitchNon-emptyKeep as is
ToggleSwitchNot applying :focus styles to :activeKeep as is
TypeaheadSearchNot expanded as defaultKeep as is
TypeaheadSearchNot auto-expanding by defaultKeep as is

Acceptance criteria

  • All instances where :not() selectors are used for non-binary states are identified and updated to set the default styles on the root selector and override those styles with more specific selectors
  • Testing is done to ensure there are no visual regressions caused by these changes

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

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

[design/codex@main] styles, Button, Message: Remove some `:not()` selectors

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

AnneT changed the task status from Open to In Progress.Jun 22 2023, 7:37 PM

Change 932326 merged by jenkins-bot:

[design/codex@main] styles, Button, Message: Remove some `:not()` selectors

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

Skipping design sign-off since this didn't include any design changes

Change 935794 had a related patch set uploaded (by Catrope; author: Catrope):

[mediawiki/core@master] Update Codex from v0.13.0 to v0.14.0

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

Test wiki created on Patch demo by ATomasevich (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/64dc3126c0/w

Change 935794 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.13.0 to v0.14.0

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

Test wiki on Patch demo by ATomasevich (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/64dc3126c0/w/