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

Fix CSS parsing by stripping BOM #1611

Merged
merged 4 commits into from
Nov 20, 2018
Merged

Fix CSS parsing by stripping BOM #1611

merged 4 commits into from
Nov 20, 2018

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Nov 12, 2018

Steps to reproduce

  1. Activate the Essence Pro Genesis theme (or use the wp_enqueue_style() snippet below in another theme)
  2. View the source of the front page
  3. Expected: the ionicons stylesheet should have more that 0 bytes in the <style amp-custom>:

ionicons-expected

4. Actual: the stylesheet has 0 bytes...it seems that it's stripped in tree-shaking:

ionicons-actual

This stylesheet is enqueued in the Essence Pro theme's functions.php:

wp_enqueue_style(
	'ionicons',
	'//unpkg.com/ionicons@4.1.2/dist/css/ionicons.min.css',
	array(),
	CHILD_THEME_VERSION
);

This PR outputs the ionicons @font-face, as shown in the 'Expected' screenshot. Still, this approach probably needs more thought.

This issue doesn't look to exist on other Genesis themes that use an ionicons stylesheet. They use a different URL for the stylesheet, and it seems to appear fine.

Sometimes this appears in $parsed_selector['tags'],
and the logic look for the presence of the tag on the page.
This would apply well if it were an HTML tag like div,
but @font-face wouldn't appear as an element in an HTML document.
So add a conditional to check if the tag is in the
'allowed_at_rules'.
Still, this might not be the best solution.
Also, it looks like @font-face should appear at the top
of the stylesheet.
@kienstra kienstra changed the title [WIP] Address issue with stripped @font-face [WIP] Address stripped @font-face Nov 12, 2018
@hellofromtonya
Copy link
Contributor

@kienstra What's the status on this PR?

@kienstra
Copy link
Contributor Author

Status

Hi @hellofromtonya,
This PR is still in progress. It might need a different approach. I'll continue working on it in the next few days.

In the Essence Pro theme,
the ionicons CSS file has a "\ufeff" character.
For some reason, this blocks the style rule from applying
on the AMP endpoint, though it doesn't affect the non-AMP endpoint.
@kienstra
Copy link
Contributor Author

kienstra commented Nov 20, 2018

Byte Order Mark
Steps to reproduce

  1. Activate the Essence Pro theme, or simply use this snippet:
wp_enqueue_style(
	'ionicons',
	'https://unpkg.com/ionicons@4.1.2/dist/css/ionicons.min.css',
);
  1. Checkout 62ca70c from this PR's branch. Without that commit, the Ionicons stylesheet doesn't display at all.
  2. Go to the AMP URL for the homepage
  3. Expected: The ionicons font loads, and the search and menu icons at the top of the page display
  4. Actual: The ionicons fonts don't load, as the stylesheet seems to render with the “\ufeff” character (the Byte Order Mark):

byte-order-mark

Strangely, the “\ufeff” character wasn't a problem on the non-AMP endpoint. That character seems to be at the very beginning of the stylesheet, though it's not normally visible:

https://unpkg.com/ionicons@4.1.2/dist/css/ionicons.min.css

If you run this snippet inside PhpStorm, it'll show that the character is in the file:

preg_match( '/\x{FEFF}/u', wp_remote_get( 'https://unpkg.com/ionicons@4.1.2/dist/css/ionicons.min.css' )['body'] )

So 35c7386 removes this character from the concatenated CSS.

I'm not sure this is the right solution, but it has worked so far as a workaround.

@kienstra kienstra changed the title [WIP] Address stripped @font-face Address stripped @font-face Nov 20, 2018
@kienstra
Copy link
Contributor Author

Request For Review

Hi @westonruter,
Could you please review this PR, captured in https://github.com/studiopress/genesis-amp/issues/38?

Thanks!

@kienstra kienstra requested a review from westonruter November 20, 2018 13:53
@westonruter
Copy link
Member

@kienstra I think the issue is fully resolved simply be stripping the BOM before the CSS is parsed. I believe this is a bug with the PHP-CSS-Parser, so I've opened MyIntervals/PHP-CSS-Parser#150.

Please take a look to confirm. I'm now seeing the expected @font-face in the custom styles:

@font-face{font-family:"Ionicons";src:url("//unpkg.com/ionicons@4.1.2/dist/fonts/ionicons.eot?v=4.1.1");src:url("//unpkg.com/ionicons@4.1.2/dist/fonts/ionicons.eot?v=4.1.1#iefix") format("embedded-opentype"),url("//unpkg.com/ionicons@4.1.2/dist/fonts/ionicons.woff2?v=4.1.1") format("woff2"),url("//unpkg.com/ionicons@4.1.2/dist/fonts/ionicons.woff?v=4.1.1") format("woff"),url("//unpkg.com/ionicons@4.1.2/dist/fonts/ionicons.ttf?v=4.1.1") format("truetype"),url("//unpkg.com/ionicons@4.1.2/dist/fonts/ionicons.svg?v=4.1.1#Ionicons") format("svg");font-weight:normal;font-style:normal}

@westonruter westonruter changed the title Address stripped @font-face Fix CSS parsing by stripping BOM Nov 20, 2018
@westonruter westonruter added this to the v1.0 milestone Nov 20, 2018
@kienstra
Copy link
Contributor Author

Looks Good, Thanks For Fixing

Hi @westonruter,
Thanks for moving the removal of the BOM higher, and removing the line from the conditional.

As expected, the Ionicons load on AMP endpoints with Essence Pro:

ionicons-amp-endpoint

I bumped the cache group locally to force re-processing the stylesheet, but there's of course no need to bump that here.

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

Successfully merging this pull request may close these issues.

3 participants