Make WordPress Core

Opened 5 years ago

Last modified 2 months ago

#46976 new defect (bug)

Twenty Nineteen: remove postcss-focus-within

Reported by: afercia's profile afercia Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.0
Component: Bundled Theme Keywords: needs-testing has-patch
Focuses: Cc:

Description

Maybe I'm missing something but seems to me postcss-focus-within added in https://github.com/WordPress/twentynineteen/pull/392 doesn't do anything.

It's supposed to be used together with its companion focus-within JS polyfill which is not used in the theme.

It is supposed to work this way:

  • postcss-focus-within processes the CSS and duplicates all the :focus-within rules with rules that use a [focus-within] attribute selector
  • focus-within adds the [focus-within] data-attribute in the DOM

As Twenty Nineteen doesn't use focus-within, there are no [focus-within] data-attributes in the DOM.

However, all the related CSS rules are in the stylesheet and appear to be unnecessary.

Worth noting that to make the nav menu work in IE 11, a custom solution has been implemented in touch-keyboard-navigation.js which uses an is-focused CSS class.

Also, ideally the is-focused CSS class should be added in the DOM only for browsers that don't support :focus-within.

Attachments (3)

46976-for-testing-only.patch (280.7 KB) - added by poena 2 months ago.
For testing only, includes the built CSS files.
46976-2.patch (272.5 KB) - added by poena 2 months ago.
Removes the postcss-focus-within dependency, without updated, built, CSS files
46976-3.patch (272.8 KB) - added by poena 2 months ago.
Removes the postcss-focus-within dependency and updates the RTL CSS configuration. Does not include updated CSS files.

Download all attachments as: .zip

Change History (9)

#1 @kjellr
5 years ago

cc @allancole for a gut check on whether this is necessary or not.

#2 @karmatosed
3 months ago

I wanted to check back on this as it has sat for a little while. @afercia do you still feel the same about this? If that is the case I would recommend testing and a patch.

#3 @afercia
3 months ago

postcss-focus-within is still in the dev dependencies and to my understanding it doesn't do anything. Cc @poena

#4 @karmatosed
3 months ago

  • Keywords needs-testing needs-patch added

#5 @poena
2 months ago

I have tried to solve this but ran into something unexpected.

When I remove postcss-focus-within from the dependencies in package.json (including letting it create a new lock file) as well as remove it from the postcss.config.js config file, there are unexpected CSS changes when I build the CSS.

Expected result:
The CSS blocks that uses [focus-within] are removed from style.css, example:

.main-navigation .main-menu .menu-item-has-children:not(.off-canvas)[focus-within] > .sub-menu {

Actual result:
The CSS blocks that uses [focus-within] are removed from style.css, example:

.main-navigation .main-menu .menu-item-has-children:not(.off-canvas)[focus-within] > .sub-menu {

Other CSS changes related to browsers specific styles are also applied:
This CSS is deleted:

::-moz-selection {
  background-color: #bfdcea;
}

This webkit CSS is added:

.post-navigation .nav-links a .meta-nav {
  color: #767676;
  user-select: none;
  -webkit-user-select: none; // THIS is added
          user-select: none; // THIS is added
}

I think this is because in the postcss.config.js, focus-within was used with disablePolyfillReadyClass

From the post-csss-focus-within package documentation:

disablePolyfillReadyClass
The disablePolyfillReadyClass option determines if selectors are prefixed with an indicator class.
This class is only set on your document if the polyfill loads and is needed.

By default this option is false. Set this to true to prevent the class from being added.

https://www.npmjs.com/package/postcss-focus-within

So maybe these additional CSS changes are a non issue?

Last edited 2 months ago by poena (previous) (diff)

@poena
2 months ago

For testing only, includes the built CSS files.

@poena
2 months ago

Removes the postcss-focus-within dependency, without updated, built, CSS files

@poena
2 months ago

Removes the postcss-focus-within dependency and updates the RTL CSS configuration. Does not include updated CSS files.

#6 @poena
2 months ago

  • Keywords has-patch added; needs-patch removed

Testing instructions:

46976-for-testing-only.patch

  • Apply the patch. Review the CSS changes. Confirm that the navigation in the theme works.

46976-2.patch

  • Please ignore this patch, it has an issue with the rtl stylesheet.

46976-3.patch

  • Apply the patch.

In the terminal, run these commands to install the dependencies and build the CSS changes:

npm install
npm run build

Confirm that there is no presence of [focus-within] in the CSS files.
Confirm that the navigation in the theme works.

Note: See TracTickets for help on using tickets.