Problem/Motivation

Long strings within comments break the layout of the page.

How to replicate:

Make a comment with very long line (like ______________________) or a long url

It will break the layout of the comment boxes.

Example screenshots:


 

 

 

Current code at HEAD for the .comment__content wrapper

.comment__content {
  position: relative;
  display: table-cell;
  padding: 10px 25px 10px 25px;
  vertical-align: top;
  width: 100%;
  border: 1px solid #d3d7d9;
  font-size: 0.929em;
  line-height: 1.6;
}

Proposed resolution

Add a fix so that long strings wrap within the .comment__content container.

Remaining tasks

Write a patch
Visual review - Post screenshots
Code review

User interface changes

Fixes a broken page layout caused by long strings in comments.

API changes

none

Data model changes

none

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because strings in comments can break the page layout of Bartik - especially on mobile
Issue priority Normal because it is a visual bug with Bartik only.
Unfrozen changes Unfrozen because it only changes CSS.
Prioritized changes The main goal of this issue is usability. Currently if someone adds a long URL to a comment the site layout breaks on smaller screens.
Disruption Non-disruptive
CommentFileSizeAuthor
#48 long-comments-1085472-35.patch396 bytesemma.maria
#45 core-strings-break-bartiks-page-layout-1085472-8-44.patch406 bytesmartins.kajins
#43 Screen Shot 2015-09-07 at 21.49.51.png122.93 KBemma.maria
#43 Screen Shot 2015-09-07 at 21.49.31.png65.1 KBemma.maria
#43 Screen Shot 2015-09-07 at 21.45.48.png69.44 KBemma.maria
#43 Screen Shot 2015-09-07 at 21.45.00.png66.18 KBemma.maria
#35 long-comments-1085472-35.patch396 bytesrudraram
#33 IMG_4568.jpg56.46 KBemma.maria
#33 Screen Shot 2015-09-07 at 14.33.22.png58.27 KBemma.maria
#33 Screen Shot 2015-09-07 at 14.26.37.png38.15 KBemma.maria
#33 Screen Shot 2015-09-07 at 14.26.31.png45.55 KBemma.maria
#32 Screen Shot 2015-09-07 at 14.20.01.png46.28 KBemma.maria
#32 Screen Shot 2015-09-07 at 14.20.09.png47.3 KBemma.maria
#32 Screen Shot 2015-09-07 at 14.17.56.png52.09 KBemma.maria
#30 Test D8 Contrib.png54.29 KBrudraram
#28 Screen Shot 2015-09-01 at 15.15.03.png59.96 KBemma.maria
#23 Word-wrap_comments_in_Bartik-1085472-7.patch379 bytescorbacho
#11 Word-wrap_comments_in_Bartik_0.patch379 byteskpun
#7 Word-wrap_comments_in_Bartik.patch377 byteskpun
#5 asdf - drupal 8.1.x 2015-07-30 22-25-26.png48.33 KBcorbacho
#4 Word-wrap_comments_in_Bartik.patch1.12 KBkpun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

corbacho’s picture

corbacho CreditAttribution: corbacho commented

I removed that fix because wasn't working in Google Chrome.

tatianataylor’s picture

tatianataylor CreditAttribution: tatianataylor commented

Corbacho,

Would you be able to say which .inc file or module was this code at? I am trying to find where is the issue in the code first so maybe I can create a patch.

Thanks,
T.

kpun’s picture

kpun CreditAttribution: kpun as a volunteer and commented
Assigned: Unassigned » kpun
Issue summary: View changes
kpun’s picture

kpun CreditAttribution: kpun as a volunteer and commented
Status: Active » Needs review
FileSize
Word-wrap_comments_in_Bartik.patch1.12 KB

@Corbacho i think the changes you have mentioned should be made in the .comment section .

corbacho’s picture

corbacho CreditAttribution: corbacho commented
Version: 7.x-dev » 8.1.x-dev
Issue tags: +CSS novice
FileSize
asdf - drupal 8.1.x 2015-07-30 22-25-26.png48.33 KB

Hi kpun, I can't replicate anymore the bug in Drupal 7. It seems it has been fixed with the solution you are proposing.
btw, The patch contains changes to the menu.module that shouldn't be there.

I was going to close the issue... but I can replicate the problem in Drupal 8 (changing issue metadata)

screenshot

Could you add these 3 lines to Drupal 8 ? I tested on Chrome devtools, and it worked.

.comment {
    display: table;
    table-layout: fixed;
    width: 100%;
}
corbacho’s picture

corbacho CreditAttribution: corbacho commented
Issue summary: View changes
kpun’s picture

kpun CreditAttribution: kpun as a volunteer and commented
FileSize
Word-wrap_comments_in_Bartik.patch377 bytes

done

Status: Needs review » Needs work

The last submitted patch, 7: Word-wrap_comments_in_Bartik.patch, failed testing.

Status: Needs work » Needs review

kpun queued 7: Word-wrap_comments_in_Bartik.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: Word-wrap_comments_in_Bartik.patch, failed testing.

kpun’s picture

kpun CreditAttribution: kpun as a volunteer and commented
FileSize
Word-wrap_comments_in_Bartik_0.patch379 bytes
kpun’s picture

kpun CreditAttribution: kpun as a volunteer and commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: Word-wrap_comments_in_Bartik_0.patch, failed testing.

kpun’s picture

kpun CreditAttribution: kpun as a volunteer and commented

corbacho could you please tell why the patch does not work ? I am not able to figure it out.

corbacho’s picture

corbacho CreditAttribution: corbacho commented
Status: Needs work » Needs review

Thanks for the patch!
it seems a random error, not related to the patch. Let's try again

emma.maria queued 11: Word-wrap_comments_in_Bartik_0.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: Word-wrap_comments_in_Bartik_0.patch, failed testing.

Status: Needs work » Needs review

corbacho queued 11: Word-wrap_comments_in_Bartik_0.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: Word-wrap_comments_in_Bartik_0.patch, failed testing.

corbacho’s picture

corbacho CreditAttribution: corbacho commented

me not understand

Status: Needs work » Needs review

nlisgo queued 11: Word-wrap_comments_in_Bartik_0.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: Word-wrap_comments_in_Bartik_0.patch, failed testing.

corbacho’s picture

corbacho CreditAttribution: corbacho commented
Status: Needs work » Needs review
FileSize
Word-wrap_comments_in_Bartik-1085472-7.patch379 bytes

let's try re-uploading patch #7 from kpun (credit to him)

Status: Needs review » Needs work

The last submitted patch, 23: Word-wrap_comments_in_Bartik-1085472-7.patch, failed testing.

Status: Needs work » Needs review

emma.maria queued 23: Word-wrap_comments_in_Bartik-1085472-7.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: Word-wrap_comments_in_Bartik-1085472-7.patch, failed testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented

Ah! The reason why it is not working is that style.css does not exist in Bartik in D8. The CSS now has a SMACSS style architecture so the patch needs to affect the new files.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
FileSize
Screen Shot 2015-09-01 at 15.15.03.png59.96 KB

I have also found this issue within a Forum table also...

corbacho’s picture

corbacho CreditAttribution: corbacho commented

:O mistery solved!
I'll ping kpun if he wants to finish the job

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented
FileSize
Test D8 Contrib.png54.29 KB

@emma.maria @corbacho This issue looks file with the comment section but exists with the forums table. Attaching comment section screenshot.

word-wrap

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Assigned: kpun » Unassigned
Issue tags: -CSS novice +CSS, +frontend, +Novice

The issue posted in #28 is different to the original issue. The original issue is with no word spacing.

I will have a test and maybe post a seperate issue.

Also unassigning for now as @kpun has not been active on this issue for a month and we should open it up again.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
FileSize
Screen Shot 2015-09-07 at 14.17.56.png52.09 KB
Screen Shot 2015-09-07 at 14.20.09.png47.3 KB
Screen Shot 2015-09-07 at 14.20.01.png46.28 KB

Let's ignore the Forum table issue within this issue, it's not the same.

The issue here is that if you add a long string to a comment, it will continue to push the comment out of the page container because of this style. This is particularly needed for small mobile widths as the space for comments is so small...
 

 
I tried using the solution posted in the IS and the previous patch to the current codebase but it caused further regressions and did not help :-(
 

 

 
The comments code has been refactored since this bug was discovered. So we need a new CSS solution!
The current code on the comment content is...

.comment__content {
  position: relative;
  display: table-cell;
  padding: 10px 25px 10px 25px;
  vertical-align: top;
  width: 100%;
  border: 1px solid #d3d7d9;
  font-size: 0.929em;
  line-height: 1.6;
}

PS. word-wrap: break-word which is set on the body does not work on mobile devices for some reason.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
FileSize
Screen Shot 2015-09-07 at 14.26.31.png45.55 KB
Screen Shot 2015-09-07 at 14.26.37.png38.15 KB
Screen Shot 2015-09-07 at 14.33.22.png58.27 KB
IMG_4568.jpg56.46 KB

I've updated the Issue summary.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Title: Word-wrap comments in Bartik » Long strings within comments break Bartik's page layout.
rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented
FileSize
long-comments-1085472-35.patch396 bytes

As word-wrap: break-word is not working on mobile devices I have used word-break: break-all just for the .comment__wrapper. I don't know if this is ok but it works. Patch uploaded

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 35: long-comments-1085472-35.patch, failed testing.

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented

@emma.maria The below image from your upload shows 8.0.x and the version for this issue is mentioned as 8.1.x-dev.
mobile

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review

Ok thanks, I've changed the version to 8.0.x I would like this fixed ASAP.

rudraram queued 35: long-comments-1085472-35.patch for re-testing.

rudraram’s picture

rudraram CreditAttribution: rudraram at Axelerant commented

@emma.maria Thoughts on #35 ?

briandev’s picture

briandev CreditAttribution: briandev commented

Maybe we can set the "break-word" to only apply to anchor links within comments:

.comment__content a {
  word-break: break-all;
}
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2015-09-07 at 21.45.00.png66.18 KB
Screen Shot 2015-09-07 at 21.45.48.png69.44 KB
Screen Shot 2015-09-07 at 21.49.31.png65.1 KB
Screen Shot 2015-09-07 at 21.49.51.png122.93 KB

Thanks @rudraram for the patch!

I can confirm that word-wrap has no effect but word-break fixes the visual issue.

I checked word-break on caniuse.com and it has pretty good support.
Namely the word-break: break-word combination has this description...

Chrome, Safari and other WebKit/Blink browsers also support the unofficial break-word value which is treated like word-wrap: break-word.

Therefore I am happy with this solution!

Screenshots:

Chrome

Firefox

Safari

IE9

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: long-comments-1085472-35.patch, failed testing.

martins.kajins’s picture

martins.kajins CreditAttribution: martins.kajins commented
Status: Needs work » Needs review
FileSize
core-strings-break-bartiks-page-layout-1085472-8-44.patch406 bytes

emma.maria queued 35: long-comments-1085472-35.patch for re-testing.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented

@martins.kajins thanks for your patch. However your solution you provided is different from the patch that was tested, solved the issue and was set to RTBC in #35.

The patch in #35 was set to red by the testbot and still cleanly applies. It is retesting now and I will set this back to RTBC based on #35 again.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Status: Needs review » Reviewed & tested by the community
FileSize
long-comments-1085472-35.patch396 bytes

To avoid confusion with what patch to commit here is a straight re-upload of @rudraram's patch from #35.

Setting this issue back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed eef4588 on 8.0.x 
    Issue #1085472 by kpun, emma.maria, corbacho, rudraram, martins.kajins,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

paintingguy’s picture

paintingguy CreditAttribution: paintingguy commented

Sorry if I am duplicating the Q&A but I'm having this issue right now with D8 current release. Is there a fix in this post ? Thanks.

paintingguy’s picture

paintingguy CreditAttribution: paintingguy commented

Ah, I tried this and it worked:

diff --git a/core/themes/bartik/css/components/comments.css b/core/themes/bartik/css/components/comments.css
index 30e1be8..03bbfd1 100644
--- a/core/themes/bartik/css/components/comments.css
+++ b/core/themes/bartik/css/components/comments.css
@@ -61,6 +61,7 @@

   border: 1px solid #d3d7d9;
   font-size: 0.929em;
   line-height: 1.6;
+  word-break: break-all;
 }
 
 .comment__content:before {
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Related issues: +#2679126: Words break unnecessarilly at end of line in forum topic replies

I wonder if this issue has introduced a regression: #2679126: Words break unnecessarilly at end of line in forum topic replies.

Perhaps #42 above offers a direction.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Version: 8.0.x-dev » 9.x-dev

An alternative solution to this issue is now proposed in #2679126

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.