Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#52287 closed defect (bug) (fixed)

Twenty Twenty One: Inline Images displaying on new lines (as display: block)

Reported by: talldanwp's profile talldanwp Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.6
Component: Bundled Theme Keywords: good-first-bug has-patch commit
Focuses: css Cc:

Description (last modified by SergeyBiryukov)

In Twenty Twenty One, inline images are display: block instead of display: inline-block.

This is the case both in the editor and in the front-end.

There's a similar ticket here for Twenty Twenty that could be worth fixing at the same time: #50418

Steps to reproduce:

  1. Add a paragraph block
  2. Type some text
  3. Add an inline image

Expected behavior:
The image displays on the same row as the text.

Actual behavior:
The image displays on a new line.

Attachments (10)

Inline image test.png (76.2 KB) - added by mukesh27 4 years ago.
Screenshot 2021-01-13 at 4.34.11 pm.png (24.2 KB) - added by talldanwp 4 years ago.
Reporter screenshot
with-pr-874.png (115.1 KB) - added by poena 4 years ago.
With the current PR applied:
Screen Shot 2021-02-03 at 12.18.57.png (56.9 KB) - added by paaljoachim 4 years ago.
Applied the patch. Inline image is still on a separate line.
Inline-image-2021.gif (336.9 KB) - added by paaljoachim 4 years ago.
Working patch for adding an inline image.
52287-vertical-align-middle.patch (27.5 KB) - added by poena 4 years ago.
Display: block is removed, but the alignment is kept
inline-image-vertical-align-middle.png (253.8 KB) - added by poena 4 years ago.
Inline images with vertical-align middle
52287-no-vertical-align.patch (27.2 KB) - added by poena 4 years ago.
Display: block and the alignment is removed
inline-image-no-vertical-align.png (248.9 KB) - added by poena 4 years ago.
Inline images without vertical-align middle
52287.patch (1.0 KB) - added by poena 4 years ago.
Removes display:block and duplicate CSS.

Download all attachments as: .zip

Change History (32)

#1 @mukesh27
4 years ago

In my test on admin inline image working fine.

#2 @talldanwp
4 years ago

@mukesh27 Is that Twenty Twenty One? It doesn't look like the same theme, but granted you might have changed some options.

I'll upload an example of what I see.

@talldanwp
4 years ago

Reporter screenshot

This ticket was mentioned in PR #874 on WordPress/wordpress-develop by gonzalezlopezjm.


4 years ago
#3

  • Keywords has-patch added; needs-patch removed

Prevents that an inline image inserted into editor goes into new line in the TwentyTwentyOne Theme

Trac ticket: https://core.trac.wordpress.org/ticket/52287

#4 @poena
4 years ago

Hi!
I have tested the pull request. I did not see any side effects except the following:

There is a style in the image block file that affects all images, not only the block: sass/05-blocks/image/_style.scss#L41

This is duplicating the height and the max width that is already added in the sass/04-elements/media.scss file,
and it is adding a vertical-align:middle to the images on the front.
Because of the vertical alignment, the inline images do not look the same in the editor and front when the PR is applied.

I do not know the reason why the vertical alignment was included, further testing is needed to see if removing it has any side effects.

The left image is in the editor, right is on the front.

Last edited 4 years ago by poena (previous) (diff)

@poena
4 years ago

With the current PR applied:

#5 @SergeyBiryukov
4 years ago

  • Description modified (diff)

#6 @mukesh27
4 years ago

  • Keywords needs-testing added

Agree with @poena either we have to remove that vertical alignment from the front CSS or we have to include vertical alignment in editor CSS to make the same style on both front and editor.

needs-testing keyword added for more testing.

#7 follow-up: @paaljoachim
4 years ago

Tested a local site with Twenty Twenty theme.
Added some text to a paragraph block.
Added an inline image.
Noticed it showed up on a separate line.

Testing the patch.

Command:

npm run grunt patch:https://github.com/WordPress/wordpress-develop/pull/874

I noticed no difference from before and after applying the above.

I also added

npm run build

To see if that would help. No difference.
I assume I am doing something wrong.

#8 in reply to: ↑ 7 @poena
4 years ago

Replying to paaljoachim:

Tested a local site with Twenty Twenty theme.
Added some text to a paragraph block.
Added an inline image.
Noticed it showed up on a separate line.

Testing the patch.

Command:

npm run grunt patch:https://github.com/WordPress/wordpress-develop/pull/874

I noticed no difference from before and after applying the above.

I also added

npm run build

To see if that would help. No difference.
I assume I am doing something wrong.

This ticket is for Twenty Twenty-One, not Twenty Twenty, but maybe the above was a typo?

#9 @paaljoachim
4 years ago

Hi @poena

That is correct. It was a typo. I am testing with Twenty Twenty One. I am retesting now and applied the patch and even rebuilt the local environment.

I write some text and in between the text add an inline image.

This is what I see.

Last edited 4 years ago by paaljoachim (previous) (diff)

@paaljoachim
4 years ago

Applied the patch. Inline image is still on a separate line.

#10 @poena
4 years ago

Did you run npm build inside the twentytwentyone folder?

If you select the image in the editor and look at the source (in the browser dev tools),
does it say

.editor-styles-wrapper img {
    display: inline-block;
    height: auto;
    max-width: 100%;
}
Last edited 4 years ago by poena (previous) (diff)

#11 @paaljoachim
4 years ago

Went into:

cd wordpress-develop

Added:

git reset --hard

Added:

npm run grunt patch:https://github.com/WordPress/wordpress-develop/pull/874

Added:

npm run dev

Checked http://localhost:8889/
Noticed that the image even a very small one was added to a seperate line.

Checking the Chrome console.
It did not say inline-block. Which means I have not been able to test the patch yet.

CSS seen:

.editor-styles-wrapper img {
    display: block;
    height: auto;
    max-width: 100%;
}

---

Logged out of the localhost dev environment site.
Next back in terminal. I added:

npm run build:dev

Hmm I am still not able to successfully test the patch.
@hellofromtonya
Any thoughts?

This ticket was mentioned in Slack in #core by paaljoachim. View the logs.


4 years ago

#13 @paaljoachim
4 years ago

With help from @desrosj and @davidb in the core slack channel I got the patch working.
I added the instructions for testing 2019, 2020 and 2021 here:
https://meta.trac.wordpress.org/ticket/5581#comment:3

@paaljoachim
4 years ago

Working patch for adding an inline image.

#14 @jeroenrotty
4 years ago

I've tested the patch for Twenty Twenty One and can confirm together with @rolfsiebers that the inline-block patch does work as intended. Had some trouble finding the correct steps to build the theme as well, but found the correct steps thanks to @paaljoachim.

Last edited 4 years ago by jeroenrotty (previous) (diff)

This ticket was mentioned in Slack in #core by francina. View the logs.


4 years ago

#16 @poena
4 years ago

  • Keywords needs-refresh added

If I understand correctly the current patch has been tested, but not whether the
vertical-align:middle in comment #4 can be safely removed.

I will mark this as needs refresh.

@poena
4 years ago

Display: block is removed, but the alignment is kept

@poena
4 years ago

Inline images with vertical-align middle

@poena
4 years ago

Display: block and the alignment is removed

@poena
4 years ago

Inline images without vertical-align middle

#17 @poena
4 years ago

So, decisions, which one do we prefer. @melchoyce, any thoughts?

#18 @melchoyce
4 years ago

I personally think centered is a bit less awkward looking.

@poena
4 years ago

Removes display:block and duplicate CSS.

#19 @poena
4 years ago

  • Keywords commit added; needs-testing needs-refresh removed

Patch 52287.patch is a cleaner patch for inline images with vertical-align: middle, and only includes the changes to the scss files, not the built files.

#20 @peterwilsoncc
4 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 50351:

Twenty Twenty-One: Display inline-images inline.

Remove display: block property from inline images and display them center aligned vertically.

Props jeroenrotty, melchoyce, mukesh27, paaljoachim, poena, talldanwp.
Fixes #52287.

This ticket was mentioned in Slack in #core by monikarao. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.