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

Missing CSS file #2755

Open
AlterWeb opened this issue Nov 22, 2022 · 8 comments
Open

Missing CSS file #2755

AlterWeb opened this issue Nov 22, 2022 · 8 comments

Comments

@AlterWeb
Copy link
Contributor

Preconditions (*)

  1. OpenMage 19.4.19

Steps to reproduce (*)

  1. Select base/default theme

Expected result (*)

  1. No CSS files are missing

Actual result (*)

  1. skin/frontend/base/default/css/styles-ie.css is added to the HTML but this file doesn't exist. This non existing file is added in the app/design/frontend/base/default/layout/page.xml layout file. Removing it from this file may lead to issues because it is not added in the default/default theme but the file skin/frontend/default/default/css/styles-ie.css does exist. The same is true for the default/blank and the default/blue theme. The default/iphone and default/modern theme also have this CSS file but they add it themself in the page.xml layout file.
@AlterWeb AlterWeb added the bug label Nov 22, 2022
@AlterWeb
Copy link
Contributor Author

Found this issue because it breaks the CSS merging in OpenMage 19.4.19 (#2753) for themes that extend on the base/default theme but do not have a styles-ie.css.

@fballiano
Copy link
Contributor

doesn't #2754 fix this one too?

@AlterWeb
Copy link
Contributor Author

It fixes the CSS merging. While the CSS merging is turned on everything works like it should (with only a unnecessary file lookup). But when the merging is disabled there is a missing CSS file included, resulting in a 404. Still not a very big issue but nice to fix it.

@AlterWeb
Copy link
Contributor Author

AlterWeb commented Nov 22, 2022

Okay, I looked a bit further into this. It's not just the skin/frontend/base/default/css/styles-ie.css but almost all CSS files from the app/design/frontend/base/default/layout/page.xml layout file that are not in the base/default skin folder. It was just that the specific theme for this customer had all the CSS files except the styles-ie.css in their skin folder. This never got noticed before because the 404 is only visible in IE and don't broke the design of this theme untill the #2753 bug which now already is fixed.

It's a bit strange that the CSS files are added in the base/default version of the page.xml file while there are no CSS files in the corresponding skin folder. We could say it's the theme developers task to make sure all CSS files added from the default page.xml file are included in there theme's skin folder or they have to override the page.xml layout file to remove them.

A fix on our side could be to add only the CSS files in the page.xml file that have a CSS file in the corresponding skin folder. So we would remove them from the base/default version of the page.xml and add them to the default/default version. The downfall of this approach is that it could break other themes that where made depending on those CSS files being added by default (in the base/default fallback theme).

Another fix on our side could be adding empty CSS files to the base/default theme. In this way there is always an existing backup version. I'm not sure however if loading empty files is much better then generating 404 errors. Both are not ideal.

@kiatng
Copy link
Contributor

kiatng commented Nov 23, 2022

styles-ie.css was removed in PR #1073. But it should only affect v20. The file was not touch in v19. See https://github.com/OpenMage/magento-lts/blob/1.9.4.x/skin/frontend/default/default/css/styles-ie.css Can you check to see if that file exists? If not, try adding it and see if it works.

@AlterWeb
Copy link
Contributor Author

AlterWeb commented Nov 23, 2022

@kiatng Thank you for replying. It is still there in 19.4.* the issue is that it is not in skin/frontend/base/default/css/styles-ie.css. If you create a theme within a new package namespace (I'm not sure it is called this way but what I mean is within app/design/frontend/NEWTHEME/default and skin/frontend/NEWTHEME/default) it is not extending the default/default theme but the base/default theme. In this base/default theme the CSS files are added in the page.xml layout file but most of them are not available in the skin/frontend/base/default folder. So if a theme developer (like the commercially bought theme one of our clients is using) doesn't add the CSS files to there theme it has no fallback of those CSS files, resulting in a 404.

@AlterWeb AlterWeb reopened this Nov 23, 2022
@kiatng
Copy link
Contributor

kiatng commented Nov 24, 2022

In the Designer's Guide to Magento, there is some explanation on the base theme:

Page 15

The base package was introduced in CE v1.4 and EE v1.8. The role of the base package is to provide hooks
to all of Magento’s core functionality, so that your custom themes can include in them just the changes to
that functionality that are specific to the design or business they are intended to support.

Page 16

the base package has only a default theme associated with it, although it is not actually a full theme because
it lacks most of the skin files.

So, the base package is not intended to be used as is.

Then why styles.css, styles-ie.css, and print.css are included here

<action method="addCss"><stylesheet>css/styles.css</stylesheet></action>
<action method="addItem"><type>skin_css</type><name>css/styles-ie.css</name><params/><if>lt IE 8</if></action>
<action method="addCss"><stylesheet>css/widgets.css</stylesheet></action>
<action method="addCss"><stylesheet>css/print.css</stylesheet><params>media="print"</params></action>

It's probably to maintain backward compatibility.

@AlterWeb
Copy link
Contributor Author

AlterWeb commented Nov 24, 2022

@kiatng Thank you for the reference to the official documentation on this. The line:

it lacks most of the skin files.

Looks like they knew the files where missing.

Then why styles.css, styles-ie.css, and print.css are included here

The inconsistency also is that the widgets.css is included in the base/default theme.

It's probably to maintain backward compatibility.

I think we should indeed (at least for the 19.* version) keep the backward compatibility, so we don't remove the lines in the page.xml. It could break more then it would fix. If we would like to do anything, we could add the missing CSS files as backup to the base/default theme because that won't break anything and it is in line with the purpose of this base package, like mentioned in the documentation:

The base package is the final fallback point

Or we decide that this is how Magento has chosen it should work and the developers of themes are responsible for changing the page.xml file to only include the CSS files they use in there theme. Then it's not a bug but a feature of Magento / OpenMage ;-) and a bug from the specific theme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants