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

Make AMP Layouts first class in the validator. #1143

Merged
merged 1 commit into from
Dec 11, 2015
Merged

Make AMP Layouts first class in the validator. #1143

merged 1 commit into from
Dec 11, 2015

Conversation

powdercloud
Copy link
Contributor

This implements the logic in src/custom-element.js, applyLayout_.

validator.proto:

  • Introduce AmpLayout, a specification per TagSpec.
    When added, AmpLayout takes over responsibility for
    validating the four style attributes width,height,layout,sizes.
  • AmpLayout::Layout is an enum with the layouts.
  • I've modeled the natural dimensions with two booleans:
    defines_default_width and defines_default_height.
    This is efficient, and has sufficient expressive power for
    the validation needs. E.g., if only defines_default_height is set,
    that means the width will be auto.

validator.protoascii:

  • For all AMP tags, I've looked through the implementations
    in github to see how they override isLayoutSupported. E.g.,
    see here for amp-img.js: builtins/amp-img.js
    isLayoutSizeDefined is a predicate covering fixed,
    fixed-height, responsive, and fill, and in addition,
    nodisplay is always allowed. This, along with the whitelist
    from layout.js for hasNaturalDimensions is what then makes
    up a tag spec.

validator.js is implemented in the same way:

  • When AmpLayout is active (as in, the TagSpec under
    consideration has the amp_layout field set), then the four
    layout tags are added as optional via a taglist, $AMP_LAYOUT_ATTRS,
    to disable the usual attribute checks.
  • Instead, ValidateLayout is triggered, which works like so:
    • ParseLayout looks up the enum values.
    • CssLengthAndUnit is a parsing routine for the length values
      for width and height that we deal with. I made this into a
      constructor so that I can return four named values. This thing
      supports 'auto', based on a constructor argument.
    • After the input attributes have been parsed (that is, layout,
      width, height), CalculateWidth, CalculateHeight, and
      CalculateLayout compute the effective layout spec of the element.
      This is where the defaults get implemented, and the logic is
      intended to produce the same results as the original.
    • After this, we verify that the computed layout is OK for the
      tag, based on the specification from the protoascii
      (AmpLayout::supported_layouts). In the runtime, this is
      done via the isLayoutSupported hook I mentioned above.
    • After this, we run verification specific to particular layout
      values, e.g., FIXED_HEIGHT requires that the width field is
      set to 'auto'. I kept those errors short in our usual style.

Thanks to @dvoytenko and @Gregable for advice and review
(and catching bugs!).

@Gregable
Copy link
Member

LGTM

This implements the logic in src/custom-element.js, applyLayout_.

validator.proto:
- Introduce AmpLayout, a specification per TagSpec.
  When added, AmpLayout takes over responsibility for
  validating the four style attributes width,height,layout,sizes.
- AmpLayout::Layout is an enum with the layouts.
- I've modeled the natural dimensions with two booleans:
  defines_default_width and defines_default_height.
  This is efficient, and has sufficient expressive power for
  the validation needs. E.g., if only defines_default_height is set,
  that means the width will be auto.

validator.protoascii:
- For all AMP tags, I've looked through the implementations
  in github to see how they override isLayoutSupported. E.g.,
  see here for amp-img.js: builtins/amp-img.js
  isLayoutSizeDefined is a predicate covering fixed,
  fixed-height, responsive, and fill, and in addition,
  nodisplay is always allowed. This, along with the whitelist
  from layout.js for hasNaturalDimensions is what then makes
  up a tag spec.

validator.js is implemented in the same way:
- When AmpLayout is active (as in, the TagSpec under
  consideration has the amp_layout field set), then the four
  layout tags are added as optional via a taglist, $AMP_LAYOUT_ATTRS,
  to disable the usual attribute checks.
- Instead, ValidateLayout is triggered, which works like so:
  * ParseLayout looks up the enum values.
  * CssLengthAndUnit is a parsing routine for the length values
    for width and height that we deal with. I made this into a
    constructor so that I can return four named values. This thing
    supports 'auto', based on a constructor argument.
  * After the input attributes have been parsed (that is, layout,
    width, height), CalculateWidth, CalculateHeight, and
    CalculateLayout compute the effective layout spec of the element.
    This is where the defaults get implemented, and the logic is
    intended to produce the same results as the original.
  * After this, we verify that the computed layout is OK for the
    tag, based on the specification from the protoascii
    (AmpLayout::supported_layouts). In the runtime, this is
    done via the isLayoutSupported hook I mentioned above.
  * After this, we run verification specific to particular layout
    values, e.g., FIXED_HEIGHT requires that the width field is
    set to 'auto'. I kept those errors short in our usual style.

Thanks to @dvoytenko and @Gregable for advice and review
(and catching bugs!).
@powdercloud
Copy link
Contributor Author

@Gregable and I found some problem unrelated to this change which makes validator/build.py fail, and we'll fix it in another pr. In the meantime I've updated the PR to make Travis happy. Also we should make it so that Travis runs our build.py script to catch problems.

@powdercloud powdercloud assigned Gregable and unassigned dvoytenko Dec 11, 2015
powdercloud added a commit that referenced this pull request Dec 11, 2015
Make AMP Layouts first class in the validator.
@powdercloud powdercloud merged commit 2a6187d into ampproject:master Dec 11, 2015
@powdercloud powdercloud deleted the layouts branch April 8, 2016 22:13
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