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 image block centering when resizing to a smaller size #26376

Merged
merged 4 commits into from
Nov 3, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Oct 22, 2020

Description

Fixes #23986

When resizing an image to be smaller, the block was unexpectedly centering.

The wrapping figure for the image block is set to display: table. When its contents are resized to a smaller size this wrapper's width collapses to its content size. At the same time blocks have auto left and right margin, so the image ends up centered.

The fix I've applied is to add with: 100% to the figure so that it behave more like display: block.

How has this been tested?

  1. Add an image block
  2. Make it smaller
  3. The image should stay left aligned

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@talldan talldan added [Type] Regression Related to a regression in the latest release [Block] Image Affects the Image Block CSS Styling Related to editor and front end styles, CSS-specific issues. labels Oct 22, 2020
@talldan talldan requested a review from ajlende as a code owner October 22, 2020 05:11
@talldan talldan self-assigned this Oct 22, 2020
@github-actions
Copy link

github-actions bot commented Oct 22, 2020

Size Change: +54 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/block-editor/index.js 130 kB -24 B (0%)
build/block-library/editor-rtl.css 9.02 kB +49 B (0%)
build/block-library/editor.css 9.02 kB +48 B (0%)
build/block-library/index.js 146 kB -35 B (0%)
build/block-library/style-rtl.css 7.84 kB +11 B (0%)
build/block-library/style.css 7.85 kB +10 B (0%)
build/edit-navigation/index.js 11.2 kB -2 B (0%)
build/edit-post/index.js 306 kB +16 B (0%)
build/edit-post/style-rtl.css 6.41 kB +5 B (0%)
build/edit-post/style.css 6.39 kB +5 B (0%)
build/edit-site/index.js 22.1 kB +1 B
build/edit-site/style.css 3.88 kB -1 B
build/edit-widgets/index.js 26.4 kB -1 B
build/edit-widgets/style-rtl.css 3.13 kB +4 B (0%)
build/edit-widgets/style.css 3.13 kB +2 B (0%)
build/editor/index.js 43.1 kB -34 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/theme-rtl.css 837 B 0 B
build/block-library/theme.css 838 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 772 B 0 B
build/data/index.js 8.77 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-site/style-rtl.css 3.88 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@talldan talldan added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 22, 2020
@jasmussen
Copy link
Contributor

This is an excellent PR. Thank you!

Before:

before

After:

after

I would certainly agree that this PR enables the expected behavior, and that what's shipping is less ideal.

However there's one problem, and it's the same problem that caused us to explore the display: table; solution in the first place. When captions grow long, they should ideally only be as wide as the image is, and no wider. But here, that's no longer the case:

Screenshot 2020-10-22 at 11 42 50

It's still the case with floats, that's good:

Screenshot 2020-10-22 at 11 43 13

In fact, that may be good enough — if the block isn't floated, it is meant to take up the full width, right? But if we were to decide that the above is fine — a resized image that isn't floated can have a caption that spills beyond its own width — then we should likely solve this issue in a different way.

display: table; was specifically chosen because it let the 2nd element, the figcaption, have the same width as the first element, the img, without resizing the figure. If the caption no longer needs to behave like that unless floated, we could simply limit the display: table; to being applied only to floats.

Curious on your thoughts, Kjell! Also CC: @jffng and @MichaelArestad in case you have thoughts.

@tellthemachines
Copy link
Contributor

On a similar note to @jasmussen 's comment above, I notice that this solves the problem for the image, but leaves the caption centred:

Screen Shot 2020-10-23 at 4 33 56 pm

Before, both resized image and caption were left-aligned, as there was a bit of CSS that set the margin on resized figures to 0:

[data-type="core/image"] figure.is-resized {
    margin: 0;
}

Could we just add that back, or was it breaking anything?

@talldan
Copy link
Contributor Author

talldan commented Oct 23, 2020

@tellthemachines I think the alignment of the caption is theme specific (Twenty Twenty-One aligns it left). The caption should be centered from the styles the plugin provides by default, but as @jasmussen says it should also be the same width as the image.

@jasmussen This does already seem broken in master, but it's only noticable if you upload an already very small image. The result is the caption is not the same width as the image:
Screenshot 2020-10-23 at 2 40 41 pm

So it looks like the caption doesn't work anyway, at least not consistently. Possibly a separate bug? I'm not really sure how we can fix it without another wrapper around the image.

@jasmussen
Copy link
Contributor

This does already seem broken in master, but it's only noticable if you upload an already very small image.

To me that's an argument in favor of doing what you suggest — only limiting the caption width if an image is floated, or resized.

However I still think if we agree on going that route, that it deserves looking into refactoring display: table; to only affect floats and resized images then.

@tellthemachines
Copy link
Contributor

@tellthemachines I think the alignment of the caption is theme specific (Twenty Twenty-One aligns it left).

That screenshot was from Twenty Twenty One, so it's definitely broken then 😅

In any case, is there anything stopping us from doing what I suggested above?

Before, both resized image and caption were left-aligned, as there was a bit of CSS that set the margin on resized figures to 0:

 [data-type="core/image"] figure.is-resized {
   margin: 0;
 }

Could we just add that back, or was it breaking anything?

@jasmussen
Copy link
Contributor

[data-type="core/image"] figure.is-resized {
	margin: 0;
}

Apologies, I forgot to respond to that one. IOU one milkshake 📝

This would only be in the editor, correct?

In general I'm a little nervous both about touching this CSS at all, because it's so extremely widely used, but also because any additional CSS added is CSS that could affect how themes deal with it. Notably margins in a context where we have a centered column, can cause trouble when auto is expected horizontally. So that could be what it was breaking.

@tellthemachines
Copy link
Contributor

Milkshake accepted 😋

This would only be in the editor, correct?

Yup, using [data-type="core/image"] ensures that it won't affect the front end. But now I see that the selector structure is wrong, because the data-type is on the same figure element that takes is-resized. Maybe that snippet was removed because the markup changed?

We could still make it work by using [data-type="core/image"].is-resized instead, with the advantage of slightly lower specificity.

@talldan
Copy link
Contributor Author

talldan commented Oct 29, 2020

This doesn't help unfortunately:

[data-type="core/image"].is-resized {
	margin: 0;
}

Removing any block margin makes the block sit on the left of the screen.
Screenshot 2020-10-29 at 1 30 36 pm

I have theme styles disabled to reproduce this.

@talldan
Copy link
Contributor Author

talldan commented Oct 29, 2020

Not really sure what a good solution is. I think the problem possibly originates from removing wrappers from the block.

But also blocks in the editor have margin-left: auto; margin-right: auto; by default to center them, which is in contrast to how themes seem to often work, instead applying margin to the entry-content.

The columns block now also seems to have a very similar bug with a single column - #26538

@tellthemachines
Copy link
Contributor

This doesn't help unfortunately

Setting the margin to 0 should fix it as long as you don't set the width to 100%. This bug appeared because the margins on the table element were set to auto, so it gets centred.

@talldan
Copy link
Contributor Author

talldan commented Oct 29, 2020

@tellthemachines I did remove the 100% width first.

margin: 0 overrides margin-left: auto which makes the block appear on the very left of the screen.

@talldan
Copy link
Contributor Author

talldan commented Oct 29, 2020

Change to editor.scss:

[data-type="core/image"].is-resized {
	margin: 0;
}

Computed styles of the figure:
Screenshot 2020-10-29 at 3 40 32 pm

Result in Twenty Twenty One:
Screenshot 2020-10-29 at 3 40 58 pm

Result with theme styles deactivated:
Screenshot 2020-10-29 at 3 41 16 pm

@jasmussen
Copy link
Contributor

You're both right.

  • If the margins aren't auto, then the wide/fullwide/centered column setup doesn't work.
  • If the margins aren't set to 0, then a resized image is unexpectedly centered.

This is because display: table; overrides a 100% width, which was intentionally chosen so that captions could be the same width as its adjacent image.

In other words, combinging display: table; with width: 100%, on a figure element which is intrinsically born as a block level element is to overdo it.

That's why I think the solution outlined at the bottom of my previous comment is the way forward: only apply the display: table; property on floated blocks. That way we don't have to set either margin or width on the unfloated figure element in order for it to behave.

@carolinan
Copy link
Contributor

It is impossible for me to keep with all the changes, is this related? WordPress/twentytwentyone#737

@jasmussen
Copy link
Contributor

Looks unrelated, carolinan, but I'll respond on the TT1 repo.

@talldan
Copy link
Contributor Author

talldan commented Nov 2, 2020

@jasmussen I've made the change you suggested, let me know what you think.

@jasmussen
Copy link
Contributor

Nice work, I love that DIFF!

Before:

before

After:

after

It looks like the change made the table caption rules somehow not take in the editor. The frontend looks fine. As soon as I apply the following CSS to the figcaption element in the editor, things are good again:

Screenshot 2020-11-02 at 10 15 48

@talldan
Copy link
Contributor Author

talldan commented Nov 3, 2020

@jasmussen Good catch. I hadn't tested with a longer caption. The issue was that the editor handles alignments slightly differently, using a wrapper element with a data attribute, so I had to replicate the table styles for the editor.

It should be working now with the last commit 😄

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

This is working well!

Caption doesn't follow the width of the resized image when unaligned, but I think that was understood to be a separate issue?

Screen Shot 2020-11-03 at 2 38 19 pm

@talldan talldan merged commit 1fbe105 into master Nov 3, 2020
@talldan talldan deleted the fix/image-block-centering branch November 3, 2020 04:33
@talldan
Copy link
Contributor Author

talldan commented Nov 3, 2020

Thanks for the reviews, I think we got there eventually!

@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 3, 2020
@talldan talldan mentioned this pull request Nov 4, 2020
6 tasks
tellthemachines pushed a commit that referenced this pull request Nov 11, 2020
* Fix image block centering when resizing to a smaller size

* Revert previous 100% width fix

* Remove display: table when image block is resized to avoid centering of block

* Match frontend classes for alignment in editor
tellthemachines added a commit that referenced this pull request Nov 12, 2020
* Cover block: Restore default overlay background (#26569)

* Restore default Cover overlay background

The default Cover block overlay background was removed. This changed the
style of existing blocks on existing sites.

Restore the default background in such a way that it does not conflict
with theme-provided background-color styles and there is no need to
artificially add extra specificity.

Fix regression: #26545

* Limit the interface skeleton to a max width of 100% to prevent some blocks managing to exceed the width (#26552)

* Cover Block: Restore opacity controls (#26625)

* Fix image block centering when resizing to a smaller size (#26376)

* Fix image block centering when resizing to a smaller size

* Revert previous 100% width fix

* Remove display: table when image block is resized to avoid centering of block

* Match frontend classes for alignment in editor

* Gallery: Remove caption fallback for alt attribute (#26676)

* Fix quote block default alignment (#26680)

* Gallery: Remove obsolete deprecation entry (#26736)

* Do not apply text color if it is being overriden (#24626)

* Fix: Preset colors don't work on button block style outline (#26707)

* Tests: Add fixture for Column deprecation (#26774)

* Fix/column width units (#26757)

* Fix issues with different units in column widths

* Columns with fixed width should keep width on recalculation

* Address review feedback

* fix undefined index notice in Social Link Block (#25663)

* fix undefined index notice

* use isset instead of array_key_exists check

* Update packages/block-library/src/social-link/index.php

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>

* Adds in missing styling for toolbar when using text-only setting (#26769)

* Adds in missing styling for toolbar when using text-only setting

* Increases margin

* Moves style to correct file

* Inserter: Fix 'Browse All' in Quick Inserter (#26443)

* Inserter: Fix 'Browse All' in Quick Inserter

Maintain the insertion point in @wordpress/block-editor state when
moving from the Quick Inserter to the Inserter.

This fixes a bug where the insertion point is wrong after clicking a
block appender and selecting 'Browse All'.

Care is taken to not regress a previous bug where the insertion point is
wrong after clicking an inline insertion button and selecting 'Browse
ALl'.

* Inserter: Fix typo

Co-authored-by: Daniel Richards <daniel.richards@automattic.com>

* Call getBlockInsertionPoint once

* Add useSelect dependencies

* Make setInsertionPoint unstable

* Avoid setting `isVisible` state when `SET_INSERTION_POINT` is triggered

* Make index required and clarify rootClientId usage

* Split insertionPoint into two reducers

* Fix getInsertionPoint selector that expects default state of reducer to be null

* Avoid resetting insertionPoint state on HIDE_INSERTION_POINT

Co-authored-by: Daniel Richards <daniel.richards@automattic.com>

Co-authored-by: Jon Surrell <jon.surrell@automattic.com>
Co-authored-by: Jacopo Tomasone <Copons@users.noreply.github.com>
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
Co-authored-by: Bernie Reiter <ockham@raz.or.at>
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
Co-authored-by: Oliver Juhas <webmandesign@users.noreply.github.com>
Co-authored-by: Jorge Costa <jorge.costa@automattic.com>
Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de>
Co-authored-by: Tammie Lister <tammie@automattic.com>
Co-authored-by: Robert Anderson <robert@noisysocks.com>
@tellthemachines tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Regression Related to a regression in the latest release
Projects
None yet
4 participants