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 IE hacks from stylesheets since fail AMP validation #1099

Closed
westonruter opened this Issue Apr 27, 2018 · 5 comments

Comments

3 participants
@westonruter
Copy link
Member

westonruter commented Apr 27, 2018

AMP validation fails on IE hacks, for example:

textarea {
	font-size: 100%; /* Corrects font size not being inherited in all browsers */
	margin: 0; /* Addresses margins set differently in IE6/7, F3/4, S5, Chrome */
	vertical-align: baseline; /* Improves appearance and consistency in all browsers */
	*vertical-align: middle; /* Improves appearance and consistency in IE6/IE7 */
}

Results in:

image

The CSS parser now part of the plugin is supposed to support an awareness of IE hacks, so it's possible that we could be able to automatically delete any property that has such a hack with something like this:

--- a/includes/sanitizers/class-amp-style-sanitizer.php
+++ b/includes/sanitizers/class-amp-style-sanitizer.php
@@ -901,6 +901,13 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
 			}
 		}
 
+		// Remove all properties that have IE hacks since they fail AMP validation.
+		foreach ( $ruleset->getRules() as $rule ) {
+			if ( 0 !== count( $rule->getIeHack() ) ) {
+				$ruleset->removeRule( $rule );
+			}
+		}
+
 		if ( $ruleset instanceof AtRuleSet && 'font-face' === $ruleset->atRuleName() ) {
 			$this->process_font_face_at_rule( $ruleset, $options );
 		}

The assumption is that IE hacks are obsolete since they target versions of IE that are long dead.

@westonruter westonruter added the css label Apr 27, 2018

@DavidCramer DavidCramer self-assigned this May 2, 2018

@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented May 2, 2018

@westonruter I've been testing this and it does remove them. The sanitiser simply identifies them as invalid properties and removes it as it would any other invalid css rules.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented May 2, 2018

@DavidCramer Cool. I'm curious as to why getIeHack exists then in the library. At least it would be good to create a PR with tests for the various IE hacks in the wild and make sure that they are stripped out.

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

@westonruter westonruter added this to To do in v1.0 May 2, 2018

@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented May 2, 2018

@westonruter I tried to get the isIehack to be populated, but it never got to that point where getIeHack could find it, since it already stripped it.
I'll add some tests in that focuses on the IE ( and other browser hacks for that matter ) being stripped out, and do a PR based on this ticket.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jun 4, 2018

@DavidCramer please push up the tests you have so we can close this.

@westonruter westonruter moved this from To do to In progress in v1.0 Jun 4, 2018

@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented Jun 5, 2018

@westonruter pushed to 4987bff Sorry for the delay.

@westonruter westonruter moved this from In progress to Ready for QA in v1.0 Jun 5, 2018

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Jun 6, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

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