Problem/Motivation

After installing Drupal Core there is a horizontal scrollbar on the Extend page at /admin/modules.

Proposed resolution

Update the css so the Extend page does not display a horizontal scrollbar.

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit

User interface changes

Update the css so the Extend page does not display a horizontal scrollbar.

Before

Seven

Screenshot of Extend page in Seven before patch, long description scrolls off page

Bartik

Screenshot of Extend page in Bartik before patch, long description scrolls off page (and is super broken in other ways)

Claro

Note that Claro does not display the bug, so no changes to Claro are required:

Screenshot of Extend page in Claro, no scrolling (before and after patch)

After

Seven

Screenshot of Extend page in Seven after patch, long description wraps properly

Bartik

Screenshot of Extend page in Bartik after patch, long description wraps properly

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#51 3033734.49_51.interdiff.txt2.31 KBdww
#51 3033734-51.patch3.06 KBdww
#49 3033734-49.umami-after.png272.14 KBdww
#49 3033734-49.umami-before.png192.63 KBdww
#49 3033734-49.seven-vertical-toolbar-open.png262.54 KBdww
#49 3033734-49.seven-vertical-toolbar-closed.png245.86 KBdww
#49 3033734-49.claro-vertical-toolbar-open.png312.05 KBdww
#49 3033734-49.claro-vertical-toolbar-closed.png279.7 KBdww
#49 3033734-49.bartik-vertical-toolbar-open.png247.3 KBdww
#49 3033734-49.bartik-vertical-toolbar-closed.png301.09 KBdww
#49 3033734.33_49.interdiff.txt459 bytesdww
#49 3033734-49.patch616 bytesdww
#44 3033734-44.bartik.after_.png33.7 KBdww
#44 3033734-44.bartik.before.png30.54 KBdww
#44 3033734-44.seven_.after_.png37.43 KBdww
#44 3033734-44.seven_.before.png32.28 KBdww
#44 3033734-44.claro_.before-and-after.png44.36 KBdww
#42 After_seven.png364.27 KBpriyanka.sahni
#42 After_claro.png221.22 KBpriyanka.sahni
#42 After_bartik.png214.63 KBpriyanka.sahni
#42 Patch#40.png198.3 KBpriyanka.sahni
#38 3033734-after-patch.png50.49 KBIndrajithKB
#38 3033734-before-patch.png58.74 KBIndrajithKB
#37 Screen Shot 2020-05-17 at 13.31.12.png64.84 KBshaktik
#34 interdiff_30-33.txt516 bytesmeena.bisht
#33 Horizontal-Scroll-bar-after-installing-admin-toolbar-modules-3033734-33.patch1.24 KBmeena.bisht
#30 Horizontal-Scroll-bar-after-installing-admin-toolbar-modules-3033734-30.patch640 byteskostyashupenko
#27 chrome-ipadpro-after.png105.19 KBVivek Panicker
#27 chrome-ipadpro-before.png97.22 KBVivek Panicker
#26 after_patch.png85.7 KBSivaji_Ganesh_Jojodae
#26 before_patch.png103.48 KBSivaji_Ganesh_Jojodae
#19 horizontal scroll issue.png183.52 KBpratik_kamble
#19 horizontal scroll after applying patch.png252.88 KBpratik_kamble
#12 Horizontal-Scroll-bar-after-installing-admin-toolbar-modules-3033734-12.patch1.09 KBRangaswini
#9 admin-scrollbar-responsive-before-3033734-9.png18.85 KBpawandubey
#9 admin-scrollbar-responsive-after-3033734-9.png16.64 KBpawandubey
#9 interdiff_2-9.txt866 bytespawandubey
#9 Horizontal-Scroll-bar-after-installing-admin-toolbar-modules-3033734-9.patch1.09 KBpawandubey
#6 drupal_8.8.x_extend.png43.51 KBpawandubey
#6 drupal_8.6.13_extend.png41.7 KBpawandubey
#2 Horizontal-Scroll-bar-after-installing-admin-toolbar-modules-3033734-2.patch808 bytesMaithri Shetty
module-scrollbar.png142.29 KBMaithri Shetty
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Maithri Shetty created an issue. See original summary.

Maithri Shetty’s picture

Fixed Horizontal Scroll bar in module listing page after installing admin toolbar modules

dww’s picture

Project: Drupal core » Admin Toolbar
Version: 8.6.x-dev » 8.x-1.x-dev
Component: system.module » Code
Status: Needs review » Needs work

I was asked to take a look at this issue. Seems like a bug in admin_toolbar, not core's toolbar. Moving to a more appropriate queue.

idebr’s picture

Title: Horizontal Scroll bar after installing admin toolbar modules » Horizontal scrollbar on /admin/modules
Project: Admin Toolbar » Drupal core
Version: 8.x-1.x-dev » 8.8.x-dev
Component: Code » ajax system
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +CSS, +frontend

This issue is not related to the Admin toolbar, since it can be reproduced with only Drupal core. I updated the issue summary accordingly.

idebr’s picture

Component: ajax system » CSS
pawandubey’s picture

@Maithri Shetty

I am not able to replicate this issue in 8.6.13 and 8.8.x-dev version as per the attached screenshot.

I have done the clean installation and also tried to replicate the same in different resolutions but didn't found this issue.

Please recheck at your end and let us know.

pawandubey’s picture

Status: Needs review » Active
idebr’s picture

Status: Active » Needs review

You need an extension with a description that exceeds a single line. You can achieve this by increasing the description of an existing module or reducing your viewport width.

pawandubey’s picture

@idebr

Thanks for your input and yes I am able to replicate this issue when module description text is longer then we are getting the horizontal scrollbar for all the resolution.

Patch#2 covered the same and fix is also verified whereas in the breakpoints < 600px the issue still remain and for that I have re-rolled the patch for the same.

Please verify it and let me know if any changes are require.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Rangaswini’s picture

Assigned: Unassigned » Rangaswini
Status: Needs review » Needs work
Rangaswini’s picture

Rangaswini’s picture

Assigned: Rangaswini » Unassigned
pratik_kamble’s picture

Issue tags: +GlobalContributionWeekend2020, +GCWIndia2020, +punedrupalgroup
pratik_kamble’s picture

Patch LGTM. +1 RTBC. The horizontal scrolling issue does not persist anymore.

pratik_kamble’s picture

Status: Needs work » Reviewed & tested by the community
pawandubey’s picture

@rangaswini Please share the interdiff of this path with the earlier one.

@pratik_kamble As you have tested this issue, please share the screenshot for the same.

C-Logemann’s picture

Issue tags: -GlobalContributionWeekend2020 +ContributionWeekend2020

Fixing issue tag to "ContributionWeekend2020“ as suggested on global event page.

pratik_kamble’s picture

pratik_kamble’s picture

@pawandubey Interdiff can't be added as your patch was not applicable to 8.9.x branch.

lauriii’s picture

Issue tags: +Usability

Status: Reviewed & tested by the community » Needs work
Rithesh BK’s picture

Assigned: Unassigned » Rithesh BK
Issue tags: +VbContribution2020

we will work on VbContribution2020

Vivek Panicker’s picture

Triggered the testbot.
Patch #12 is passing.

Vivek Panicker’s picture

Assigned: Rithesh BK » Vivek Panicker
Status: Needs work » Needs review
Sivaji_Ganesh_Jojodae’s picture

Assigned: Vivek Panicker » Unassigned
FileSize
103.48 KB
85.7 KB

The patch #12 works as advertised.

I couldn't reproduce the issue on Google Chrome. In Firefox the issue can be noted at around 1024px width.

See the before & after patch screenshots attached.

Vivek Panicker’s picture

Before applying patch:
Tested for various device configurations available by default in Google Chrome. Found issue in in IPad Pro.
After applying patch:
Issue resolved.

Sivaji_Ganesh_Jojodae’s picture

Status: Needs review » Reviewed & tested by the community

Patch #12, looks good. +1 for RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So Claro has the same CSS but doesn't suffer the same problem. The fix to Seven here results in the descriptions being cut off but in Claro they are multi-line. I think claro's fix is better.

