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

Fix wide image to contain in content #1281

Merged
merged 3 commits into from
Aug 7, 2018

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Jul 24, 2018

This PR solves the problem of wide images overflowing the content width. It adds an inline style of width: auto to the <figure> element when it has the class attribute 'wp-block-image' but not is-resized.

Why?

Gutenberg adds the class attributes and sets the CSS rule to:

.wp-block-image {
    width: -webkit-fit-content;
    width: -moz-fit-content;
    width: fit-content;
}

The width value of fit-content is causing wider images to overflow. See the details in issue #1086 more information as well as screenshots. You can view the example posts here: Gutenberg post and Classic Editor post.

By adding the inline CSS rule, wider images rescale and fit within the content.

It's worth noting that the issue does not occur with the Classic Editor.

Closes #1086.

Tonya Mork added 2 commits July 24, 2018 12:21
A `<figure>` element with `.wp-block-image` and not `.is-resized` has its width set to a default of `fit-content`.  That CSS rule value causes wider images to overflow out of the parent container.  For this element, this commit adds an inline style of `width: auto`.

Closes #1086.
* @since 1.0
* @see https://github.com/Automattic/amp-wp/issues/1086
*
* @param DOMNode $node The DOMNode to adjust and replace.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be able to safely replace DOMNode with DOMElement here. We already know it is an element because we just called it via AMP_DOM_Utils::create_node().

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

This does fix the issue in my testing, but I'm struggling to understand why it fixes the issue. Is it because Chrome (et al) don't support width:fit-content with determining sizes for custom elements like amp-img? Can intrinsic sizes be only determined for replaced elements such as img and video?

@hellofromtonya
Copy link
Contributor Author

This does fix the issue in my testing, but I'm struggling to understand why it fixes the issue. Is it because Chrome (et al) don't support width:fit-content with determining sizes for custom elements like amp-img? Can intrinsic sizes be only determined for replaced elements such as img and video?

@westonruter Agreed. This PR feels like a bandaid to a more systematic problem between image rendering in non-AMP vs. AMP modes.

Let's put this PR on hold until we get a response back from the AMPHTML team on issue 17084.

@hellofromtonya hellofromtonya changed the title Fix wide image to contain in content [WIP] Fix wide image to contain in content Jul 25, 2018
@hellofromtonya
Copy link
Contributor Author

We will go ahead and merge this PR as it fixes the overflowing image issue. But we'll keep issue #1086 open to track for regression once the AMPHTML team resolves the open issue.

@hellofromtonya hellofromtonya merged commit d2ad9d6 into develop Aug 7, 2018
@westonruter westonruter added this to the v1.0 milestone Aug 29, 2018
@westonruter westonruter changed the title [WIP] Fix wide image to contain in content Fix wide image to contain in content Sep 5, 2018
@westonruter westonruter deleted the fix/wide-image-alignments branch September 6, 2018 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants