Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSS @layer bundling misses selectors inside media queries #9368

Closed
7 tasks done
argyleink opened this issue Jul 25, 2022 · 10 comments · Fixed by #9929
Closed
7 tasks done

CSS @layer bundling misses selectors inside media queries #9368

argyleink opened this issue Jul 25, 2022 · 10 comments · Fixed by #9929
Labels
bug: upstream Bug in a dependency of Vite feat: css

Comments

@argyleink
Copy link

Describe the bug

@import 'layers.css' layer(demo);

where layers.css contains:

h1 {
  color: red;
}

@media (min-width: 100px) {
  h1 {
    color: green;
  }
}

incorrectly produces:

@layer demo {
  h1 {
    color: red;
  }
}

@media (min-width: 100px) {
  h1 {
    color: green;
  }
}

instead of (this is just one correct way):

@layer demo {
  h1 {
    color: red;
  }

  @media (min-width: 100px) {
    h1 {
      color: green;
    }
  }
}

This means any media queries are unlayered, giving them the top most precedence and specificity over styles in layers.

Vanilla Vite example: https://stackblitz.com/edit/vitejs-vite-vlemhc?file=layers.css,style.css
Demo: https://vitejs-vite-vlemhc--5173.local.webcontainer.io/

I'd like a way to disable it for now, but don't see one.

Reproduction

https://stackblitz.com/edit/vitejs-vite-vlemhc?file=layers.css,style.css

System Info

System:
    OS: macOS 12.4
    CPU: (16) x64 Intel(R) Xeon(R) W-2140B CPU @ 3.20GHz
    Memory: 799.40 MB / 64.00 GB
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 18.2.0 - /usr/local/bin/node
    npm: 8.9.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 103.0.5060.134
    Chrome Canary: 106.0.5201.0
    Firefox: 102.0.1
    Firefox Developer Edition: 102.0
    Firefox Nightly: 103.0a1
    Safari: 15.5
    Safari Technology Preview: 15.4
  npmPackages:
    vite: 3.0.2 => 3.0.2

Used Package Manager

npm

Logs

No response

Validations

argyleink added a commit to argyleink/gui-challenges that referenced this issue Jul 25, 2022
argyleink added a commit to argyleink/gui-challenges that referenced this issue Jul 25, 2022
* initial demo port

* reduced motion updates, config updates

* update open props to latest

* add product images

* fixes page scrolling

* use the prop

* nits and notes

* more notes

* adds ability to spam next/prev

* task note

* finish accessible content

* bit more prevention for heavy spamming of next/prev

* fixes scroll event keypress bubbling by making carousel focusable

* adds snap scroll item wrapper for non-janky transforms

* demo style updates for multiple carousels

* switch from aria-disabled to disabled, use keyframes for keypress animation, adds content visibility

* adds hover effect to pagination dots

* fixes event bubbling in minimap so items arent accidentally selected

* better button hover states

* removes viewport units

* relative layout

* better layouts for when pagination or controls are set to none

* hide arrows if user has js turned off

* no scrollbar option

* fit nits

* todo updates

* fixes tiny buttons on iOS

* best of both worlds: browser event source is correct and container is no longer focusable

* adds posinset and setsize on the tabs

* update to latest open props

* make defaults more obvious they are an api

* clean up some notes

* fixes scroller gap being slightly shy of the fit needed

* fixes firefox next/prev buttons not snapping to next with scrollBy

* converted to a class

* adds unlisteners

* add more demos

* note a todo for el removal observation and unlistening

* fixes content-visibility affecting document body scroll

* updates todos

* adds aria-label with position of item via js to unburden devs

* handles lots of pagination items and keeps active in view

* hide scrollbars on pagination too if config is set

* better place to scroll dots into view

* add more todos

* tabindex -1 doesnt need this fix

* page load no longer scrolls to the carousel

* add another note

* fixes scroll regression after pagination scroll was added

* use a row size so no layout shift

* pagination hides scrollbars by default

* fixes safari not scrolling items into view

* adds scroll-hint to pagination area for gallery

* add pagination and controls with JS

* js setting as much aria as possible

* clean up initialization

* fixes wide screens

* notes

* support scroll snap always config option

* pagination looks for figcaption text or img alt for labels

* move item styles to external stylesheet

* adds testimonial demo

* fixes horizontal overflow bug when reaching end of pagination

* minor pagination style improvements

* fixes firefox not supporting scrollIntoViewIfNeeded()

* removes duplicate shirt

* demo title change

* adds 1 at a time demo

* first demo has free scroll

* move custom layouts to custom stylesheet

* adds full width demo

