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

Visual difference on Image styles between AMP and non-AMP pages #4419

Closed
amedina opened this issue Mar 23, 2020 · 3 comments · Fixed by #4421
Closed

Visual difference on Image styles between AMP and non-AMP pages #4419

amedina opened this issue Mar 23, 2020 · 3 comments · Fixed by #4421
Labels
Bug Something isn't working Integration: WP Core
Milestone

Comments

@amedina
Copy link
Member

amedina commented Mar 23, 2020

Bug Description

Pair browsing of an AMP-first site in Transitional mode shows visual differences on image layouts.

Expected Behaviour

There should be no visual disparities.

Steps to reproduce

  1. Activate the plugin in Transitional mode
  2. Activate TwentyTwenty
  3. Install Theme Unit Test data
  4. Pair browse blog feed

Screenshots

Additional context

Version: 1.5.0-RC1

Screen Shot 2020-03-22 at 9 55 06 PM

Screen Shot 2020-03-22 at 10 04 53 PM

Screen Shot 2020-03-22 at 10 04 38 PM

Screen Shot 2020-03-22 at 9 56 22 PM


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@amedina amedina added Bug Something isn't working Integration: WP Core labels Mar 23, 2020
@amedina amedina added this to the v1.5 milestone Mar 23, 2020
@westonruter
Copy link
Member

westonruter commented Mar 23, 2020

It's because the amp-img is getting display:inline-block whereas non-AMP img has display:block. In particular, it's because Twenty Twenty has this CSS:

svg,
img,
embed,
object {
	display: block;
	height: auto;
	max-width: 100%;
}

However, an amp-img with layout=intrinsic gets:

.i-amphtml-layout-intrinsic {
    display: inline-block;
    position: relative;
    max-width: 100%
}

This is overriding the specificity of the img selector.

I think what may be needed is adding a style rule to the core theme sanitizer specifically for Twenty Twenty that does this:

amp-img[layout=intrinsic] {
    display: block;
}

@westonruter
Copy link
Member

This is not a regression in 1.5-alpha. I tested in WordPress 5.0 with AMP plugin v1.4.x and it happens there as well.

@westonruter
Copy link
Member

I have an initial fix in #4421 specific for Twenty Twenty. It works by prepending a style rule to style[amp-custom] which sets amp-img to have display:block, with a selector that is specific enough to be weightier than .i-amphtml-layout-intrinsic.

There may be an opportunity to generalize this fix at the CSS parser level. For example, if coming across a selector that has just img and the rule includes display:block, then this could automatically result a style rule being added like:

.amp-block-img { display:block }

While at the same time adding the amp-block-img class to every amp-img. This should be more robust/automatic, though I can imagine there would be more edge cases. Namely, images added via amp-list or amp-script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Integration: WP Core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants