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

Block Styles: Fix long strings of text without spaces overflow the block #34222

Merged

Conversation

arthur791004
Copy link
Contributor

@arthur791004 arthur791004 commented Aug 23, 2021

Description

Currently, some of the themes set the word-break: break-word; or overflow-wrap: break-word; css to the heading block or paragraph block. For example, Independent Publisher 2 sets word-break: break-word here to .entry-content and heading and paragraph inherit that property. However, some of the themes (Twenty Twenty-One, Blank Canvas and so on) don't set that property so that the multi-line strings without spaces make the pages broken and generate the horizontal scrollbar on mobile devices.

How has this been tested?

  1. Create or Edit a post
  2. Insert the following blocks
    • multi-line strings without spaces as one of following
      • heading
      • paragraph
      • heading or paragraph inside the cover block
      • button
      • list item
      • caption inside the embed video
      • caption inside the Pullquote or quote
  3. Save and view the post on the new tab
  4. Open Chrome Developer Tools and change to mobile view
  5. Check the multi-line strings break the page and generate a horizontal scrollbar or not

Screenshots

Before After

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).

@roo2
Copy link
Contributor

roo2 commented Aug 23, 2021

@arthur791004 one of the automated checks flagged https://github.com/WordPress/gutenberg/pull/34222/files#diff-3721697b4de86118e261dd67207c3022a76cb2882ed34fea6de180206c427601

It looks like the difference is actually line breaks in the returned string, I wonder if the css is actually loaded by the tests and having this effect

@roo2
Copy link
Contributor

roo2 commented Aug 24, 2021

I tested this locally with a8c-wp-env and found that twenty-twentyone and blank canvas weren't demonstrating the issue.

The rule affecting the .block-editor-block-list__layout .block-editor-block-list__block was giving them correct wrapping
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-list/style.scss#L123

.block-editor-block-list__layout .block-editor-block-list__block {
    position: relative;
    overflow-wrap: break-word;
}

Do you have more specific repro instructions?

@roo2
Copy link
Contributor

roo2 commented Aug 24, 2021

original issue: Automattic/wp-calypso#51249

@arthur791004
Copy link
Contributor Author

@roo2 Oops...I'm sorry for that I forgot to say that the problem only happens when you view the post 😅

@ramonjd ramonjd added [Block] Heading Affects the Headings Block [Block] Paragraph Affects the Paragraph Block CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement. labels Aug 24, 2021
@roo2
Copy link
Contributor

roo2 commented Aug 24, 2021

only happens when you view the post

Ahh of course! sorry I missed that. I can reproduce the issue with Headings on the live site but not in the editor, with or without the cover block.
This happened with every theme that I tested.

Before:

Screen Shot 2021-08-25 at 6 54 18 am

I can also confirm that applying this PR fixes the issue ✅

After:

Screen Shot 2021-08-25 at 6 58 54 am

@roo2
Copy link
Contributor

roo2 commented Aug 24, 2021

I found that the issue also affects very long links in lists

  1. Add a list element
  2. Add a very long unbroken link
  3. View in preview/live site on mobile

Screen Shot 2021-08-25 at 7 12 26 am

@arthur791004 arthur791004 force-pushed the fix/long-strings-overflow-the-block branch from cc04984 to 9a7068a Compare August 25, 2021 03:47
@arthur791004 arthur791004 force-pushed the fix/long-strings-overflow-the-block branch from 9a7068a to 690b83e Compare August 25, 2021 03:47
@arthur791004
Copy link
Contributor Author

@roo2 Good catch 👍 I also tested the list before but I found that it works well because I wrap the string into paragraph 😅
I fix the problem in the list, thank you!

Copy link
Contributor

@roo2 roo2 left a comment

Choose a reason for hiding this comment

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

While we're here, there's one more fix we can make. The Quote and Pullquote block both have "Citation" fields, which are currently not wrapping.

Screen Shot 2021-08-27 at 1 09 39 pm

I tested in Chrome Safari and Firefox and can't find any other blocks that need this fix

@@ -14,7 +14,7 @@ $blocks-block__margin: 0.5em;
padding: calc(0.667em + 2px) calc(1.333em + 2px); // The extra 2px are added to size solids the same as the outline versions.
text-align: center;
text-decoration: none;
overflow-wrap: break-word;
word-break: break-word; // overflow-wrap doesn't work well if a link is wrapped in the div, so use word-break here.
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested that buttons needs word-break in order to wrap correctly wheras headings will wrap with overflow-wrap alone (Tested in Chrome). 👍

But I couldn't work out why this is the case. I think that the comment is good for this interesting case. 👍

@arthur791004
Copy link
Contributor Author

While we're here, there's one more fix we can make. The Quote and Pullquote block both have "Citation" fields, which are currently not wrapping.

Good catch 👍 I've fixed it, thank you!

image

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

LGTM 👍. Tested with headings in twentytwentyone. Though I don't know if it has any other side effects.

I'd say let's merge it and observe the impacts in prod.

@kevin940726 kevin940726 merged commit fe1d036 into WordPress:trunk Sep 2, 2021
@github-actions github-actions bot added this to the Gutenberg 11.5 milestone Sep 2, 2021
@arthur791004 arthur791004 deleted the fix/long-strings-overflow-the-block branch September 2, 2021 06:23
@benbreedlove
Copy link

Hey, so sorry to bring this back up, I only just ran into the changes from this branch last week. I wanted to check that you did precisely what you wanted to, here. Changing the default behaviors of all headers, lists, and paragraphs from W3C recommendations is possibly a larger change than you meant to make. Particularly I'm looking at the selectors that target p, h1 through h6, ol and ul globally. Did you perhaps mean for those items to be more particularly targeted? Thanks for your time.

@JakePT
Copy link

JakePT commented Feb 10, 2022

I just ran into an issue caused by this change. This was on a theme that has not adopted FSE or anything like that, so this really seems like an overstep to me, to make this decision on behalf of all themes and affect global styles like this.

@arthur791004
Copy link
Contributor Author

@JakePT Do you have the steps to reproduce your issue?

@JakePT
Copy link

JakePT commented Feb 14, 2022

@arthur791004 Well the issue was just that the theme's CSS did not have overflow-wrap: break-word; on it before the update, and after the update it did.

This particulate site had some large text on desktop in a div that was slightly too small. This wasn't a problem because the text just overflowed a little bit and displayed fine, but after the update the words were broken so that the text could wrap. This looked much much worse.

This change doesn't just affect headings that run out of space on mobile, it affects all headings at all times, including on desktop where text overflowing does not automatically mean that the text is being cropped.

@yaronuliel
Copy link

Agreeing with @JakePT , the css rules apply to global tags h1, h2, h3, h4, h5, h6 and p - and affect nodes that has nothing to do with blocks.
Generally speaking, a library shouldn't apply rules that affect elements beyond the scope of the library. I had a site affected by it in production after upgrading WordPress version

@arthur791004
Copy link
Contributor Author

arthur791004 commented Mar 29, 2022

I'm sorry for the late reply. I totally agree with you all that I should not change the default behavior for p, h1 through h6, ol and ul. I'll revert the change for those elements. See #39846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Heading Affects the Headings Block [Block] Paragraph Affects the Paragraph Block CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants