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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

amp-story: "lower-third" grid area incorrectly positioned #13433

Closed
ithinkihaveacat opened this issue Feb 11, 2018 · 12 comments
Closed

amp-story: "lower-third" grid area incorrectly positioned #13433

ithinkihaveacat opened this issue Feb 11, 2018 · 12 comments
Assignees
@newmuis @Enriqe
Labels
GFI Candidate P1: High Priority Type: Bug WG: stories
Projects
Stories - By Type
Milestone
Backlog Bugs

Comments

@ithinkihaveacat
Copy link
Contributor

This only appears to affect beta versions of Safari.

The "lower-third" grid area of the "thirds" template is not positioned in the bottom third of the viewport in beta versions of Safari. (Screenshots are from desktop Safari Technology Preview Release 49 (Safari 11.2, WebKit 13606.1.3.3); it has been reported that the version of Safari that ships with the beta version of iOS exhibits the same behavior.)

Actual (beta Safari):

screen shot 2018-02-11 at 18 32 22

Expected (stable Safari):

screen shot 2018-02-11 at 18 32 13

Source code:

<!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-0.1.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>
@ithinkihaveacat ithinkihaveacat changed the title amp-story: amp-story: "lower-third" grid area incorrectly positioned Feb 11, 2018
@ampprojectbot ampprojectbot added this to the Pending Triage milestone Feb 12, 2018
@newmuis
Copy link
Contributor

newmuis commented Feb 14, 2018

Looks like Safari is not respecting grid area names with dashes 👎

@matthewgcallahan
Copy link

matthewgcallahan commented May 10, 2018 •
edited

@newmuis I'm noticing this in production on stories like https://www.washingtonpost.com/graphics/2018/opinions/amp-stories/ann-telnaes-trump-russia/ with iOS 11.3.1

Are there any updates on this? We've often moved to just using vertical templates

@TWsky
Copy link

TWsky commented Jun 4, 2018

@newmuis our partners production story has also countered the same issue.
Even the official sample (https://graphics.tvbs.com.tw/amp_stories/amp-pets-story/pets-completed.html) would failed for the position layout on the dog's page.
Hope there's any update on this.

Affected version: iOS 11.4

@abdelbolanos-cbc
Copy link

@newmuis any update on this one?, this bug is really annoying. Thanks

@newmuis
Copy link
Contributor

newmuis commented Jul 3, 2018 •
edited

Let's continue investigating this during the upcoming fixit

@newmuis newmuis added this to Incoming (Untriaged) in Stories - By Type via automation Jul 3, 2018
@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. Do you have any updates?

@droob
Copy link

droob commented Sep 17, 2018 •
edited

Safari's collapsing the height of amp-story-grid-layer. On the dog layer in the sample, for example, 1fr ends up being the height of the tallest grid section content, which is the <p>. Setting a fixed height for amp-story-grid-layer (or a calculated one, like calc(100% - 100px)) yields the expected results.

@ampprojectbot
Copy link
Member

This is a high priority issue but it hasn't been updated in awhile. Do you have any updates?

@abdelbolanos-cbc
Copy link

This bug is very annoying, I am going to change our stories to prevent use this thirds that Safari breaks.

Safari now
image

Chrome looks fine
image

@abdelbolanos-cbc
Copy link

If anybody is interested I got way to implement something similar to thirds but using CSS and works for Safari.

CSS

     /* Third template fix - thirds require template="horizontal" */
      .thirds {
        padding: 32px;
        top: 0px;
        height: 100%;
        width: 100%;
        position: absolute;
      }
      .thirds > div.upper,  .thirds > div.middle, .thirds > div.lower {
        height: 33%;
      }
      /* End Third template fix */

HTML

        <amp-story-grid-layer template="horizontal"> <!-- template needs to be horizontal -->
		<div> <!-- important don't remove this div -->
			<div class="thirds">
				<div class="upper"> <!-- your content here --> </div>
				<div class="middle"> <!-- your content here --> </div>
				<div class="lower"> <!-- your content here --> </div>
			</div>
		</div>
	</amp-story-grid-layer>

@newmuis newmuis mentioned this issue Oct 22, 2018
Merged
@newmuis
Copy link
Contributor

newmuis commented Oct 22, 2018

I'm sending a fix in #18871 that uses @droob's solution (setting an explicit height) that appears to work. It will likely take a little over a week to make it to production.

Thanks @droob!

Stories - By Type automation moved this from Incoming (Untriaged) to Done Oct 23, 2018
@Enriqe
Copy link
Contributor

Enriqe commented Nov 8, 2018 •
edited

We had to revert the last PR since it was causing breakages to existing publisher stories because grid layers with template="vertical" don't necessarily have to be 100% height.

I'm sending a PR soon to apply 100% height only to thirds and fill.

/cc @newmuis

@Enriqe Enriqe reopened this Nov 8, 2018
Stories - By Type automation moved this from Done to Incoming (Untriaged) Nov 8, 2018
@Enriqe Enriqe mentioned this issue Nov 8, 2018
Merged
@Enriqe Enriqe mentioned this issue Nov 8, 2018
Closed
Stories - By Type automation moved this from Incoming (Untriaged) to Done Nov 15, 2018
@newmuis newmuis mentioned this issue Nov 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@newmuis newmuis

@Enriqe Enriqe

Labels
GFI Candidate P1: High Priority Type: Bug WG: stories
Projects
Stories - By Type
  
Done
Milestone
Backlog Bugs
Development

No branches or pull requests
9 participants
@ithinkihaveacat @droob @newmuis @matthewgcallahan @TWsky @Enriqe @dreamofabear @ampprojectbot @abdelbolanos-cbc