* add cascade layers

* add scroll hint demo

* wraps basics in where for easy overrides

* removes some todos

* adds paged collection demo

* responsive paged collection demo

* fixes production build by updating to latest vite and postcss-preset-env and passing custom media to plugin

* fixes full width on large monitors

* adds node removed observer to remove listeners and stuff

* adds touch-action to horizontal scroll areas

* more postcss transform cleanup

* tiny update to support RTL

* add todo

* finishes rtl keyboard support

* minor refactor of keypress animation handling

* support setting scroll start

* fixes typo in scroll start logic

* style header elements in examples

* cleanliness refactor

* minor demo style/text nudge

* updates state initialization with scrollstart logic

* made 2 more methods private

* adds goToElement() method so rootscroller is never invoked

* remove touch-action

* improves forced colors mode pagination and controls

* fixes disabled button having subtle shadow

* switch to eager in-view class assignment

* improves interaction pace with in-view class animation

* cruft removal

* some renaming and added more comments

* easing adjustment to pagination marker

* removes concept of scroll-item

* better product examples

* remove contain-intrinsic-size, shouldnt have that many items

* perf fix with will-change (safari desktop benefited the most)

* fixes product details alignment

* removes guard since no longer using scrollIntoView

* updates comment about usage

* update open props

* fixes rtl regression

* more fixes for RTL

* adds hubs example

* slight improvement to hubs demo

* workaround for vite cascade layers import bug vitejs/vite#9368

* adds build workflow and readme entry
@romainmenke
Copy link

I suspect this will be fixed by : postcss/postcss-import#496
Haven't verified this but it seems to be the same issue and vite uses postcss-import.

@sapphi-red sapphi-red added bug: upstream Bug in a dependency of Vite feat: css and removed pending triage labels Jul 26, 2022
@dutchcelt
Copy link

Currently (v3.0.3), both in a <style> tag or in a linked CSS file the import rule:
@import url("https://unpkg.com/open-props/") layer(designsystem);
results in
@import url("https://unpkg.com/open-props/");

Basically importing without the layer.

@romainmenke
Copy link

A fix was released in postcss-import as v15.0.0

https://github.com/postcss/postcss-import/blob/master/CHANGELOG.md#1500--2022-08-30

@sapphi-red Does this require an update in vite before end users can use this updated version?

@sapphi-red sapphi-red linked a pull request Aug 31, 2022 that will close this issue
1 task
@sapphi-red
Copy link
Member

@romainmenke Thanks!
Yes, this requires an update in Vite.

@romainmenke
Copy link

@sapphi-red I don't know anything about Vite so maybe this doesn't make sense, but can end users easily pass plugin options to postcss-import or does Vite do all the configuration?

@sapphi-red
Copy link
Member

@romainmenke Vite does all the configuration and currently it does not allow any configuration from users. Also Vite is shipped with postcss-import bundled in. So users cannot use the new version until a new release is cut.

@romainmenke
Copy link

In that case it is important to set a new plugin option for anonymous cascade layers : nameLayer

postcss-import needs to desugar anonymous layers to uniquely named layers but it doesn't have enough information about how it is used to generate these unique names.

example from the tests : https://github.com/postcss/postcss-import/blob/master/test/layer.js#L67-L80

example from the postcss-preset-env site :

const crypto = require('crypto');
const cwd = process.cwd();

module.exports = ctx => {
	const isProd = ctx.env === 'production';

	return {
		map: !isProd,
		plugins: {
			'postcss-import': {
				nameLayer: hashLayerName,
			},
			'postcss-preset-env': {
				stage: 0,
				preserve: true,
				features: {
					'custom-properties': false,
					'custom-media-queries': {
						preserve: false,
					},
				},
			},
			'cssnano': isProd ? {
				preset: 'default',
			} : false,
		},
	};
};

function hashLayerName(index, rootFilename) {
	if (!rootFilename) {
		return `import-anon-layer-${index}`;
	}

	// A stable, deterministic and unique layer name:
	// - layer index
	// - relative rootFilename to current working directory
	return `import-anon-layer-${crypto
		.createHash('sha256')
		.update(`${index}-${rootFilename.split(cwd)[1]}`)
		.digest('hex')
		.slice(0, 12)}`;
}

@sapphi-red
Copy link
Member

@romainmenke I see. Thank you very much for the explanation!

@romainmenke
Copy link

@argyleink v3.1.0 was released today.
Would be awesome if you could try this version to verify that this is now fully fixed.

@argyleink
Copy link
Author

bug is fixed 👍🏻

@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug: upstream Bug in a dependency of Vite feat: css
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants