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

Remove space from 'data: url()' in stylesheets #1164

Merged
merged 6 commits into from May 26, 2018

Conversation

Projects
None yet
2 participants
@amedina
Copy link
Member

amedina commented May 21, 2018

Fixes #1089

@amedina amedina force-pushed the amedina/fix-1089 branch from 83c1e6f to 9649267 May 21, 2018

@amedina amedina force-pushed the amedina/fix-1089 branch from bbb254f to 178eb49 May 26, 2018

@amedina amedina requested a review from westonruter May 26, 2018

* Test handling of stylesheets with spaces in the background-image URLs.
*
* @dataProvider get_data_urls
* @covers AMP_Style_Sanitizer::normalize_urls()

This comment has been minimized.

@westonruter

westonruter May 26, 2018

Member

I think this is covering AMP_Style_Sanitizer::remove_spaces_from_data_urls() now, right?

* @param string $source Source URL string.
* @param string|null $expected Expected normalized URL string.
*/
public function test_normalize_urls( $source, $expected ) {

This comment has been minimized.

@westonruter

westonruter May 26, 2018

Member

Likewise, test_ remove_spaces_from_data_urls

'url_with_spaces' => array(
'html { background-image:url(url with spaces.png); }',
'html{background-image:url("urlwithspaces.png")}',
),

This comment has been minimized.

@westonruter

westonruter May 26, 2018

Member

Should include data: URL as discussed. Include spaces as reported in support forums.

if ( empty( $base_url ) ) {
return;
}
private function normalize_urls( $urls, $stylesheet_url = '' ) {

This comment has been minimized.

@westonruter

westonruter May 26, 2018

Member

I think most of the changes to normalize_urls should be reverted back to real_path_urls because as we found out, the PHP CSS parser fails to parse URLs with spaces in them anyway, so this will never be fed any such URLs. The added comments should be retained, of course.

This comment has been minimized.

@amedina

amedina May 26, 2018

Member

Reverted all, except some new code comments.

$options['stylesheet_url']
);
}
$this->normalize_urls(

This comment has been minimized.

@westonruter

westonruter May 26, 2018

Member

Per below, I think this change can be reverted.

@amedina amedina changed the title [WIP] Remove space from 'data: url()' in stylesheets Remove space from 'data: url()' in stylesheets May 26, 2018

@amedina amedina force-pushed the amedina/fix-1089 branch from 9fef5b0 to 882fa94 May 26, 2018

@amedina amedina force-pushed the amedina/fix-1089 branch from 882fa94 to 5644721 May 26, 2018

@amedina amedina self-assigned this May 26, 2018

@westonruter westonruter added this to the v1.0 milestone May 26, 2018

@westonruter westonruter merged commit 2fb9397 into develop May 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the amedina/fix-1089 branch May 26, 2018

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