Make WordPress Core

Opened 7 years ago

Last modified 3 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: has-patch has-test-info
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 years ago.
For testing only, includes the built CSS files.
46976-2.patch (272.5 KB) - added by poena 2 years ago.
Removes the postcss-focus-within dependency, without updated, built, CSS files
46976-3.patch (272.8 KB) - added by poena 2 years 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 (10)

#1 @kjellr
7 years ago

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

#2 @karmatosed
2 years 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
2 years ago

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

#4 @karmatosed
2 years ago

  • Keywords needs-testing needs-patch added

#5 @poena
2 years 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 years ago by poena (previous) (diff)

@poena
2 years ago

For testing only, includes the built CSS files.

@poena
2 years ago

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

@poena
2 years ago

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

#6 @poena
2 years 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.

#7 @ozgursar
3 months ago

  • Keywords has-test-info added; needs-testing removed

Patch Testing Report

Patch Tested1: https://core.trac.wordpress.org/attachment/ticket/46976/46976-for-testing-only.patch

Patch Tested3: https://core.trac.wordpress.org/attachment/ticket/46976/46976-3.patch

Environment

  • WordPress: 7.0-alpha-61215-src
  • PHP: 8.2.29
  • Server: nginx/1.29.4
  • Database: mysqli (Server: 8.4.7 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 144.0.0.0
  • OS: macOS
  • Theme: Twenty Nineteen 3.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.1

Steps taken

  1. Install and activate Twenty Nineteen theme
  2. Add menu and submenu items
  3. Check menu items' CSS
  4. Confirm presence of [focus-within] styles (see screenshot - before patch)
  5. Apply patch 46976-for-testing-only
  6. Check menu functionality. Menu and submenu works as expected.
  7. Check CSS. Confirm that [focus-within] are replaced with :focus-within
  8. 46976-3.patch
  9. Run npm install and npm run build`
  10. Check menu functionality. Menu and submenu works as expected.
  11. Check CSS. Confirm no presence of [focus-within] in the CSS.
  12. ✅ Patch is solving the problem

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 {}

Screenshots/Screencast with results

Before applying patch:
https://i.imgur.com/I6pUnx2.png

After applying 46976-for-testing-only.patch:
https://i.imgur.com/J79G0oo.png

After applying 46976-3.patch
https://i.imgur.com/5WIKwax.png

Note: See TracTickets for help on using tickets.