- Issue created by @utkarsh_33
- Status changed to Needs review
about 1 year ago 9:24am 8 November 2023 Hi @Utkarsh_33,
I tested your #3 MR and it is working fine. The dialog title overflow issue is fixed now. But I can see this issue in other elements too, so according to me it should fixed globally like:body { word-break }
- Status changed to RTBC
about 1 year ago 1:59pm 8 November 2023 - ๐บ๐ธUnited States smustgrave
Cleaned up the issue summary some and attached an after screenshot of the issue being fixed.
- Status changed to Needs work
12 months ago 10:47pm 15 November 2023 - ๐บ๐ธUnited States xjm
Interesting.
- My first thought upon reading the issue title was "Wouldn't this apply to all modals?" Fortunately, the changes are being added to
core/themes/claro/css/components/dialog.pcss.css
, so the fix also applies to all modals. - My second thought was "Wouldn't this apply to all content, not just labels? The classes being updated are
.ui-dialog .ui-dialog-titlebar
, so the change here is only affecting the title, not the modal content. - My third thought was "Is this limited only to Claro?" In general, if one theme has a bug, we should check the other themes for the same bug.
- Finally, I'd forgotten the exact details of how the word break property behaves. I was unsure if this would break only long words, or all words for wrapping. Fortunately https://developer.mozilla.org/en-US/docs/Web/CSS/word-break has a handy interactive tool. The value of this property I was worried about is
word-break: break-all;
.break-word
is correct and so that's fine.
Points 1 and 4 above are fine. However, I think we need additional testing for points 2 and 3. Try putting a long word in the dialog content area and see what happens. And see if other core themes have the same issue.
Thanks!
- My first thought upon reading the issue title was "Wouldn't this apply to all modals?" Fortunately, the changes are being added to
I tested the above 2 mentioned points in #6 ๐ Long words overflow out of modals Needs work manually and everything is working as expected for all the 3 themes that i tested. Attaching the screen recording and screen shots for the same.
- Status changed to Needs review
12 months ago 4:33am 16 November 2023 - Status changed to RTBC
12 months ago 11:04am 16 November 2023 - ๐ฎ๐ณIndia shyam_bhatt Gujarat
@Utkarsh_33 @xjm I have checked the "MR !5290" and patch #6 ๐ Long words overflow out of modals Needs work both are working fine.
Please check the below images. The above change is working fine in every device layout.
Before:
After:
+ Moving to RTBC
- Status changed to Needs work
12 months ago 2:19pm 16 November 2023 - ๐บ๐ธUnited States xjm
Thanks @Utkarsh_33 for testing those scenarios. Adding credit.
@Shyam_Bhatt The issue was already manually tested, so we do not need additional screenshots after that. (So, I have not granted credit for it.)
Based on @Utkarsh_33's testing, it looks like Olivero and Stark instead truncate the word with an ellipsis, rather than wrapping it (probably
text-overflow: ellipsis;
?). We should do the same here.Thanks everyone!
- Status changed to Needs review
12 months ago 4:26am 17 November 2023 - Status changed to RTBC
12 months ago 3:14pm 21 November 2023 - ๐บ๐ธUnited States smustgrave
Confirmed the latest changes now use ellipsis vs wordbreak.
I've updated the issue summary with the new proposed solution and a new after screenshot since the old one was the word-break
- Status changed to Needs review
11 months ago 4:33pm 9 December 2023 - ๐บ๐ธUnited States xjm
I tried to figure out where the CSS for the ellpsis in Stark was coming from in order to compare the other style changes, and now I'm confused:
[ayrton:drupal | Sat 10:20:38] $ grep -r "ellipsis" * | grep "css" | grep -v "vendor" | grep -v "node_modules" core/modules/locale/css/locale.admin.css: text-overflow: ellipsis; core/modules/workspaces/css/workspaces.toolbar.pcss.css: text-overflow: ellipsis; core/modules/workspaces/css/workspaces.toolbar.css: text-overflow: ellipsis; core/modules/system/tests/src/Functional/Pager/PagerTest.php: $elements = $this->cssSelect(".pager__item--ellipsis:contains('โฆ')"); core/modules/system/tests/src/Functional/Pager/PagerTest.php: $elements = $this->cssSelect(".pager__item--ellipsis:contains('โฆ')"); core/themes/stable9/css/locale/locale.admin.css: text-overflow: ellipsis; core/themes/stable9/css/system/system.admin.css: text-overflow: ellipsis; core/themes/claro/css/theme/media-library.pcss.css: text-overflow: ellipsis; core/themes/claro/css/theme/media-library.css: text-overflow: ellipsis;
Olivero does not extend Stable 9, so where is the ellipsis there coming from? Additionally, the CSS in
system.admin.css
is not targeted to dialogs:.system-modules details { overflow: hidden; /* truncates descriptions if too long */ white-space: nowrap; text-overflow: ellipsis; color: #5c5c5b; line-height: 20px;
๐ค
Maybe someone could drill down inspecting the Stark and Olivero CSS in browser developer tools to document the comparable CSS there? Failing that, maybe a frontend framework manager can give us some direction.
- ๐ฎ๐ณIndia gauravvvv Delhi, India
I have checked in Claro theme and we are using
word-wrap: break-word; hyphens: auto;
at most of places.1. Media library: https://git.drupalcode.org/issue/drupal-3400027/-/blob/3400027-the-label-overflows/core/themes/claro/css/theme/media-library.pcss.css
2. Vertical Tabs: https://git.drupalcode.org/issue/drupal-3400027/-/blob/3400027-the-label-overflows/core/themes/claro/css/components/vertical-tabs.pcss.css
3. Form--managed-file.css
4. details.cssand in details.css file of Olivero theme as well.
We're not using ellipsis inside Olivero theme. We should use
word-wrap: break-word; hyphens: auto;
here as well. - Status changed to Needs work
10 months ago 5:55pm 9 January 2024 - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
I tried to figure out where the CSS for the ellpsis in Stark was coming from in order to compare the style changes, and now I'm confused:
jQuery UI's CSS in the vendor dir (๐ on including your grep!)
Based on @Utkarsh_33's testing, it looks like Olivero and Stark instead truncate the word with an ellipsis, rather than wrapping it (probably text-overflow: ellipsis;?). We should do the same here.
I probably agree with this, but since Claro uses larger fonts I think it's worth checking some common modals at narrow-but-common widths. Fewer characters can fit in that titlebar with Claro, so I'd like to rule out the truncation making it unusable. For example, things like deleting content via admin/content are done via Modal and those could get long. It's also possible that some translations result in significantly more characters. This may have been the reason Claro opted to override Jquery's default dialog styling and allow multple lines.
If the conclusion is to match the other themes, then the feedback in the MR I left should be addressed.
- ๐ฎ๐ณIndia deepanshu.varshney
deepanshu.varshney โ made their first commit to this issueโs fork.
- Status changed to Needs review
10 months ago 1:11pm 11 January 2024 - ๐ฎ๐ณIndia deepanshu.varshney
I have made the changes as per the comment #17
- Status changed to RTBC
10 months ago 11:19am 15 January 2024 - ๐ฎ๐ณIndia Nitin shrivastava
@deepanshu.varshney
MR#18 is working fine, after applying the patch the overflow issue is resolved.Before :
After :
Thanks!
- Status changed to Needs work
10 months ago 5:23pm 16 January 2024 - ๐ฎ๐ณIndia Preeti.chawla
After reviewing all the comments and scenarios with the themes (Carlo, Olivero, and Stark), I found that adding an ellipsis makes the title less readable and unclear.
I think we should follow the approach which is in this MR (https://git.drupalcode.org/project/drupal/-/merge_requests/5290/diffs?co...) for better readability and understanding."