Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#51157 closed defect (bug) (fixed)

Bundled Themes: List block appears too far to the left after 5.5 upgrade

Reported by: kjellr's profile kjellr Owned by: sabernhardt's profile sabernhardt
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-screenshots needs-testing dev-feedback has-patch
Focuses: css, rtl Cc:

Description (last modified by SergeyBiryukov)

After upgrading to WordPress v5.5, the list block appears too far to the left in the editor in some themes:

  • Twenty Fourteen
  • Twenty Sixteen
  • Twenty Seventeen

(Possibly a couple more, but we should test and verify)

This was fixed in Twenty Fifteen earlier this year, so these themes likely need a similar fix: #50029

More details in this GitHub issue:
https://github.com/WordPress/gutenberg/issues/24780


http://cldup.com/LkCxBQaH8Q.jpg

http://cldup.com/cG4MIyW3wJ.png

Attachments (20)

51157.patch (2.7 KB) - added by amolv 4 years ago.
Default List margin removed from theme.
Capture d’écran 2020-09-19 à 10.28.58.png (157.4 KB) - added by audrasjb 4 years ago.
After the patch (Twenty Eleven example)
Twenty-Fifteen-RTL-first-patch.png (151.5 KB) - added by sabernhardt 4 years ago.
Editor with various lists in Twenty Fifteen, in Arabic
Twenty-Eleven-RTL-first-patch.png (102.4 KB) - added by sabernhardt 4 years ago.
Editor with various lists in Twenty Eleven, in Arabic
51157.1.patch (7.0 KB) - added by amolv 4 years ago.
Patch refresh, RTL language support.
RTL language support.png (643.4 KB) - added by amolv 4 years ago.
Test after patch refresh.
2010-UL-TwentyTen.png (639.2 KB) - added by francina 4 years ago.
TwentyTen
2011-UL-TwentyEleven.png (784.4 KB) - added by francina 4 years ago.
Twenty Eleven Screenshot
2012-UL-TwentyTwelve.png (196.1 KB) - added by francina 4 years ago.
Twenty Twelve Screenshot
2013-UL-TwentyThirteen.png (688.3 KB) - added by francina 4 years ago.
Twenty Thirteen
2014-UL-TwentyFourteen.png (426.7 KB) - added by francina 4 years ago.
Twenty Fourteen
2015-UL-TwentyFifteen.png (621.8 KB) - added by francina 4 years ago.
Twenty Fifteen
2016-UL-TwentySixteen.png (519.4 KB) - added by francina 4 years ago.
Twenty Sixteen
2017-UL-TwentySeventeen.png (1.3 MB) - added by francina 4 years ago.
Twenty Seventeen
2019-UL-TwentyNineteen.png (486.7 KB) - added by francina 4 years ago.
Twenty Nineteen
2020-UL-TwentyTwenty.png (701.0 KB) - added by francina 4 years ago.
Twenty Twenty
example-posts-with-lists.xml (15.2 KB) - added by sabernhardt 4 years ago.
sample posts for both LTR and RTL list content
2020-nested-list-rtl.png (12.0 KB) - added by sabernhardt 4 years ago.
nested unordered list in a list block, Twenty Twenty, RTL
51157.2.patch (10.5 KB) - added by sabernhardt 4 years ago.
indenting nested lists and normalizing classic block lists
screenshots-after-51157.2.patch.zip (2.0 MB) - added by sabernhardt 4 years ago.
editor screenshots after 51157.2.patch

Change History (45)

#1 @SergeyBiryukov
4 years ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 5.5.2

#2 @sabernhardt
4 years ago

  • Focuses css added

@amolv
4 years ago

Default List margin removed from theme.

#3 @amolv
4 years ago

  • Keywords has-patch added; needs-patch removed

twentyten, twentyeleven, twentythirteen, twentyfourteen, twentysixteen, twentyseventeen - in this themes, default margin of list is removed like #50029

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

@audrasjb
4 years ago

After the patch (Twenty Eleven example)

#4 @audrasjb
4 years ago

  • Keywords has-screenshots added; needs-testing removed

I tested the proposed patch and it fixes the issue for each related bundled theme.
Marking this for commit.

#5 @audrasjb
4 years ago

  • Keywords commit added

