Skip to content

Commit

Permalink
Merge pull request #119 from Lullabot/layout-related-correction
Browse files Browse the repository at this point in the history
Layout related correction
  • Loading branch information
sidkshatriya authored Jul 4, 2016
2 parents ec72088 + 1525bcc commit be3dd21
Show file tree
Hide file tree
Showing 11 changed files with 140 additions and 26 deletions.
1 change: 1 addition & 0 deletions src/AMP.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class AMP
'Lullabot\AMP\Pass\TwitterTransformPass',
'Lullabot\AMP\Pass\StandardScanPass',
'Lullabot\AMP\Pass\StandardFixPass',
'Lullabot\AMP\Pass\AmpImgFixPass',
'Lullabot\AMP\Pass\StandardFixPassTwo',
'Lullabot\AMP\Pass\MinimumValidFixPass',
'Lullabot\AMP\Pass\StatisticsPass'
Expand Down
80 changes: 80 additions & 0 deletions src/Pass/AmpImgFixPass.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
<?php
/*
* Copyright 2016 Google
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace Lullabot\AMP\Pass;

use Lullabot\AMP\Spec\AmpLayoutLayout;
use Lullabot\AMP\Validate\GroupedValidationResult;
use Lullabot\AMP\Utility\ActionTakenLine;
use Lullabot\AMP\Utility\ActionTakenType;
use Lullabot\AMP\Validate\Context;
use Lullabot\AMP\Validate\SValidationResult;
use Lullabot\AMP\Validate\ParsedValidatorRules;
use QueryPath\DOMQuery;
use Lullabot\AMP\Validate\SValidationError;
use Lullabot\AMP\Spec\ValidationErrorCode;
use Lullabot\AMP\Validate\ParsedTagSpec;


/**
* Class AmpImgFixPass
* @package Lullabot\AMP\Pass
*
* Try to fix amp-img tags with bad layouts or responsive layouts in which height and width are not specified properly
*/
class AmpImgFixPass extends ImgTagTransformPass
{
function __construct(DOMQuery $q, Context $context, SValidationResult $validation_result, GroupedValidationResult $grouped_validation_result, ParsedValidatorRules $parsed_rules, array $options)
{
parent::__construct($q, $context, $validation_result, $grouped_validation_result, $parsed_rules, $options);
}

function pass()
{
/** @var SValidationError $error */
foreach ($this->validation_result->errors as $error) {
if (in_array($error->code, [ValidationErrorCode::MANDATORY_ATTR_MISSING, ValidationErrorCode::IMPLIED_LAYOUT_INVALID, ValidationErrorCode::SPECIFIED_LAYOUT_INVALID]) &&
!$error->resolved &&
!empty($error->dom_tag) &&
$error->dom_tag->tagName == 'amp-img'
) {
$amp_img_el = new DOMQuery($error->dom_tag);

if (in_array($error->code, [ValidationErrorCode::IMPLIED_LAYOUT_INVALID, ValidationErrorCode::SPECIFIED_LAYOUT_INVALID])) {
$amp_img_el->attr('layout', 'responsive');
$error->addActionTaken(new ActionTakenLine('amp-img', ActionTakenType::AMP_IMG_FIX_RESPONSIVE));
$error->resolved = true;
}

$layout = ParsedTagSpec::parseLayout($amp_img_el->attr('layout'));
if ($error->code == ValidationErrorCode::MANDATORY_ATTR_MISSING &&
($layout !== AmpLayoutLayout::RESPONSIVE || !in_array($error->params[0], ['height', 'width']))
) {
continue;
}

$success = $this->setAmpImgAttributes($amp_img_el);
if ($success) {
$error->addActionTaken(new ActionTakenLine('amp-img', ActionTakenType::AMP_IMG_FIX));
$error->resolved = true;
} else {
$error->resolved = false;
}
}
}
}
}
18 changes: 15 additions & 3 deletions src/Pass/ImgTagTransformPass.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,18 @@ function pass()
$context_string = $this->getContextString($dom_el);
$new_dom_el = $this->cloneAndRenameDomElement($dom_el, 'amp-img');
$new_el = $el->prev();
$el->remove(); // remove the old img tag

$this->setAmpImgAttributes($new_el);
$success = $this->setAmpImgAttributes($new_el);
// We were not able to get the image dimensions, abort conversion.
if(!$success) {
$this->addActionTaken(new ActionTakenLine('img', ActionTakenType::IMG_COULD_NOT_BE_CONVERTED, $lineno, $context_string));
// Abort the conversion and remove the new img tag
$new_el->remove();
continue;
}

$el->remove(); // remove the old img tag
$this->setLayoutIfNoLayout($new_el, 'responsive');
$this->context->addLineAssociation($new_dom_el, $lineno);
$this->addActionTaken(new ActionTakenLine('img', ActionTakenType::IMG_CONVERTED, $lineno, $context_string));
}
Expand Down Expand Up @@ -163,9 +172,12 @@ protected function setAmpImgAttributes(DOMQuery $el)
if ($dimensions !== false) {
$el->attr('width', $dimensions['width']);
$el->attr('height', $dimensions['height']);
return true;
} else {
return false;
}
}

$this->setLayoutIfNoLayout($el, 'responsive');
return true;
}
}
7 changes: 5 additions & 2 deletions src/Pass/StandardFixPass.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,12 @@ class StandardFixPass extends BasePass
ValidationErrorCode::DISALLOWED_ATTR,
ValidationErrorCode::MISSING_URL,
ValidationErrorCode::UNESCAPED_TEMPLATE_IN_ATTR_VALUE,
ValidationErrorCode::TEMPLATE_PARTIAL_IN_ATTR_VALUE
// @todo ValidationErrorCode::MUTUALLY_EXCLUSIVE_ATTRS?
ValidationErrorCode::TEMPLATE_PARTIAL_IN_ATTR_VALUE,
ValidationErrorCode::ATTR_DISALLOWED_BY_SPECIFIED_LAYOUT,
ValidationErrorCode::ATTR_DISALLOWED_BY_IMPLIED_LAYOUT,
ValidationErrorCode::SPECIFIED_LAYOUT_INVALID
];

protected $remove_tags_for_codes = [
ValidationErrorCode::WRONG_PARENT_TAG,
ValidationErrorCode::DISALLOWED_TAG,
Expand Down
3 changes: 3 additions & 0 deletions src/Utility/ActionTakenType.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class ActionTakenType
const PROPERTY_REMOVED = 'property value pair was removed from attribute due to validation issues.';
const PROPERTY_REMOVED_ATTRIBUTE_REMOVED = 'property value pair was removed from attribute due to validation issues. The resulting attribute was empty and was also removed.';
const IMG_CONVERTED = 'tag was converted to the amp-img tag.';
const IMG_COULD_NOT_BE_CONVERTED = 'tag could NOT be converted to the amp-img tag as the image is not accessible.';
const INSTAGRAM_CONVERTED = 'instagram embed code was converted to the amp-instagram tag.';
const PINTEREST_CONVERTED = 'pinterest embed code was converted to the amp-pinterest tag.';
const VINE_CONVERTED = 'vine embed code was converted to the amp-vine tag.';
Expand All @@ -47,4 +48,6 @@ class ActionTakenType
const TAG_REMOVED_FROM_HEAD_AFTER_REVALIDATE_FAILED = 'tag removed from head as it still does not validate. Could not fix tag validation problems.';
const ATTRIBUTE_REMOVED_MUTUALLY_EXCLUSIVE = 'attribute(s) removed as they were mutually exclusive.';
const ISSUE_RESOLVED = 'no further action required as this issue was resolved due to an earlier fix';
const AMP_IMG_FIX = 'tried to fix problems with amp-img by adjusting one or more of: height, width, and setting layout to responsive';
const AMP_IMG_FIX_RESPONSIVE = 'tried to fix problems with amp-img by setting layout to responsive';
}
14 changes: 7 additions & 7 deletions src/Validate/ParsedTagSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -643,21 +643,21 @@ public function validateLayout(Context $context, array $attrs_by_key, SValidatio
$input_layout = self::parseLayout($layout_attr);
if (!empty($layout_attr) && $input_layout === AmpLayoutLayout::UNKNOWN) {
$context->addError(ValidationErrorCode::INVALID_ATTR_VALUE,
['layout', self::getTagSpecName($this->spec), $layout_attr], $this->spec->spec_url, $result);
['layout', self::getTagSpecName($this->spec), $layout_attr], $this->spec->spec_url, $result, 'layout');
return;
}

$input_width = new CssLengthAndUnit($width_attr, true);
if (!$input_width->is_valid) {
$context->addError(ValidationErrorCode::INVALID_ATTR_VALUE,
['width', self::getTagSpecName($this->spec), $width_attr], $this->spec->spec_url, $result);
['width', self::getTagSpecName($this->spec), $width_attr], $this->spec->spec_url, $result, 'width');
return;
}

$input_height = new CssLengthAndUnit($height_attr, true);
if (!$input_height->is_valid) {
$context->addError(ValidationErrorCode::INVALID_ATTR_VALUE,
['height', self::getTagSpecName($this->spec), $height_attr], $this->spec->spec_url, $result);
['height', self::getTagSpecName($this->spec), $height_attr], $this->spec->spec_url, $result, 'height');
return;
}

Expand All @@ -667,15 +667,15 @@ public function validateLayout(Context $context, array $attrs_by_key, SValidatio

if ($height->is_auto && $layout !== AmpLayoutLayout::FLEX_ITEM) {
$context->addError(ValidationErrorCode::INVALID_ATTR_VALUE,
['height', self::getTagSpecName($this->spec), $height_attr], $this->spec->spec_url, $result);
['height', self::getTagSpecName($this->spec), $height_attr], $this->spec->spec_url, $result, 'height');
return;
}

if (!in_array($layout, $this->spec->amp_layout->supported_layouts)) {
$code = empty($layout_attr) ? ValidationErrorCode::IMPLIED_LAYOUT_INVALID :
ValidationErrorCode::SPECIFIED_LAYOUT_INVALID;
$context->addError($code,
[$layout, self::getTagSpecName($this->spec)], $this->spec->spec_url, $result);
[$layout, self::getTagSpecName($this->spec)], $this->spec->spec_url, $result, empty($layout_attr) ? '' : 'layout');
return;
}

Expand All @@ -702,7 +702,7 @@ public function validateLayout(Context $context, array $attrs_by_key, SValidatio
return;
} else if ($width->is_auto) {
$context->addError(ValidationErrorCode::INVALID_ATTR_VALUE,
['width', self::getTagSpecName($this->spec), 'auto'], $this->spec->spec_url, $result);
['width', self::getTagSpecName($this->spec), 'auto'], $this->spec->spec_url, $result, 'width');
return;
}
}
Expand All @@ -717,7 +717,7 @@ public function validateLayout(Context $context, array $attrs_by_key, SValidatio
$code = empty($layout_attr) ? ValidationErrorCode::ATTR_DISALLOWED_BY_IMPLIED_LAYOUT :
ValidationErrorCode::ATTR_DISALLOWED_BY_SPECIFIED_LAYOUT;
$context->addError($code,
['heights', self::getTagSpecName($this->spec), $layout], $this->spec->spec_url, $result);
['heights', self::getTagSpecName($this->spec), $layout], $this->spec->spec_url, $result, 'heights');
return;
}
}
Expand Down
21 changes: 14 additions & 7 deletions tests/test-data/full-html/amp_layouts.html.out
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@

<!-- invalid: layout=fill with height=auto; height=auto is only allowed
for flex-item -->
<amp-img layout="fill" width="50" height="auto"></amp-img>
<amp-img layout="fill" width="50"></amp-img>

<!-- valid: layout=flex-item -->
<amp-img layout="flex-item" src="itshappening.gif"></amp-img>
Expand All @@ -82,10 +82,10 @@
TODO(johannes): Long run we should get rid of container or use it.
https://github.com/ampproject/amphtml/issues/1109
-->
<amp-img layout="container" src="itshappening.gif"></amp-img>
<amp-img src="itshappening.gif"></amp-img>

<!-- invalid: layout=unknown -->
<amp-img layout="unknown" src="itshappening.gif"></amp-img>
<amp-img src="itshappening.gif"></amp-img>

<!-- valid: should configure natural dimensions; default layout -->
<amp-pixel src="https://www.example.com/make-my-visit-count"></amp-pixel>
Expand Down Expand Up @@ -116,10 +116,10 @@
<amp-pixel src="https://www.example.com/make-my-visit-count" width="auto" height="1px"></amp-pixel>

<!-- invalid: width=X. -->
<amp-pixel src="https://www.example.com/make-my-visit-count" width="X" height="1px"></amp-pixel>
<amp-pixel src="https://www.example.com/make-my-visit-count" height="1px"></amp-pixel>

<!-- invalid: height=X. -->
<amp-pixel src="https://www.example.com/make-my-visit-count" height="X" width="1px"></amp-pixel>
<amp-pixel src="https://www.example.com/make-my-visit-count" width="1px"></amp-pixel>

<!-- valid responsive layout with heights -->
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" width="400" height="300" layout="responsive" heights="(min-width:760px) 33%, (min-width:500px) 75%, 125%"></amp-img>
Expand All @@ -128,10 +128,10 @@
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" width="400" height="300" heights="(min-width:760px) 33%, (min-width:500px) 75%, 125%"></amp-img>

<!-- invalid: can't have heights with specified layout fixed -->
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" width="400" height="300" layout="fixed" heights="(min-width:760px) 33%, (min-width:500px) 75%, 125%"></amp-img>
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" width="400" height="300" layout="fixed"></amp-img>

<!-- invalid: can't have heights with implied layout fixed_height -->
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" height="300" heights="(min-width:760px) 33%, (min-width:500px) 75%, 125%"></amp-img>
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" height="300"></amp-img>
</body>

