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.

Sign up for GitHub

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

Jump to bottom

Split Reader mode style template part stylesheet from action-supplied styles #4205

Merged
merged 2 commits into from
Jan 31, 2020
Merged

Split Reader mode style template part stylesheet from action-supplied styles #4205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e7fa75
Split Reader mode style template part stylesheet from action-supplied…
westonruter Jan 30, 2020
d4c266a
Discontinue preserving original style[amp-custom]
westonruter Jan 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 6 additions & 24 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,13 +1233,8 @@ private function process_style_element( DOMElement $element ) {
'imported_font_urls' => $parsed['imported_font_urls'],
];

if ( $element->hasAttribute( 'amp-custom' ) && ! $this->amp_custom_style_element ) {
$this->amp_custom_style_element = $element;
} else {

// Remove from DOM since we'll be adding it to amp-custom.
$element->parentNode->removeChild( $element );
}
// Remove from DOM since we'll be adding it to a newly-created style[amp-custom] element later.
$element->parentNode->removeChild( $element );

$this->set_current_node( null );
}
Expand Down Expand Up @@ -2624,14 +2619,6 @@ private function finalize_styles() {

// Add style[amp-custom] to document.
if ( $stylesheet_groups[ self::STYLE_AMP_CUSTOM_GROUP_INDEX ]['included_count'] > 0 ) {

// Ensure style[amp-custom] is present in the document.
if ( ! $this->amp_custom_style_element ) {
$this->amp_custom_style_element = $this->dom->createElement( 'style' );
$this->amp_custom_style_element->setAttribute( 'amp-custom', '' );
$this->dom->head->appendChild( $this->amp_custom_style_element );
}

/*
* On AMP-first themes when there are new/rejected validation errors present, a parsed stylesheet may include
* @import rules. These must be moved to the beginning to be honored.
Expand All @@ -2641,16 +2628,11 @@ private function finalize_styles() {
$css .= implode( '', $this->get_stylesheets() );
$css .= $stylesheet_groups[ self::STYLE_AMP_CUSTOM_GROUP_INDEX ]['source_map_comment'];

/*
* Let the style[amp-custom] be populated with the concatenated CSS.
* !important: Updating the contents of this style element by setting textContent is not
* reliable across PHP/libxml versions, so this is why the children are removed and the
* text node is then explicitly added containing the CSS.
*/
while ( $this->amp_custom_style_element->firstChild ) {
$this->amp_custom_style_element->removeChild( $this->amp_custom_style_element->firstChild );
}
// Create the style[amp-custom] element and add it to the <head>.
$this->amp_custom_style_element = $this->dom->createElement( 'style' );
$this->amp_custom_style_element->setAttribute( 'amp-custom', '' );
$this->amp_custom_style_element->appendChild( $this->dom->createTextNode( $css ) );
$this->dom->head->appendChild( $this->amp_custom_style_element );
}

/*
Expand Down
8 changes: 7 additions & 1 deletion templates/html-start.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@
<meta charset="utf-8">
<meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">
<?php
// Note: The following two <style> tags are combined into <style amp-custom> via AMP_Style_Sanitizer.
// Splitting up the styles into two stylesheets allows for plugin-supplied styles via the amp_post_template_css action
// to be excluded from the styles in the style template part, which are more important given they style the overall page.

/**
* Fires when rendering <head> in Reader mode templates.
*
Expand All @@ -33,8 +37,10 @@
*/
do_action( 'amp_post_template_head', $this );
?>
<style amp-custom>
<style class="style-template-part">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, given that the document is passed through the sanitizers.

<?php $this->load_parts( [ 'style' ] ); ?>
</style>
<style class="amp-post-template-css-action">
<?php
/**
* Fires when printing CSS styles in Reader mode templates.
Expand Down