#6 @sabernhardt
4 years ago

  • Focuses rtl added
  • Keywords needs-patch added; has-patch commit removed

The patch looks good with regular list blocks in LTR languages, but it still needs some adjustments for RTL support (even Twenty Fifteen).

@sabernhardt
4 years ago

Editor with various lists in Twenty Fifteen, in Arabic

@sabernhardt
4 years ago

Editor with various lists in Twenty Eleven, in Arabic

@amolv
4 years ago

Patch refresh, RTL language support.

@amolv
4 years ago

Test after patch refresh.

#7 @amolv
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

CSS updated for RTL language support.

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


4 years ago

@francina
4 years ago

TwentyTen

@francina
4 years ago

Twenty Eleven Screenshot

@francina
4 years ago

Twenty Twelve Screenshot

@francina
4 years ago

Twenty Thirteen

@francina
4 years ago

Twenty Fourteen

@francina
4 years ago

Twenty Fifteen

@francina
4 years ago

Twenty Sixteen

@francina
4 years ago

Twenty Seventeen

@francina
4 years ago

Twenty Nineteen

@francina
4 years ago

Twenty Twenty

#9 @francina
4 years ago

Tested on a 16" Retina display, only LTR. The patch applies cleanly.
Twenty Fifteen and Twenty Seventeen seem to have very little margin on their left, so they are not consistent with the rest of the themes.

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


4 years ago

@sabernhardt
4 years ago

sample posts for both LTR and RTL list content

#11 @sabernhardt
4 years ago

Thanks for testing. Unfortunately, the patch is for editor styles, not for the front end. And while I would prefer having a margin to indent the lists, both Twenty Fifteen (at larger screen sizes) and Twenty Seventeen have had no left or right margin since each theme was released. Reconsidering that now would require a separate ticket and plenty of testing.

For further testing here, if anyone would like to import some starter/example posts, example-posts-with-lists.xml contains my lists for both LTR and RTL directions (I used Arabic, with Polylang plugin). Each post features simple ordered and unordered lists, plus nested lists (including mixed types). Then the same set of lists is repeated inside a full-width group block and a classic block. Other possible situations could be tested as well.

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

@sabernhardt
4 years ago

nested unordered list in a list block, Twenty Twenty, RTL

#12 @sabernhardt
4 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

Twenty Twelve is fine with or without the patch.

All 7 of the themes in the patch now center simple lists in list blocks in both LTR and RTL languages. However, only Twenty Eleven shows nested lists in RTL list blocks properly, and Twenty Thirteen has trouble with nested lists in both LTR and RTL languages. Most themes have problems with nested lists and/or inconsistent list indentation in the classic block, too.