</html>
Expand Down Expand Up @@ -318,16 +318,19 @@ FAIL
<amp-img layout="fill" width="50" height="auto"> on line 76
- The attribute 'height' in tag 'amp-img' is set to the invalid value 'auto'.
[code: INVALID_ATTR_VALUE category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.height attribute was removed due to validation issues.
- The mandatory attribute 'src' is missing in tag 'amp-img'.
[code: MANDATORY_ATTR_MISSING category: AMP_TAG_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]

<amp-img layout="container" src="itshappening.gif"> on line 96
- The specified layout 'CONTAINER' is not supported by tag 'amp-img'.
[code: SPECIFIED_LAYOUT_INVALID category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.layout attribute was removed due to validation issues.

<amp-img layout="unknown" src="itshappening.gif"> on line 99
- The attribute 'layout' in tag 'amp-img' is set to the invalid value 'unknown'.
[code: INVALID_ATTR_VALUE category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.layout attribute was removed due to validation issues.

<amp-pixel src="https://www.example.com/make-my-visit-count" width="auto" height="1px"> on line 130
- The implied layout 'FIXED_HEIGHT' is not supported by tag 'amp-pixel'.
Expand All @@ -336,18 +339,22 @@ FAIL
<amp-pixel src="https://www.example.com/make-my-visit-count" width="X" height="1px"> on line 134
- The attribute 'width' in tag 'amp-pixel' is set to the invalid value 'X'.
[code: INVALID_ATTR_VALUE category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-pixel.html]
ACTION TAKEN: amp-pixel.width attribute was removed due to validation issues.

<amp-pixel src="https://www.example.com/make-my-visit-count" height="X" width="1px"> on line 138
- The attribute 'height' in tag 'amp-pixel' is set to the invalid value 'X'.
[code: INVALID_ATTR_VALUE category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-pixel.html]
ACTION TAKEN: amp-pixel.height attribute was removed due to validation issues.

<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" width="400" height="300" layout="fixed" heights="(min-width:760px) 33%, (min-width:500px) 7... on line 150
- The attribute 'heights' in tag 'amp-img' is disallowed by specified layout 'FIXED'.
[code: ATTR_DISALLOWED_BY_SPECIFIED_LAYOUT category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.heights attribute was removed due to validation issues.

<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no-n" height="300" heights="(min-width:760px) 33%, (min-width:500px) 75%, 125%"> on line 154
- The attribute 'heights' in tag 'amp-img' is disallowed by implied layout 'FIXED_HEIGHT'.
[code: ATTR_DISALLOWED_BY_IMPLIED_LAYOUT category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.heights attribute was removed due to validation issues.

COMPONENT NAMES WITH JS PATH
------------------------------
Expand Down
13 changes: 9 additions & 4 deletions tests/test-data/full-html/mandatory_dimensions.html.out
Original file line number Diff line number Diff line change
Expand Up @@ -113,17 +113,17 @@

<!-- Invalid Examples -->
<!-- Container layout isn't supported by amp-img. -->
<amp-img src="img" layout="container">
<amp-img src="img" layout="container" width="42" height="42">
<amp-img src="img">
<amp-img src="img" width="42" height="42">
<amp-img src="img" layout="responsive">

<!-- Layout of responsive/fixed without width/height -->
<amp-img src="img" layout="responsive">
<amp-img src="img" layout="fixed">
<amp-img src="img" layout="responsive" width="42">
<amp-img src="img" layout="fixed" height="42">
<amp-img src="img" layout="fixed" height="auto">
<amp-img src="img" layout="fixed" width="auto" height="42">
<amp-img src="img" layout="fixed">
<amp-img src="img" layout="fixed" height="42">

<!-- Inconsistent units -->
<amp-img src="img" layout="responsive" width="42px" height="42rem">
Expand Down Expand Up @@ -320,14 +320,17 @@ FAIL
<amp-img src="img" layout="container"> on line 110
- The specified layout 'CONTAINER' is not supported by tag 'amp-img'.
[code: SPECIFIED_LAYOUT_INVALID category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.layout attribute was removed due to validation issues.

<amp-img src="img" layout="container" width="42" height="42"> on line 111
- The specified layout 'CONTAINER' is not supported by tag 'amp-img'.
[code: SPECIFIED_LAYOUT_INVALID category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.layout attribute was removed due to validation issues.

<amp-img src="img"> on line 112
- The implied layout 'CONTAINER' is not supported by tag 'amp-img'.
[code: IMPLIED_LAYOUT_INVALID category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img tried to fix problems with amp-img by setting layout to responsive

<amp-img src="img" layout="responsive"> on line 115
- The mandatory attribute 'height' is missing in tag 'amp-img'.
Expand All @@ -348,10 +351,12 @@ FAIL
<amp-img src="img" layout="fixed" height="auto"> on line 119
- The attribute 'height' in tag 'amp-img' is set to the invalid value 'auto'.
[code: INVALID_ATTR_VALUE category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.height attribute was removed due to validation issues.

<amp-img src="img" layout="fixed" width="auto" height="42"> on line 120
- The attribute 'width' in tag 'amp-img' is set to the invalid value 'auto'.
[code: INVALID_ATTR_VALUE category: AMP_LAYOUT_PROBLEM see: https://www.ampproject.org/docs/reference/amp-img.html]
ACTION TAKEN: amp-img.width attribute was removed due to validation issues.

<amp-img src="img" layout="responsive" width="42px" height="42rem"> on line 123
- Inconsistent units for width and height in tag 'amp-img' - width is specified in 'px' whereas height is specified in 'rem'.
Expand Down
Loading

0 comments on commit be3dd21

Please sign in to comment.