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.

Sign up for GitHub

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

Jump to bottom

Fix text wrapping in Firefox. #7472

Merged
merged 3 commits into from
Jun 26, 2018
Merged

Fix text wrapping in Firefox. #7472

merged 3 commits into from
Jun 26, 2018

Conversation

jasmussen
Copy link
Contributor

This PR, maybe, fixes #6049. CC: @SuperGeniusZeb.

It is a one line code change, but this change is put into a highly unscoped and generic location, so I would appreciate both lots of testing and sanity checking that there are no adverse side-effects to doing this.

For reference, this is what we're using: https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-wrap
We could also have used https://developer.mozilla.org/en-US/docs/Web/CSS/word-break

Screenshot:

screen shot 2018-06-22 at 13 01 08

Fix text wrapping in Firefox.
d194326 
This PR, maybe, fixes #6049. CC: @SuperGeniusZeb.

It is a one line code change, but this change is put into a highly unscoped and generic location, so I would appreciate both lots of testing and sanity checking that there are no adverse side-effects to doing this.

For reference, this is what we're using: https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-wrap
We could also have used https://developer.mozilla.org/en-US/docs/Web/CSS/word-break
@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Jun 22, 2018
@jasmussen jasmussen self-assigned this Jun 22, 2018
@jasmussen jasmussen requested a review from a team June 22, 2018 11:03
@jasmussen jasmussen added this to the 3.2 milestone Jun 22, 2018
@afercia
Copy link
Contributor

afercia commented Jun 22, 2018

The only potential side-effect I can think of is about the screen-reader-text class. Thankfully, it uses word-wrap: normal !important; which overrides overflow-wrap: break-word;. Am I correct in saying word-wrap is an alias, right?

Without that, overflow-wrap: break-word; would make any word inside the 1 by 1 pixel wide screen-reader-text element break and basically the text would by laid out "vertically" without spaces. Screen readers would announce the text without spaces, see below (I've removed word-wrap from the screen reader text class for testing purposes):

screenshot 9

So the screen-reader-text class should be safe. However, wouldn't be possible to apply it on some more scoped element? The list of bocks for example? At least, we could exclude the WordPress UI.

Aside:
what if a theme doesn't use overflow-wrap: break-word;? Wouldn't user see the preview different form the front-end?

afercia
afercia reviewed Jun 22, 2018
View reviewed changes
Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

Please see comment.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Jun 22, 2018 •
edited
Loading

@jasmussen
I can confirm this fixes the issue on Firefox. (Tested on Firefox Beta.)

As a side note, I noticed a couple interesting things which I think may be intentional and are likely not related to this PR at all but I thought I would point out anyway just in case:

  • Verse blocks get a scrollbar whenever they overflow. Even multiple separate words will not break to a new line. I would guess that this is not desirable behavior?
  • Preformatted blocks break text like most other blocks (except the aforementioned Verse block). I am not sure if this is desirable behavior or not, given the purpose of the block.
  • Table blocks get a scrollbar when they overflow. This seems reasonable to me.

@afercia
word-wrap is indeed an alias for overflow-wrap. See also:
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-wrap

what if a theme doesn't use overflow-wrap: break-word;? Wouldn't user see the preview different form the front-end?

If a theme does not set a value for overflow-wrap, then the value will be determined by the browser. The browser defaults vary, hence this issue. A theme would be able to override the Gutenberg editor styling with an explicit overflow-wrap: normal or overflow-wrap: break-word, so given that there is no real default value common to all browsers, Gutenberg should be opinionated here and use the more UI/UX-friendly option by default, with themes being able to override that styling if desired.

@jasmussen
Copy link
Contributor Author

For now I've scoped to the block list class, and it's working for me in Firefox. Let me know if that works for you, and editor styles!

@mcsf
Copy link
Contributor

mcsf commented Jun 25, 2018

Testing on Firefox Dev Ed, this looks good to me. Haven't found regressions on Firefox or Chrome.

@ZebulanStanphill
Copy link
Member

Tested on Firefox Beta, and everything seems to be working for me.

@jasmussen
Copy link
Contributor Author

Cool, can anyone approve this then I can merge and we can test.

@tofumatt tofumatt self-requested a review June 26, 2018 09:28
@tofumatt
Copy link
Member

Reviewing now...

tofumatt
tofumatt approved these changes Jun 26, 2018
View reviewed changes
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Looks good, I'm just gonna update the branch with a comment tweak then we're 👍

editor/components/block-list/style.scss Outdated
@@ -136,6 +136,9 @@
padding-left: $block-padding;
padding-right: $block-padding;

// Break long spaceless words so they don't overflow the block.
overflow-wrap: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

This is totally correct for Firefox; we had to add this to plenty of places in the Add-ons website.

I forget if Firefox is doing the "correct", but annoying, thing here (it feels like it), but yeah… this comes up a lot 😄

editor/components/block-list/style.scss Outdated
@@ -136,6 +136,9 @@
padding-left: $block-padding;
padding-right: $block-padding;

// Break long spaceless words so they don't overflow the block.
Copy link
Member

Choose a reason for hiding this comment

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

I'm gonna add a comma after long here, because it should have one 😄

@tofumatt
Copy link
Member

Comment tweak pushed, merge when green! 👍

@jasmussen jasmussen merged commit a56c54d into master Jun 26, 2018
@jasmussen jasmussen deleted the fix/6049 branch June 26, 2018 09:46
oxyc added a commit to generoi/gutenberg that referenced this pull request Jun 26, 2018
@oxyc
Merge branch 'master' of https://github.com/WordPress/gutenberg
118219b 
* 'master' of https://github.com/WordPress/gutenberg: (69 commits)
  fix: Show permalink editor in editor (WordPress#7494)
  Fix text wrapping in Firefox. (WordPress#7472)
  Try another approach to fixing the sibling inserter in Firefox (WordPress#7530)
  fix: Improve "add block" text in NUX onboarding (WordPress#7511)
  Implement core style of including revisions data on Post response (WordPress#7495)
  Testing: Add e2e test for PluginPostStatusInfo (WordPress#7284)
  Add end 2 end test for sidebar behaviours on mobile and desktop. (WordPress#6877)
  Only save metaboxes when it's not an autosave (WordPress#7502)
  Fix broken links in documentation (WordPress#7532)
  Remove post type 'viewable' compatibility shim (WordPress#7496)
  Fix typo. (WordPress#7528)
  Blocks: Remove wrapping div from paragraph block (WordPress#7477)
  fix: change import for InnerBlocks (WordPress#7484)
  Polish library just a teeeeensy bit (WordPress#7522)
  feat: Add snapshot update script (WordPress#7514)
  Display server error message when one exists (WordPress#7434)
  Fix issues with gallery in IE11. (WordPress#7465)
  Polish region focus style (WordPress#7459)
  Fix IE11 formatting toolbar visibility (WordPress#7413)
  Update plugin version to 3.1. (WordPress#7402)
  ...
@ZebulanStanphill ZebulanStanphill mentioned this pull request Jun 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afercia afercia afercia left review comments

@tofumatt tofumatt tofumatt approved these changes

Assignees

@jasmussen jasmussen

Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
3.2
Development

Successfully merging this pull request may close these issues.

Text in various blocks does not break/wrap at block boundaries (Gecko/Firefox default styling issue)
5 participants
@jasmussen @afercia @ZebulanStanphill @mcsf @tofumatt