kostyashupenko’s picture

Version: 8.9.x-dev » 9.0.x-dev
Status: Needs work » Needs review
FileSize
640 bytes
meena.bisht’s picture

Assigned: Unassigned » meena.bisht
meena.bisht’s picture

Status: Needs review » Needs work

The above patch is added in the themes folder, hence the changes are not reflecting.
All these CSS properties should also be removed from the modules folder i.e core/modules/css/system/system.admin.css.

meena.bisht’s picture

meena.bisht’s picture

xjm’s picture

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

branch). Patches can be tested against other branches as needed when they're uploaded without changing the issue's version selector.

The issue will be automatically updated to 8.9.x after the last 8.8.x bugfix release. Thanks!

xjm’s picture

Version: 8.8.x-dev » 9.1.x-dev

Come to think of it, this is probably actually minor-only as an internal CSS change that could disrupt styling that's worked around the bug already. So disregard what I said in #35; this should be a 9.1.x-only change at this point.

shaktik’s picture

Patch #33 is working fine on 9.1.x.

#33 patch working fine on D9

IndrajithKB’s picture

Hi @MEENA BISHT thanks for the #33 patch.
I have tested with 9.1.x It's working good.

Before patch:
before patch

After patch applied:
Only local images are allowed.

IndrajithKB’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Has this been tested in all user-facing core themes? (Seven, Claro, Bartik, Umami.) Thanks!

priyanka.sahni’s picture

Assigned: Unassigned » priyanka.sahni
priyanka.sahni’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
198.3 KB
214.63 KB
221.22 KB
364.27 KB

Verified by applying the patch#40 and tested for Seven , Claro and Bartik themes , it was working fine.

Steps to test-
Go to admin site.
Go to Appearance -> In the Administration field -> Select the theme "CLARO".
Click on extend.
Verify the UI.

Follow the same steps for SEVEN and BARTIK.

Patch#40

Seven

Claro

Bartik

priyanka.sahni’s picture

Assigned: priyanka.sahni » Unassigned
dww’s picture

Here are some more targeted screenshots that directly reveal the bug and the fix.

Claro does not have this bug. The before/after screenshots are the same, so I'm only uploading one. The patch doesn't touch Claro at all, which is good.

Seven has the bug, and the long description at a narrow screen keeps going and going. It's hard to get the screenshot to show the horizontal scrollbar, but it's there.

Bartik has the bug, badly. It's all kinds of completely broken in the before case. It still doesn't look great in the after case, but for the scope of this particular issue, the patch fixes it.

Updating summary to put screenshots in the UI changes section.
Removing manual testing tag.

Patch is entirely removing CSS we don't want. Love patches that make core smaller and better. ;)

+1 RTBC.

Thanks!
-Derek

dww’s picture

p.s. <aside class="teaching-moment screenshot-etiquette">

@Indrajith KB re: #38: It's extremely helpful if you screenshot the same thing before/after the patch. No one can see what the patch is doing from those screenshots. :)

@priyanka.sahni re: #42: You need to have a narrow screen to take these screenshots, or you don't see the bug. None of those after screenshots tell us much, since none of them would trigger the bug if the patch wasn't applied.

Also, a screenshot of applying the patch isn't necessary or helpful. The bot already tells us if the patch still applies or not. At most, you can mention "Patch still applies cleanly to X branch" or something in the text of your comment, but a screenshot isn't needed for that.
</aside>

I hope these points encourage you both to keep contributing more effectively...

Thanks again!
-Derek

IndrajithKB’s picture

Hi @dww ,
Thanks for correcting me, i was attached wrong screenshot now just updated (others getting confuse bcoz of my screen shots), Now hope others can understand what i meant.
Will take care of these points in future.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @dww for mentoring folks on screenshot creation.

+++ b/core/themes/stable/css/system/system.admin.css
@@ -98,9 +98,6 @@ small .admin-link:after {
 }
 .system-modules details {
-  overflow: hidden; /* truncates descriptions if too long */
-  white-space: nowrap;
-  text-overflow: ellipsis;
   color: #5c5c5b;
   line-height: 20px;
 }
@@ -121,7 +118,6 @@ small .admin-link:after {

@@ -121,7 +118,6 @@ small .admin-link:after {
   border: 0;
 }
 .system-modules td details {
-  height: 20px;
   margin: 0;

Whoops -- we can't go around removing CSS from Stable. (Sorry, if I'd actually read the patch before I would have caught this. The BC policy for Stable is very strict.) Instead, we're going to have to fix this in Seven and Bartik directly. Actually -- hmmm, given that we've scoped this to 9.1.x only, fixing it in system.module alone might be sufficient, since the core themes no longer inherit from Stable in D9. I'd suggest creating a patch that only changes the module CSS and testing that to see if it resolves the issue. (That also means re-testing Claro to make sure it isn't regressed by the changes.)

It is probably also too late to change Stable 9, which is presumably also affected since both system.module and Stable were.

Also, we need confirmation on whether Umami is affected, and testing of Umami. Thanks!

xjm’s picture

One other thing, can we manually test this when toggling the vertical Toolbar?

dww’s picture

Re: #47:

A) Re: mentoring - my pleasure.

B) Re: Stable - Whoops. Sorry I didn't notice those changes, either. I (of all people) should know better. ;)

In this case, removing the CSS seems harmless. If someone is working around these styles, they're either a) defining something more specific to undo them, or b) having their subtheme not load this particular CSS file so they can do their own thing. In both cases, removing the broken CSS from the base theme shouldn't break whatever they're doing. But maybe I lack imagination to dream up ways removing this might actually break something.

That said, it all works fine in all themes without touching stable. If we were going to touch stable, we should also fix stable9, but as you say, better not to touch either one. New patch that leaves out the change to core/themes/stable/css/system/system.admin.css.

C) Claro is fine without touching stable. See screenshots of vertical admin toolbar referenced below.

D) Umami is indeed broken without the fix, and looks okay with it. The demo_umami profile uses seven as the admin theme, not umami. The umami theme doesn't seem to have any real styles for system things and admin things. But fixing the system module CSS at least fixes this bug. I don't think the umami theme is intended to be used as an admin theme, but if folks try it, it won't be terrible (at least in this regard).

E) Re: #48: All works great with the vertical admin toolbar open and closed. Slew O' Screenshots attached. I'm not going to bother embedding them all in the summary, but interested readers/reviewers can open them and see.

Thanks,
-Derek

Status: Needs review » Needs work

The last submitted patch, 49: 3033734-49.patch, failed testing. View results

dww’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
2.31 KB

Core sure doesn't make it easy to do the right thing. 🤦‍♂️

I *think* this is what we're supposed to do when we have stable diverge from the module CSS. Or something. Please advise. At least the test now passes locally. /shrug

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

I'm the person who wrote the annoying-but-neccessary-for-9.0 StableDecoupledTest that was failing in #49 and fixed updated in #51. I can confirm the change made in #51 is the correct way to address the issue.

The CSS changes also look fine, so RTBC.

Btw, StableDecoupledTest is unnecessary from 9.1 onward so I'll file a issue to get that removed.

dww’s picture

Thanks, @bnjmnm! Much appreciated. Sorry if my late night dismay and consternation came across poorly. ;) I know that test was valuable, but it does seem to be unneeded going forward. Feel free to ping me in Slack once that follow-up is filed and I'll do what I can to help...

  • xjm committed 1dd0d78 on 9.1.x
    Issue #3033734 by dww, pawandubey, MEENA BISHT, Maithri Shetty,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Super, glad to see it working in all core themes, and also this is really exciting because this is the first time we get to see the benefits of our new theme policy for core theme bugfixes! Thanks for the manual testing.

StableDecoupledTest was extremely valuable for D9 development, but has indeed served its purpose now.

For @shaktik: Posting screenshots of your codebase or CLI does not advance the issue, since the automated testing infrastructure tells us whether the patch applies correctly. So, I've removed your credit for this issue. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. Thank you!

Committed and pushed the patch to 9.1.x after a final review. Thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture