WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 7 months ago

#46704 closed defect (bug) (fixed)

Twenty Nineteen: Long non-breaking text strings can cause horizontal scrolling

Reported by: ianbelanger Owned by: ianbelanger
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch has-screenshots commit
Focuses: Cc:
PR Number:

Description

While fixing #36346 I noticed that all Bundled Themes have some sort of word-wrap issues when using long non-breaking text strings. While this might be an edge case issue, I believe that it warrants fixing. Also, there doesn't seem to be any backwards compatibility issues.

In order to better track this issue in each theme, I am separating #36346 into separate tickets per theme.

Attachments (10)

46704.diff (3.8 KB) - added by ianbelanger 8 months ago.
Fixes word-wrap issues
46704-twenty-nineteen-site-title-before.PNG (30.9 KB) - added by ianbelanger 8 months ago.
.site-title before in chrome
46704-twenty-nineteen-site-title-after.PNG (35.9 KB) - added by ianbelanger 8 months ago.
.site-title after in chrome
46704-twenty-nineteen-comments-before.PNG (26.0 KB) - added by ianbelanger 8 months ago.
.comments before in Chrome
46704-twenty-nineteen-comments-after.PNG (31.2 KB) - added by ianbelanger 8 months ago.
.comments after in Chrome
46704-twenty-nineteen-widgets-before.PNG (58.1 KB) - added by ianbelanger 8 months ago.
.widgets before in chrome
46704-twenty-nineteen-widgets-after.PNG (89.0 KB) - added by ianbelanger 8 months ago.
.widgets after in chrome
46704-twenty-nineteen-footer-before.PNG (6.9 KB) - added by ianbelanger 8 months ago.
.footer before in chrome
46704-twenty-nineteen-footer-after.PNG (7.3 KB) - added by ianbelanger 8 months ago.
.footer after in chrome
46704.1.diff (5.7 KB) - added by ianbelanger 7 months ago.
Updated patch

Download all attachments as: .zip

Change History (25)

@ianbelanger
8 months ago

Fixes word-wrap issues

#1 @ianbelanger
8 months ago

  • Keywords has-patch needs-testing added

46704.diff adds

-webkit-hyphens: auto;
-moz-hyphens: auto;
hyphens: auto;
word-wrap: break-word;

to these selectors:

.site-branding,
.site-info,
.comments-area,
#colophon .widget-column .widget

and word-wrap: break-word to

.post-navigation .post-title,
.entry-title,
.not-found .page-title,
.error-404 .page-title,
.comments-title,
blockquote

to prevent horizontal scrolling when long non-breaking text strings are used.

However, hyphens: auto; currently only works in Firefox. Unfortunately nothing worked for me in Chrome, IE11 or Edge.

This patch was tested on a Windows 10 machine, in Firefox, Chrome, IE11 and Edge browsers

Testing in other OS's and browsers would be appreciated.

@ianbelanger
8 months ago

.site-title before in chrome

@ianbelanger
8 months ago

.site-title after in chrome

@ianbelanger
8 months ago

.comments before in Chrome

@ianbelanger
8 months ago

.comments after in Chrome

@ianbelanger
8 months ago

.widgets before in chrome

@ianbelanger
8 months ago

.widgets after in chrome

@ianbelanger
8 months ago

.footer before in chrome

@ianbelanger
8 months ago

.footer after in chrome

#2 @ianbelanger
8 months ago

  • Keywords has-screenshots added
  • Milestone changed from Awaiting Review to Future Release

#3 @ianbelanger
8 months ago

  • Milestone changed from Future Release to 5.2

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


7 months ago

#5 @JeffPaul
7 months ago

Per input in today's bugscrub, @dswebsme looking to test this on a Mac and update later today.

#6 @dswebsme
7 months ago

  • Keywords needs-testing removed

Patch looks good on Mac.

Tested in Chrome, Safari and Firefox.

Tested Site Title, Site Description, Post Title, Page Title, Content and Comments with non-breaking strings.

Mac results reflect AFTER screenshots captured by @ianbelanger.

#7 @mukesh27
7 months ago

#46888 was marked as a duplicate.

#8 @mukesh27
7 months ago

@ianbelanger Patch looks good on Ubuntu 18.04.2 LTS.

Tested in Google Chrome Version 73.0.3683.75 and Firefox Version 65.0.1

#9 @ianbelanger
7 months ago

  • Keywords commit added

Thanks for testing @dswebsme and @mukesh27.

I'm marking this for commit

#10 @SergeyBiryukov
7 months ago

Just wanted to note that there are some previous discussions on long non-breaking text strings in bundled themes: #25008, #25232, #29971, all closed as wontfix due to being an edge case. Should we revisit those discussions before making a decision on commit here?

#11 @laurelfulford
7 months ago

I added a comment to #36346 re: the previous tickets closed as wontfix -- it seemed like it'd be easiest to centralize that discussion in one spot. Any feedback/thoughts would be appreciated there!

#12 follow-up: @laurelfulford
7 months ago

Thanks for tackling this, @ianbelanger! I noticed a couple things while reviewing 46704.diff :

  • The RTL stylesheet needs to be regenerated as well with these changes (it's the one default theme that doesn't load the default style.css with the RTL styles). This can be done by running npm install and npm run build inside of the theme's folder.
  • In headings.scss, .post-navigation .post-title, .entry-title, .not-found .page-title... already has word-break: break-word -- I don't think the word-wrap: break-word; is needed there, too (unless I'm missing something!).
  • It looks like Edge and IE 10-11 will also support hyphens with the -ms- prefix -- do you think it's worth adding that as well?

#13 in reply to: ↑ 12 @ianbelanger
7 months ago

  • Keywords needs-refresh added

Replying to laurelfulford:

Thanks for tackling this, @ianbelanger! I noticed a couple things while reviewing 46704.diff :

  • The RTL stylesheet needs to be regenerated as well with these changes (it's the one default theme that doesn't load the default style.css with the RTL styles). This can be done by running npm install and npm run build inside of the theme's folder.

Oops, I forgot about that. I'll refresh the patch after building the theme.

  • In headings.scss, .post-navigation .post-title, .entry-title, .not-found .page-title... already has word-break: break-word -- I don't think the word-wrap: break-word; is needed there, too (unless I'm missing something!).

The reason for adding word-wrap: break-word; was to appease IE and Edge. word-break: break-word; does not work with them.

  • It looks like Edge and IE 10-11 will also support hyphens with the -ms- prefix -- do you think it's worth adding that as well?

Yes I do, good catch! I will add them in the updated patch, coming soon.

@ianbelanger
7 months ago

Updated patch

#14 @laurelfulford
7 months ago

  • Keywords needs-refresh removed

Ah, thanks @ianbelanger! That makes sense to me, and 46704.1.diff looks good!

#15 @laurelfulford
7 months ago

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

In 45258:

Twenty Nineteen: Prevent too-long strings from causing horizontal scrolling.

Add hyphen and word-wrap styles to the site title, comments, and widget areas to break too-long strings and prevent horizontal scrolling.

Props ianbelanger, dswebsme, mukesh27.
Fixes #46704.

Note: See TracTickets for help on using tickets.