Also, the lack of any margin or padding on outer lists can result in cutting off the list markers at narrower widths. (This might be better to address within Gutenberg's styles, adding more than the 10px padding on each side.)

Twenty Nineteen and Twenty Twenty were not included in the patch. Edit: I'll open separate tickets for small fixes on those.

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

#13 @francina
4 years ago

  • Keywords needs-testing added

@sabernhardt my understanding from your last comment is:

  • For LTR and RTL non-nested lists, this is good to go and ready for commit
  • Only Twenty Eleven shows nested lists in RTL > Needs new ticket?
  • Twenty Thirteen has trouble with nested lists in both LTR and RTL languages > Needs new ticket?

Paging @ianbelanger for this, to assess if something can be done to get this into 5.6
Thanks!

#14 @sabernhardt
4 years ago

With patch 51157.1.patch (and the previous changeset [47934]), I can't tell if one list is inside another list, and that could be a bigger problem than having the block appear off to the side. So I would suggest not committing yet.

On the other hand:

  1. Twenty Fifteen already implemented this change (with LTR languages).
  2. As far as I know, the patch fixes the issue originally reported on this ticket.
  3. Any further work likely does not involve changing anything in the patch.
  4. Perhaps it's easier to create and test follow-up patches in separate tickets for each theme, instead of updating and testing all themes together, especially if Twenty Nineteen and Twenty Twenty are edited as well.

If you want to commit the main part now, I still would like to have at least most of the follow-up fixes in the same release. Whatever the decision is, I'll go with it.

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


4 years ago

#16 @audrasjb
4 years ago

  • Keywords dev-feedback has-patch added; needs-patch removed

As per today's bug scrub:
@sabernhardt so it looks like your last patch partially addresses the issue. My understanding is: if the current patch is committed, we’ll need some follow-up changes to address new issues, but I’m not sure those issues would be regression after that patch or pre-existing issues.

If the patch introduces some regressions, it’s of course better to directly address them in this ticket. If they are pre-existing issues, we can open another ticket to address them.

Can you clarify the current situation please?

#17 @sabernhardt
4 years ago

My patch was only a suggestion for Twenty Twenty, and I just deleted it to avoid confusion.

Amol's second patch 51157.1.patch centers lists in list blocks as intended—with both LTR and RTL languages—for 7 themes. A side effect is that nested lists are not always indented properly.

I'll see if I can create a patch to fix nested lists before Monday for the same 7 themes.

I can create new tickets for Twenty Nineteen and Twenty Twenty (attaching my previous patch there), for consideration separately. Those two should not delay the others.

@sabernhardt
4 years ago

indenting nested lists and normalizing classic block lists

#18 @sabernhardt
4 years ago

Building off of 51157.1 for Twenty Ten to Twenty Seventeen, 51157.2.patch:

  1. Indents nested lists in list and category blocks.
  2. Matches ordered and unordered indentation for the classic block's lists.

Twenty Seventeen still needed

  1. the left margin removed from RTL nested lists and
  2. ul:not(.wp-block-gallery) to override an earlier padding rule in editor-blocks.css

Twenty Thirteen required a different approach than the others, and now the ordered and unordered lists (in list blocks) have the same indentation as each other. (The full/half-width group block alignment is noted on #51440.)

Indentation values taken from styles.css and/or editor-styles.css:

  • 1.5em Twenty Ten
  • 2.5em Twenty Eleven
  • 40px Twenty Thirteen
  • 20px Twenty Fourteen
  • 23px Twenty Fifteen (from editor-styles.css)
  • 1.5em Twenty Sixteen (from style.css at min-width:44.375em)
  • 1.5em Twenty Seventeen

Screenshots:
https://www.flickr.com/gp/34028247@N03/3A7Z8X

@sabernhardt
4 years ago

editor screenshots after 51157.2.patch

#19 @sabernhardt
4 years ago

Follow-up tickets opened for Twenty Nineteen #51573 and Twenty Twenty #51574

#20 @sabernhardt
4 years ago

#51607 was marked as a duplicate.

#21 @whyisjake
4 years ago

  • Milestone changed from 5.5.2 to 5.5.3

Can we get a unified patch for all themes and all changes? Ideally, we get this merged in the next few hours, but I am going to move to the 5.5.3 milestone.

#22 @sabernhardt
4 years ago

  • Owner set to sabernhardt
  • Status changed from new to accepted

Twenty Nineteen and Twenty Twenty have separate tickets because their issue is not directly related to the list block centering, as is the case for the other 7 themes.

I'm confident 51157.2.patch fixes the original issue of centering because the previous patch did that, and I did not meddle with that part. If someone else is willing to test 51157.2.patch, please check that I didn't make a mistake or a poor choice—or cause new problems—with the extra changes for nested lists.

In addition to the List and Classic blocks in the sample example-posts-with-lists.xml posts, I tested it with Categories, Latest Posts, and Gallery blocks.

It may be too late for 5.5.2, but it's still good to try committing in time for 5.6.

#23 @JeffPaul
4 years ago

  • Milestone changed from 5.5.3 to 5.6

We're working on a short cycle 5.5.3 release in the very near future and have no further minor releases planned so I'm punting this to 5.6 in hopes that the patch can be validated for that major release.

This ticket was mentioned in Slack in #core-css by sabernhardt. View the logs.


4 years ago

#25 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 49511:

Bundled Themes: Correct list block alignment in editor styles.

Additionally:

  • Indent nested lists in list and category blocks.
  • Match ordered and unordered indentation for the classic block's lists.

This applies to:

  • Twenty Ten
  • Twenty Eleven
  • Twenty Thirteen
  • Twenty Fourteen
  • Twenty Fifteen
  • Twenty Sixteen
  • Twenty Seventeen

Props sabernhardt, amolv, kjellr, audrasjb, francina.
Fixes #51157.

Note: See TracTickets for help on using tickets.