Opened 7 years ago
Last modified 3 months ago
#46976 new defect (bug)
Twenty Nineteen: remove postcss-focus-within
| Reported by: |
|
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-withinprocesses the CSS and duplicates all the:focus-withinrules with rules that use a[focus-within]attribute selectorfocus-withinadds 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)
Change History (10)
#2
@
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
@
2 years ago
postcss-focus-within is still in the dev dependencies and to my understanding it doesn't do anything. Cc @poena
#5
@
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?
@
2 years ago
Removes the postcss-focus-within dependency and updates the RTL CSS configuration. Does not include updated CSS files.
#6
@
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
@
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
- Install and activate Twenty Nineteen theme
- Add menu and submenu items
- Check menu items' CSS
- Confirm presence of [focus-within] styles (see screenshot - before patch)
- Apply patch 46976-for-testing-only
- Check menu functionality. Menu and submenu works as expected.
- Check CSS. Confirm that [focus-within] are replaced with :focus-within
- 46976-3.patch
- Run
npm installandnpm run build` - Check menu functionality. Menu and submenu works as expected.
- Check CSS. Confirm no presence of
[focus-within]in the CSS. - ✅ 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 {}



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