[amp-story-player] Add ShadowDOM fallback for edge #30254
Conversation
There was a problem hiding this comment.
You have some styles that will be problematic in amp-story-player-iframe.css:
:hostwill not target anything, should we create anothercontainerToUsediv and apply the same properties?- you set some CSS properties using the
mainselector, which will cause CSS collisions with non amp-story-player DOM - same for the
iframe:nth-of-type(n)selectors
| @@ -33,6 +33,12 @@ amp-story-player a:not(:first-of-type) { | |||
| visibility: hidden; | |||
| } | |||
|
|
|||
| amp-story-player.i-amphtml-story-player-loaded a:first-of-type { | |||
There was a problem hiding this comment.
Can you explain what this does and why it is needed in this PR? If you're trying to hide the links, should we just display: none;? Also, why is this not an issue when Shadow DOM is supported?
There was a problem hiding this comment.
Not sure why, but when there's no shadow root inside the amp-story-player, the <a> tags get inherited size from the parent element: (see computed size)
Compare when there's a shadow DOM as sibling of the <a> tags:
There was a problem hiding this comment.
Why is this an issue, don't the stories iframe cover it?
There was a problem hiding this comment.
Nope, when the <a> tags get a size they get pushed under the iframe.
There was a problem hiding this comment.
As discussed offline, let's make sure the iframe container covers whatever is inside the tag by using a position absolute :))
There was a problem hiding this comment.
As discussed offline: when swiping in the last story, the <a> tag and whatever is inside of it will be visible. We think it's better to just hide it.
There was a problem hiding this comment.
Why are we only hiding the first one though? Shouldn't we hide all the content within the amp-story-player tag, even if publishers add an <img> or a <p> or multiple <a>?
There was a problem hiding this comment.
Yeah, for now we only hide the first one because the player explicitly sets styles on it to display the poster image while the story loads. But we can't know for sure what publisher does inside of it. Maybe we could do a
amp-story-player > *{ display:none } amp-story-player > .main-container { display:block }. WDYT?
There was a problem hiding this comment.
amp-story-player.loaded > *{ display:none } yes :))
You'd need to test it with and without shadow DOM.
You have more context but from what I know it sounds safer?
There was a problem hiding this comment.
Thanks for the useful comments!
:host will not target anything, should we create another containerToUse div and apply the same properties?
I just deleted the :host rules cause we weren't actually using them.
you set some CSS properties using the main selector, which will cause CSS collisions with non amp-story-player DOM
same for the iframe:nth-of-type(n) selectors
Good call. I made all of these be specific to children inside .amp-story-player-main-container. Which will be the container in both cases (shadow DOM or not).
| @@ -33,6 +33,12 @@ amp-story-player a:not(:first-of-type) { | |||
| visibility: hidden; | |||
| } | |||
|
|
|||
| amp-story-player.i-amphtml-story-player-loaded a:first-of-type { | |||
There was a problem hiding this comment.
Not sure why, but when there's no shadow root inside the amp-story-player, the <a> tags get inherited size from the parent element: (see computed size)
Compare when there's a shadow DOM as sibling of the <a> tags:
| @@ -33,6 +33,12 @@ amp-story-player a:not(:first-of-type) { | |||
| visibility: hidden; | |||
| } | |||
|
|
|||
| amp-story-player.i-amphtml-story-player-loaded a:first-of-type { | |||
There was a problem hiding this comment.
Why is this an issue, don't the stories iframe cover it?
* fallback for edge * tweak css * fix specificity, use display none * rename CSS class
Closes #27358
Adds a fallback for older Edge browsers that don't support
attachShadowby using the player element as container instead of a shadow root.It also resets the width/height of the
<a>, so that it doesn't push the stories below when using the player element as a container.