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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 Fix thirds and fill layer in Safari #19194

Merged
merged 4 commits into from Nov 15, 2018
Merged

馃悰 Fix thirds and fill layer in Safari #19194

merged 4 commits into from Nov 15, 2018

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Nov 8, 2018

Fixes #13433

Even though the layer has position: absolute; bottom: 0; top: 0;, Safari collapses the height as this comment points out. Setting it explicitly prevents this.

@gmajoulet
Copy link
Contributor

I think this fix doesn't work, since there is another height attribute in the same CSS block, that will override the one you added.
How can we test your fix?

Also, should we move to positioning all our "absolute" elements with
top: 0; left: 0; height: 100%; width: 100%? Is there anywhere else in our codebase where this bug might exist?

@Enriqe
Copy link
Contributor Author

Enriqe commented Nov 8, 2018

Tested it with a simple test case and it works well:

<!doctype html>
<html amp lang="en">

<head>
  <meta charset="utf-8">
  <script async src="https://cdn.ampproject.org/v0.js"></script>
  <script async custom-element="amp-story" src="https://cdn.ampproject.org/v0/amp-story-1.0.js"></script>
  <link rel="canonical" href="http://foo.com/">
  <title>Hello, World</title>

  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
</head>

<body>
  <amp-story standalone>
    <amp-story-page id="page0">
      <amp-story-grid-layer template="thirds">
        <div grid-area="lower-third">
          <p>Hello, World</p>
        </div>
      </amp-story-grid-layer>
    </amp-story-page>
  </amp-story>
</body>

</html>

Also tested with stories that were previously broken by the previous PR and everything seemed to work fine.

Was also waiting for the visual tests to run and they certainly caught something, updated the PR.

And I just filed #19195 for the latter part of your comment.

gmajoulet
gmajoulet reviewed Nov 8, 2018
View reviewed changes
extensions/amp-story/1.0/amp-story.css
@@ -261,6 +261,7 @@ amp-story-cta-layer {

/** Grid level */
amp-story-grid-layer {
box-sizing: border-box !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

@Enriqe Enriqe Nov 15, 2018 •
edited

Choose a reason for hiding this comment

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

If we don't include it, the thirds template won't consider padding for its 100% height calculation. See screenshots below:

No box-sizing: border-box:
image

With box-sizing: border-box:
image

You can also see this in a Percy build: (the right one hasbox-sizing: border-box). And I think this is what we want, is that correct @newmuis?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe the box-sizing is necessary here.

extensions/amp-story/1.0/amp-story.css Outdated
left: 0 !important;
height: 100% !important;
width: 100% !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be overridden by the width: auto below, and will, from what I understand, break the layout. Could you please upload your test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops. In this case width: auto and width: 100% were behaving the same, though.

The tests can be found under the visual diff examples:

http://localhost:8000/examples/visual-tests/amp-story/amp-story-grid-layer-template-fill.html
http://localhost:8000/examples/visual-tests/amp-story/amp-story-grid-layer-template-thirds.html
http://localhost:8000/examples/visual-tests/amp-story/amp-story-grid-layer-template-horizontal.html
http://localhost:8000/examples/visual-tests/amp-story/amp-story-grid-layer-template-vertical.html

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not actually sure we need to change fill layers at all, since the implementation doesn't use grid-areas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@newmuis you were right and the bug only affects to classes using grid-areas. Updated the PR to only update the thirds template's height.

@gmajoulet gmajoulet merged commit 48382c0 into ampproject:master Nov 15, 2018
Enriqe added a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
@Enriqe
馃悰 Fix thirds and fill layer in Safari (ampproject#19194)
978e26b 
* apply 100% to fill and thirds

* update css

* remove width auto

* manipulate height only in thirds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@newmuis newmuis newmuis approved these changes

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Enriqe @gmajoulet @newmuis @googlebot @ampprojectbot