patternfly icon indicating copy to clipboard operation
patternfly copied to clipboard

Bug - core - Ambiguous url('../../assets...)

Open rp- opened this issue 3 years ago • 6 comments

Describe the problem ParcelCSS errors, because patternfly doesn't use absolute url() in css.

@parcel/transformer-css: Ambiguous url('../../assets/images/status-icon-sprite.svg#success') in custom property. Relative paths are resolved from the location the var() is used, not where the
 custom property is defined. Use an absolute URL instead

  /home/rp/prj/node_modules/@patternfly/react-styles/css/components/FormControl/form-control.css:122:63
    121 |   --pf-c-form-control--textarea--invalid--BackgroundPositionY: var(--pf-c-form-control--PaddingLeft);
  > 122 |   --pf-c-form-control--m-icon-sprite--success--BackgroundUrl: url("../../assets/images/status-icon-sprite.svg#success");
  >     |                                                               ^
    123 |   --pf-c-form-control--m-icon-sprite--m-warning--BackgroundUrl: url("../../assets/images/status-icon-sprite.svg#warning");
    124 |   --pf-c-form-control--m-icon-sprite--invalid--BackgroundUrl: url("../../assets/images/status-icon-sprite.svg#invalid");

  💡 Replace with: url(/node_modules/@patternfly/react-styles/css/assets/images/status-icon-sprite.svg#success)

How do you reproduce the problem? Try to use patternfly with Parcel >= 2.4.0

Expected behavior Parcel correctly building

Is this issue blocking you? Work around is to use parcel < 2.4.0 or correctly activate PostCSS as alternative

Screenshots

What is your environment?

  • OS: archlinux
  • Browser chrome
  • Version 102

What is your product and what release date are you targeting? patternfly 2022.7

rp- avatar Jun 28 '22 09:06 rp-

https://parceljs.org/languages/css/#url()

Note: Only absolute paths may be used within CSS custom properties, not relative paths. This is because url() references in custom properties are resolved from the location where the var() is used, not where the custom property is defined. This means that the custom property could resolve to different URLs depending on which file it is used in. To resolve this ambiguity, use absolute paths when referencing URLs in custom properties.

jamestalton avatar Jul 05 '22 19:07 jamestalton

Parcel makes it easy to create UI libraries that support both ES6 modules and legacy CommonJS. It would be wonderful to use Parcel with PatternFly but it is blocked by this issue.

jamestalton avatar Jul 21 '22 17:07 jamestalton

Howdy @rp- and @jamestalton! Thanks for this issue, it's an interesting one. A handful of us met and went over this today to see what we might be able to do here. Here are some thoughts up front, and I think this would be a good topic to chat about at the next PF office hours.

I think we'll need to consider this from both the PF react and core perspectives, as the ways styles are imported are different. Generally speaking, you can import PF component styles one of 2 ways:

  1. patternfly.css (at the root of the package)
    • The assets dir is in the PWD, so url() paths to assets begin with ./
  2. components/ComponentName/component-name.css
    • The assets dir is at the root of the package, so url() begins with ../../

These relative paths are good for core/non-react users as it allows someone to place the PF package wherever they want in their document root, and the paths to the assets from either patternfly.css or the individual component CSS should resolve properly.

However (as I understand it anyways), react-styles always imports component styles from the individual component CSS, meaning the current path in the url() should always start with ../../, and the path before that should always be /node_modules/@patternfly/react-styles/css/ (the path to the package + /css). So this could be something that we offer on the react side, though I would want to verify what I've said here is actually the case, and consider the possibility of this breaking anyone. If we were able to offer this, it would likely need to be an opt-in for now, so as not to inadvertently break anyone whose use case for using styles isn't something we've considered.

Also a basic question about this error - is this more of a warning about the potential issues that could arise with using a relative path in a custom property, or is this issue is failing and/or incompatible with the way Parcel is bundling the styles?


From the core perspective, here are a few potential solutions.

  1. Override the problematic CSS variables with absolute paths. This isn't a great solution as it isn't sustainable as PF may change the values and add new vars with relative paths that also need to be redefined, so it's a bit of a whack-a-mole solution, but it could work as an interim solution assuming the overrides will allow Parcel to continue and it is able to ignore the original, overriden vars with relative paths currently. For the form control component, these vars: https://github.com/patternfly/patternfly/blob/9ab78a81b9d6d63cd7795fa82b1ff673abba5332/src/patternfly/components/FormControl/form-control.scss#L189-L195

    An override may look like:

    .pf-c-form-control {
      --pf-c-form-control--m-icon-sprite--success--BackgroundUrl: url("/node_modules/@patternfly/react-styles/css/assets/images/status-icon-sprite.svg#success");
      --pf-c-form-control--m-icon-sprite--m-warning--BackgroundUrl: url("/node_modules/@patternfly/react-styles/css/assets/images/status-icon-sprite.svg#warning");
      --pf-c-form-control--m-icon-sprite--invalid--BackgroundUrl: url("/node_modules/@patternfly/react-styles/css/assets/images/status-icon-sprite.svg#invalid");
      ...
    }
    
  2. Override the $pf-global--image-path var and recompile our SCSS, however compiling the SCSS is not always an option and likely isn't ideal. The global image path var is part of our global variables, as you can see in the form control styles, and establishes the path for all image assets defined in the PF CSS. That var is declared here: https://github.com/patternfly/patternfly/blob/9ab78a81b9d6d63cd7795fa82b1ff673abba5332/src/patternfly/sass-utilities/scss-variables.scss#L135

    This is also what's needed if, for example, someone wants to override the PF global media query breakpoint vars, since those are Sass vars because we can't use CSS vars in media queries. The same idea applies here since we can't use a CSS var for the file prefix (CSS vars don't allow you to concat strings).

  3. Since this is a potentially buggy way to declare paths, instead of specifying the url() in a custom property, we could provide it as the fallback value for the property it applies to. The only downside to that would be that the custom property for the url() PF uses could no longer be referenced by a custom property itself, though that seems like an unlikely scenario. Also, I'm not sure if this fixes the original issue. From the verbage on the Parcel url() docs, it seems like it would, but not sure if it would in practice.

    Instead of:

    .pf-c-form-control {
      --pf-c-form-control--m-icon-sprite--success--BackgroundUrl: url("#{$pf-global--image-path}/status-icon-sprite.svg#success");
    
      background-image: var(--pf-c-form-control--m-icon-sprite--success--BackgroundUrl);
    }
    

    We would do it like this:

    .pf-c-form-control {
      /* Removed the --pf-c-form-control--m-icon-sprite--success--BackgroundUrl var */
    
      background-image: var(--pf-c-form-control--m-icon-sprite--success--BackgroundUrl, url("#{$pf-global--image-path}/status-icon-sprite.svg#success"));
    }
    

Let me know what you think.

mcoker avatar Aug 09 '22 17:08 mcoker

It breaks parcel as it throws an error and refuses to run. I do not have a good suggestion on the fix.

jamestalton avatar Aug 09 '22 18:08 jamestalton

Sorry, I'm also too far away from all this CSS/SCSS stuff to give any real suggestion.

Suggestion 3 might work, but we would need to test, as it sounds as it would only fail if it is used in a custom property.

rp- avatar Aug 10 '22 07:08 rp-

Just a follow up from our discussion at office hours:

  • @jschuler is going to look into the possibility of updating @patternfly/react-styles to manipulate the paths used in css url() functions to be absolute (as Parcel has suggested in the error above). It looks like it could be added to the existing scripts that copy the PF core CSS into place.
  • It would be great to know if the third solution in my other comment works. If so, I think that should be fine to update on our end, at least for the text input/form control variables since that feature is beta currently. However, it looks like we also have relative paths in these two components. This could be a breaking change as those vars can be referenced now for the values they provide, and if we remove the vars, the values can no longer be referenced that way.

@jamestalton fwiw, I put together these pages to test if relative paths in CSS vars break if the components that use the vars are used on different paths. Using patternfly.css with a path to ./assets/..., or components/Component/component.css with a path to ../../assets... seems to work fine (my examples use patternfly.css), which are the use cases we support in core. I included an image at the bottom as a control just to ensure the github.io site wasn't doing something weird with the path resolution.

  • https://mcoker.github.io/relative.html
  • https://mcoker.github.io/dir/index.html
  • https://mcoker.github.io/dir/subdir/index.html

mcoker avatar Aug 10 '22 23:08 mcoker

I have this same problem for @patternfly/patternfly and I don't use @patternfly/react-styles

mingfang avatar Aug 18 '22 03:08 mingfang

Hey @mingfang! Can you tell me more about your setup - how the code is structured and why the paths are failing?

mcoker avatar Aug 18 '22 14:08 mcoker

I switched from parceljs to esbuild and now I'm able to use @patternfly/patternfly without any problems.

mingfang avatar Aug 20 '22 01:08 mingfang

This issue has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 30 days if no further activity occurs.

stale[bot] avatar Oct 19 '22 04:10 stale[bot]

btw. this is my workaround for now:

{
  "scripts": {
    "build": "yarn patch-patternfly && parcel build",
    "patch-patternfly": "sed -i 's/..\\/..\\/assets\\//\\/node_modules\\/@patternfly\\/react-styles\\/css\\/assets\\//g' node_modules/@patternfly/react-styles/css/components/BackgroundImage/background-image.css node_modules/@patternfly/react-styles/css/components/AboutModalBox/about-modal-box.css node_modules/@patternfly/react-styles/css/components/FormControl/form-control.css"
  }
}

rp- avatar Oct 20 '22 12:10 rp-

I am still interested in seeing this work.

jamestalton avatar Nov 15 '22 14:11 jamestalton

@jamestalton since the ideal update may likely be a breaking change, I've added the breaking/v5 label to the issue and will try and prioritize it with the other work we're planning on working on for v5, which will release in the first half of next year.

Would you or anyone else in the thread be interested in contributing a fix? We have a v5 branch for work toward that release.

mcoker avatar Nov 15 '22 15:11 mcoker

Thanks @rp- for the patch!

Looking forward to a fix in the patternfly 5.0.0 release!

I got this patch working in a windows dev environment with shx via the following in my package.json

"scripts": {
    "start": "yarn patch-patternfly; parcel public/index.html",
    "build": "yarn patch-patternfly; parcel build public/index.html",
    "patch-patternfly": "shx sed -i 's/..\\/..\\/assets\\//\\/node_modules\\/@patternfly\\/react-styles\\/css\\/assets\\//g' node_modules/@patternfly/react-styles/css/components/BackgroundImage/background-image.css node_modules/@patternfly/react-styles/css/components/AboutModalBox/about-modal-box.css node_modules/@patternfly/react-styles/css/components/FormControl/form-control.css"
  }

jdrews avatar Apr 02 '23 18:04 jdrews

Here's an updated patch-patternfly to account for the changes from react-core 4.276.8 to 4.276.9

"patch-patternfly": "shx sed -i 's/..\\/..\\/assets\\//\\/node_modules\\/@patternfly\\/react-styles\\/css\\/assets\\//g' node_modules/@patternfly/react-styles/css/components/BackgroundImage/background-image.css node_modules/@patternfly/react-styles/css/components/AboutModalBox/about-modal-box.css node_modules/@patternfly/react-styles/css/components/FormControl/form-control.css node_modules/@patternfly/react-core/node_modules/@patternfly/react-styles/css/components/BackgroundImage/background-image.css node_modules/@patternfly/react-core/node_modules/@patternfly/react-styles/css/components/AboutModalBox/about-modal-box.css node_modules/@patternfly/react-core/node_modules/@patternfly/react-styles/css/components/FormControl/form-control.css node_modules/@patternfly/react-core/node_modules/@patternfly/react-styles/css/components/AboutModalBox/about-modal-box.css"

jdrews avatar May 01 '23 02:05 jdrews