Skip to content

Conversation

@florianlacreuse
Copy link
Contributor

Fix #29319

word-break property is mostly use for East Asian languages, for soft wrap opportunities between letters commonly associated with languages like Chinese, Japanese, etc.

When the overflow-wrap property is not working (IE, Edge), we need to use the old word-wrap property as a fallback.

Note: If accepted and merged, this fix should be backported to v4.

@florianlacreuse florianlacreuse requested a review from a team as a code owner August 29, 2019 08:50
@XhmikosR
Copy link
Member

This won't apply clean on v4-dev, we'll need to manually backport it.

Also, note that I have #28304 for months now because of this. We should deal with it once and for all.

@XhmikosR XhmikosR added the css label Aug 29, 2019
@florianlacreuse
Copy link
Contributor Author

florianlacreuse commented Aug 29, 2019

I agree about the backport to v4-dev, I could do it in another PR if needed.

Good point about other components using property to break words. I've gone through the scss files in v5:

  • word-wrap: break-word; is used in: code, .card, .popover and .tooltip.
  • word-break: normal; is used in: reset-text mixin, code inside pre.

word-break: break-all is only used in .text-break (v5). It seems that #28304 is no longer applicable.

It looks like you guys decide to use word-wrap: break-word and not overflow-wrap. Should I remove overflow-wrap and word-break from .text-break?

@MartijnCuppens
Copy link
Member

https://github.com/twbs/bootstrap/blob/7c8a1256238be5a5236aab4c47661e10dbb56c11/scss/_utilities.scss#L426

Do we need 3 properties here? Could you explain why?

We should do some crossbrowser testing to tackle this, I know IE, FF and chrome behave different on each of these properties. @florianlacreuse, could you make a demo with the effects?

@florianlacreuse
Copy link
Contributor Author

First of all, with this PR I had the idea to fix the overflow-wrap issue with IE / Edge without dropping properties or breaking things.

About the 3 properties, it really depends. I read several articles or messages, some recommend to use overflow-wrap and word-wrap, some only use word-wrap. Same with word-break, some use it, some don't.

AFAIK, overflow-wrap and word-wrap are the same, overflow-wrap is the new name and it's the new standard in CSS specs. Unfortunately, some browsers (IE and old Edge) require the legacy name word-wrap to work (they don't know overflow-wrap).

That's why you can see many times overflow-wrap and word-wrap used at the same time, people use overflow-wrap as new standard and also put word-wrap as fallback. That was the original goal of this PR.

Currently we could only use word-wrap and the result will be the same (unless some browsers don't support word-wrap but do support overflow-wrap? - sounds unlikely). As I said in my previous message, Bootstrap v5 only use word-wrap in several components, we can do the same with .text-break, i'm fine with it.

About word-break property: this page from mozilla explains the property with some examples. 1. break-word value is deprecated. 2. break-all value is used for CJK text, but I don't know more about it. In Bootstrap v5, word-break is only used in .text-break with deprecated break-word value. IMHO we should drop it.

@MartijnCuppens https://codepen.io/florianlacreuse/pen/BaBZMdJ

@florianlacreuse
Copy link
Contributor Author

@MartijnCuppens I know you guys are busy but it would be nice if we could chose what to do here. I still think we should drop word-break property, and keep overflow-wrap + word-wrap (or only word-wrap for consistency with other BS components / classes).

@MartijnCuppens
Copy link
Member

@florianlacreuse, I'll have a look at this somewhere this week. Thanks for (re)pinging me here, this is indeed something which should be tackled.

@MartijnCuppens
Copy link
Member

Solid research you did there, @florianlacreuse. Imo we should:

  • Drop word-break, since this doesn't really add anything
  • Also drop overflow-wrap. Although it is the standardised property of word-wrap, there's no difference if have it or not and it's very unlikely browsers will suddenly drop support for word-wrap.

@florianlacreuse, could you change this in your PR and maybe make a separate PR for v4-dev?

@XhmikosR
Copy link
Member

We should use whatever the standard is IMO. Later they might follow it.

@MartijnCuppens
Copy link
Member

We should use whatever the standard is IMO. Later they might follow it.

I agree in most cases, but in this case the property is just completely redundant.

@florianlacreuse
Copy link
Contributor Author

For now, we agree that we should remove word-break property, that's good!

overflow-wrap browsers support : https://caniuse.com/#feat=wordwrap
word-wrap browsers support: https://caniuse.com/#feat=mdn-css_properties_word-wrap

@XhmikosR overflow-wrap is indeed the new standard but it doesn't have a full support in IE11 and Edge < 18. IMHO we should use overflow-wrap and word-wrap (as a fallback) or word-wrap, no strong opinion about these two choices.

@XhmikosR
Copy link
Member

Like I said, we should make sure we support the standard so that we are future-proof. IE is a dead browser and it will just use the workaround fine. The other evergreen browsers don't fall into the same category.

@florianlacreuse
Copy link
Contributor Author

@MartijnCuppens Thank you for your feedback! PR has been updated to remove word-break.

The other issue is about other BS components / css classes using only word-wrap, like:

.card {
position: relative;
display: flex;
flex-direction: column;
min-width: 0; // See https://github.com/twbs/bootstrap/pull/22740#issuecomment-305868106
height: $card-height;
word-wrap: break-word;

.popover {
position: absolute;
top: 0;
left: 0;
z-index: $zindex-popover;
display: block;
max-width: $popover-max-width;
// Our parent element can be arbitrary since tooltips are by default inserted as a sibling of their target element.
// So reset our font and text properties to avoid inheriting weird values.
@include reset-text();
@include font-size($popover-font-size);
// Allow breaking very long words so they don't overflow the popover's bounds
word-wrap: break-word;

bootstrap/scss/_reboot.scss

Lines 281 to 284 in 4e59997

code {
@include font-size($code-font-size);
color: $code-color;
word-wrap: break-word;

bootstrap/scss/_reboot.scss

Lines 414 to 415 in 4e59997

select {
word-wrap: normal;

.tooltip {
position: absolute;
z-index: $zindex-tooltip;
display: block;
margin: $tooltip-margin;
// Our parent element can be arbitrary since tooltips are by default inserted as a sibling of their target element.
// So reset our font and text properties to avoid inheriting weird values.
@include reset-text();
@include font-size($tooltip-font-size);
// Allow breaking very long words so they don't overflow the tooltip's bounds
word-wrap: break-word;

For consistency, we should also use overflow-wrap, no?

@MartijnCuppens
Copy link
Member

@XhmikosR, from the w3c specification (https://www.w3.org/TR/css-text-3/#overflow-wrap-property):

For legacy reasons, UAs must treat word-wrap as an legacy name alias of the overflow-wrap property.

So it's officially more an alias for overflow-wrap.

MartijnCuppens
MartijnCuppens previously approved these changes Jan 2, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Jan 2, 2020

@MartijnCuppens feel free to merge and backport it to a v4-dev branch

florianlacreuse and others added 2 commits January 4, 2020 18:08
While `overflow-wrap` is the more recommanded option, `word-wrap` alone has a wider support.
@MartijnCuppens
Copy link
Member

Discussed this on Slack with @XhmikosR, we agreed on removing overflow-wrap since word-wrap is part of the specs, covers everything overflow-wrap covers and it's not worth adding it to all components @florianlacreuse listed in #29325 (comment).

@MartijnCuppens MartijnCuppens merged commit c7ce627 into twbs:master Jan 4, 2020
@florianlacreuse
Copy link
Contributor Author

Using only word-wrap is fine too, everything is consistent so I'm good. Thanks for your time!

@florianlacreuse florianlacreuse deleted the ft-fix-text-break branch January 4, 2020 19:49
@MartijnCuppens MartijnCuppens mentioned this pull request Jan 23, 2020
XhmikosR pushed a commit that referenced this pull request Jan 23, 2020
`.text-break` fix
XhmikosR pushed a commit that referenced this pull request Jan 28, 2020
`.text-break` fix
XhmikosR pushed a commit that referenced this pull request Feb 4, 2020
`.text-break` fix
XhmikosR pushed a commit that referenced this pull request Feb 16, 2020
`.text-break` fix
XhmikosR pushed a commit that referenced this pull request Feb 17, 2020
`.text-break` fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docs - Word break

3 participants