Problem/Motivation

Follow-up discovered while testing #2405059: Replace menu images with Libricons.

A menu in the sidebar configured to display expanded sub-menu items does not properly indent.

Steps to reproduce

1) Modify the Home link in the main menu to "Show as Expanded".
2) Create a Basic Page, and add it as a sub-menu link of the Home page in the main menu.
3) Move the main menu out of the Header region, into one of the Sidebar regions of Bartik theme.
4) Configure the Menu Block to display an unlimited number of levels.

EDIT Jordana:

Simpler Steps to reproduce:

  1. Install Drupal 8.4.x
  2. Add a few pages and create the menu items for them in a hierarchy:
    • Go to nod/add/page
    • On the right open Menu Settings and create a menu
    • Set the menu parent to another page (such as the page you created before
  3. Go to the Block Page (admin/structure/block) and add the Main Navigation to one of the sidebars(ie Sidebar First)
  4. See that the submenu's are not indented in the Sidebar

Proposed resolution

Add indentation for sub-menus using CSS margins.

Proposed Solution for indentation submenus - Jordana

Remaining tasks

Write a patch.
Screenshots.

User interface changes

Sub-menu items will be displayed hierarchically using indentation
(edit Jordana: by using 1em so the arrows align)

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#79 Screenshot 2017-04-28 10.54.43.png27.53 KBmikeohara
#59 2607210-59-after-patch.png8.56 KBjordana
#59 2607210-59-before-patch.png8.48 KBjordana
#59 2607210-59.patch475 bytesjordana
#52 alignment.png14.54 KBManjit.Singh
#52 submenu_1.png10.13 KBManjit.Singh
#52 submenu.png11.04 KBManjit.Singh
#50 sidebar_menus_are_missing_indentation_styles_2607210-50.patch525 bytesManinders
#50 2607210-after-margin-rtl.png3.44 KBManinders
#50 2607210-after-margin-ltr.png3.44 KBManinders
#46 2607210-after-padding.png15.63 KBManinders
#46 2607210-before-padding.png15.51 KBManinders
#46 sidebar_menus_are_missing_indentation_styles_2607210-46.patch529 bytesManinders
#44 2607210-44-after.png40.47 KBstar-szr
#44 2607210-44-at-70ecd27.png35.82 KBstar-szr
#41 interdiff-35-41.txt535 bytesjwilson3
#41 2607210-indent-bartik-expanded-submenus-sidebar-41.patch508 bytesjwilson3
#35 2607210-indent-bartik-expanded-submenus-sidebar-35.patch497 bytesjwilson3
#28 menu.png10.36 KBtimisoreana
#25 After patch.png171.45 KBThirugnanam
#24 2607210-23.patch490 bytesimalabya
#24 RTL_menu_block_after_change.png5.18 KBimalabya
#24 RTL_menu_block_before_changes.png5.24 KBimalabya
#24 LTR_menu_block_after_change.png5 KBimalabya
#24 LTR_menu_block_before_changes.png5.71 KBimalabya
#21 interdiff-2607210.txt448 bytesimalabya
#21 2607210-21.patch577 bytesimalabya
#17 interdiffcss-16-07.patch578 bytesprateekS
#17 interdiffcss-16-07.patch578 bytesprateekS
#14 2607210-rtl.png4.05 KBameymudras
#14 interdiff-2607210-7.txt355 bytesameymudras
#8 2607210-after-patch.png142.26 KBNitesh Pawar
#8 2607210-before-patch.png148.03 KBNitesh Pawar
#7 2607210-7.patch384 bytesameymudras
#6 2607210-6.patch391 bytesShabbir
display.png5.56 KBEdgarPE
structure.png6.49 KBEdgarPE
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EdgarPE created an issue. See original summary.

star-szr’s picture

Issue tags: +frontend

Can you reproduce this with Stark or Classy? If not it might be specific to Bartik.

EdgarPE’s picture

Yes, it's Bartik specific. Classy shows sub-items indented correctly.

EdgarPE’s picture

Component: theme system » Bartik theme
Shabbir’s picture

Assigned: Unassigned » Shabbir
Shabbir’s picture

Assigned: Shabbir » Unassigned
Status: Active » Needs review
FileSize
391 bytes

Providing a patch to fix the issue.

ameymudras’s picture

FileSize
384 bytes

@Shabbir, just tested the patch provided by you. The margin would also apply to the parent ul tag which is not required. Providing a specific one.

Nitesh Pawar’s picture

I applied the patch and it works fine. I have uploaded screen shot for the same.

laxman.ghavte’s picture

Status: Needs review » Reviewed & tested by the community

After patch apply it works fine

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2607210-7.patch, failed testing.

ameymudras’s picture

Status: Needs work » Needs review

Test fails were entirely unrelated

Shabbir’s picture

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

Status: Reviewed & tested by the community » Needs work

And this looks like it needs RTL support. with [dir="rtl"] RTL attribute selector.

+++ b/core/themes/bartik/css/components/menu.css
@@ -13,3 +13,7 @@ ul.menu {
+.menu-item--expanded ul.menu{
+    margin-left: 1em;
...
\ No newline at end of file

Nit picks space before the starting curly bracket, 2 space intent, and ending new line.

ameymudras’s picture

Status: Needs work » Needs review
FileSize
355 bytes
4.05 KB

Thanks joelpittet, have attached a inter-diff for the indentation. Regarding the RTL support i think it is already taken care of by bartik (please see the screenshot).

[dir="rtl"] ol ol, [dir="rtl"] ul ul {
padding: 0 1em 0.25em 0;
}

Please let me know in case i am missing something

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +CSS

@ameymudras thanks for the interdiff, that's looking better. You may have forgot the patch in #14.

Regarding RTL, if you are adding styles that are targetting left or right margin/padding you should think how that will be represented in LTR. If it's already taken care of in RTL like that then it should be fine in LTR without this patch fix.

Or in other words, it should look like the before and after shots in #8 for RTL just the same as LTR.

prateekS’s picture

Assigned: Unassigned » prateekS
prateekS’s picture

I have tested with some minor modification in menu.css and thats working fine for me and both RTL, LTR.

Attaching patch file for it. Please review it.

prateekS’s picture

prateekS’s picture

Status: Needs work » Needs review
prateekS’s picture

Assigned: prateekS » Unassigned
imalabya’s picture

+.menu-item--expanded ul.menu{
This doesn't meet our CSS coding standards: https://www.drupal.org/node/1887862

Also there was an extra new line in the end of file.

joelpittet’s picture

Status: Needs review » Needs work

The RTL needs to be addressed still, see #15.

kostyashupenko’s picture

FileSize
8.08 KB
8.13 KB

Menu looks ok on my side for rtl & ltr. What need to fix?

menu-ltr
menu-rtl

imalabya’s picture

Status: Needs work » Needs review
FileSize
5.71 KB
5 KB
5.24 KB
5.18 KB
490 bytes

Yes, the before and after for the menu should look same for both RTL as well as LTR.

After removing the changes in the patch I found that the contents in the menu.css doesn't quite effect the menu blocks.

Adding a screenshot of before and after of both RTL and LTR along with the updated patch.

NB: I have kept the bottom padding intact as it is for the theme looks.

Thirugnanam’s picture

FileSize
171.45 KB

Tested by applying the patch in simpletest.me and its working.

Saphyel’s picture

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

Tested on my local and works as #25

alexpott’s picture

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

We need to work out why #2548805: Add sensible base layout styles for lists in Bartik. added this. Has this caused visual regressions somewhere else? I suggest also reaching out to @emma.maria in case she remembers why #2548805: Add sensible base layout styles for lists in Bartik. did what it did.

timisoreana’s picture

FileSize
10.36 KB

Was tested LTR and RTL.
menu
Actual result: Identation on RTL menu is bigger than on LTR menu
Expected result: Identation on RTL and LTR menu should be the same

timisoreana’s picture

Issue tags: -Needs manual testing
emma.maria’s picture

Assigned: Unassigned » emma.maria

Core list styling was causing issues in Bartik. It was decided to add a sensible reset to help with list styling that affected so many different kinds of lists.

It was never the intention to disrupt indentation.

I will take a look at this issue myself.

emma.maria’s picture

Title: Menu display missing indentation » Visual regression: Sidebar menus are missing indentation styles.
Priority: Normal » Major

Updating issue title and priority.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

idebr’s picture

I closed #2682307: Expanded Menu Items should indent sub-tree as a duplicate of this issue.

emma.maria’s picture

I quite like the solution in #2682307: Expanded Menu Items should indent sub-tree as it doesn't take away the styles that we needed to add for Bartik for lists.
I will ask @jwilson3 to post his patch into this issue instead.

jwilson3’s picture

Patch from Issue #2682307. Kudos to @Emma.marie for finding my issue and linking the two! That almost never happens :P

This issue is specific to the Bartik theme, where there is a CSS override that sets the margins on all ul.menu to 0, so the easy fix is to override that override for the specific case of sub-menu trees.

This patch from #2682307: Expanded Menu Items should indent sub-tree does exactly that.

joelpittet’s picture

Issue tags: +Needs manual testing, +Novice

Thanks @jwilson3 that looks much more like a solution for both RTL and RTL. Just tagging for the DrupalCon Sprints. Thanks for the previous manual testing @timisoreana!

If it is the same for both RTL and RTL with manual testing I'd say this is RTBC (reviewed and tested by community)

emma.maria’s picture

Issue tags: -Novice
bronzehedwick’s picture

Status: Needs review » Reviewed & tested by the community

Tested the patch, and it applied cleanly and fixed the regression.

star-szr’s picture

Assigned: emma.maria » Unassigned
Status: Reviewed & tested by the community » Needs work

Looks good just has some standards issues.

  1. +++ b/core/themes/bartik/css/components/menu.css
    @@ -13,3 +13,12 @@ ul.menu {
    +/* Override the above to indent expanded sub-menus */
    

    IMO this comment is unnecessary, the CSS below explains this with the expanded selector and the margin. If we keep it, it should end in a period per https://www.drupal.org/node/1354#drupal.

  2. +++ b/core/themes/bartik/css/components/menu.css
    @@ -13,3 +13,12 @@ ul.menu {
    +  margin-left: 1.5em;
    

    This line is missing the LTR comment per https://www.drupal.org/node/1887862#comments - "Styling for Right-To-Left Languages".

jwilson3’s picture

Assigned: Unassigned » jwilson3
jwilson3’s picture

Status: Needs work » Needs review
FileSize
508 bytes
535 bytes

Thanks @Cottser. This patch fixes #39.1 and #39.2 I originally wanted to fix #39.1 by removing the comment as @Cottser suggested, however after reading through the docs about Blank lines (https://www.drupal.org/node/1887862#blank-lines) I understand that I would have also had to remove the blank line that separates these two new rules from the previous section which IMO seemed to not flow correctly when looking at the entire file. Therefore I fixed #39.1 by just adding the period. Hope this makes sense.

jwilson3’s picture

Assigned: jwilson3 » Unassigned
star-szr’s picture

Status: Needs review » Reviewed & tested by the community

@jwilson3 yup that makes sense, thanks!

star-szr’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots
FileSize
35.82 KB
40.47 KB

Sorry to kick this back after RTBCing it, after manual testing I think we are not quite restoring things to how they were pre-regression.

From what I can see the spacing on the left was previously 1em, not 1.5em. The screenshots I'm attaching aren't an apples-to-apples comparison but hopefully you can see the difference. The key thing to look for is the arrows should line up with the text.

The before screenshot was taken on commit 70ecd27, one commit before #2548805: Add sensible base layout styles for lists in Bartik..

70ecd27 pre-regression:

8.2.x + #41:

emma.maria’s picture

Thanks @Cottser. I'm going to take a closer look at this during the sprint at Frontend United this weekend, which you will be at too :-)

Maninders’s picture

Patch #41 is working fine, but there is 1 issue in RTL design, so i fixed it with the padding-right:0. Please test the patch, and screenshots are attached as well.

star-szr’s picture

Status: Needs review » Needs work

@Maninders thanks that seems like a good change. Putting back to needs work so #44 can be addressed.

Maninders’s picture

Maninders’s picture

Assigned: Unassigned » Maninders
Maninders’s picture

@Cottser as per your comment #44, the css code (left space reduce from 1.5em to 1em on LTR & on RTL Right space reduce from 1.5em to 1em.) is updated. Please check the screenshots and review the patch.

star-szr’s picture

@Maninders thanks! I know it's a small patch we're working on but it's good to get into the habit of doing interdiffs :) they are really helpful.

Code-wise this looks good. Needs another round of manual testing and screenshots so leaving at needs review.

Manjit.Singh’s picture

Issue summary: View changes
Issue tags: -Needs screenshots
FileSize
11.04 KB
10.13 KB
14.54 KB

#50 looks good on RTL and LTR.

upload
upload

currently, arrows are not align with the content, I don't know if it is a part of design but can we do something like this so that arrows and content should be in one line.

upload

star-szr’s picture

Assigned: Unassigned » emma.maria

@Manjit.Singh if I understand correctly that alignment you're talking about is how it was pre-regression so probably in scope here (see my comment in #44 with pre- and post-regression screenshots for a clearer example of the difference in overall alignment).

Personally I generally like to align text with other text ("hanging" bullets and other marks in the margin) instead of aligning text to bullets or arrows but we should probably go with what Bartik has established as a pattern for the purpose of resolving this. Sending over to our Bartik maintainer for her feedback on this point :)

imalabya’s picture

star-szr’s picture

Assigned: emma.maria » Unassigned

At this point we probably just want things to look like the "pre-regression" alignment as shown in #44. That's the simplest and clearest path forward I think.

paintingguy’s picture

Bartik patches (please update this in the patches for Drupal 8)

I've been having to a patch Bartik since Drupal 8 was launched, yet this patch has never been updated to the patches.
Is there any chance this could finally be put to rest? It's so annoying having to do these manually (especially when they have been acknowledged and resolved) every time a new release comes out. Thank you for your help.

This is what I add to the menu.css

diff --git a/core/themes/bartik/css/components/menu.css b/core/themes/bartik/css/components/menu.css
index efde5d2..95420de 100644
--- a/core/themes/bartik/css/components/menu.css
+++ b/core/themes/bartik/css/components/menu.css
@@ -5,7 +5,7 @@

 /* This is needed to override ul.menu styles in menu.theme.css */
 ul.menu {
-  margin: 0;
+  margin-left: 1em;
   padding: 0 0 0.25em 0;
 } 

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jordana’s picture

So I'm coming in from a duplicate issue #2794565: Multi-level menu block styling in Bartik is not great

To me it makes more sense just to add the 1em left padding to the already set "padding" style in the menu.css
Before:
core/themes/bartik/css/components/menu.css

/* This is needed to override ul.menu styles in menu.theme.css */
ul.menu {
  margin: 0;
  padding: 0 0 0.25em 0;
}

Before Patch

After:
core/themes/bartik/css/components/menu.css

/* This is needed to override ul.menu styles in menu.theme.css */
ul.menu {
  margin: 0;
  padding: 0 0 0.25em 1em;
}

After Patch

To me this seems a cleaner solution than adding a margin-left or padding-left etc etc.
This also addresses @cottser in #2607210-44: Visual regression: Sidebar menus are missing indentation styles. - the arrows line up with the text as it is 1em padding.

See my findings in this comment:
#2794565-10: Multi-level menu block styling in Bartik is not great Which basically shows that there is a padding:0 added to LTR which messes up indentation.

And this will fix the issue in both LTR and RTL, my patch and before and after pics for reference - 2607210-59.patch

EDIT: Tested Patch on 8.3.x and 8.4.x and patch fixes issue on both
RTL before after testing:
Before RTL Only local images are allowed.
After RTL Only local images are allowed.

jordana’s picture

Assigned: Unassigned » jordana
Issue tags: +FLDC17

SImpler Steps to reproduce:

  1. Install Drupal 8.4.x
  2. Add a few pages and create the menu items for them in a hierarchy:
    • Go to nod/add/page
    • On the right open Menu Settings and create a menu
    • Set the menu parent to another page (such as the page you created before
  3. Go to the Block Page (admin/structure/block) and add the Main Navigation to one of the sidebars(ie Sidebar First)
  4. See that the submenu's are not indented in the Sidebar

-- edited the Issue summary

jordana’s picture

Issue tags: +Triaged for D8 major current state
jordana’s picture

Issue summary: View changes
jordana’s picture

Issue summary: View changes
jordana’s picture

Issue summary: View changes
jordana’s picture

Issue summary: View changes
paintingguy’s picture

This is what I am using to patch this since the launch of Drupal 8.

diff --git a/core/themes/bartik/css/components/menu.css b/core/themes/bartik/css/components/menu.css
index efde5d2..95420de 100644
--- a/core/themes/bartik/css/components/menu.css
+++ b/core/themes/bartik/css/components/menu.css
@@ -5,7 +5,7 @@
 
 /* This is needed to override ul.menu styles in menu.theme.css */
 ul.menu {
-  margin: 0;
+  margin-left: 1em;
   padding: 0 0 0.25em 0;
 }
star-szr’s picture

@jordana that does indeed look much cleaner, thank you!

star-szr’s picture

Assigned: jordana » Unassigned

Unassigning to see if we can get some folks to review this.

jordana’s picture

@cottser - glad to help! Thanks for all your time :) I'm sorry to have missed you at #fldc17

@paintingguy - did you try out the patch? I understand that adding the margin-left works, but it only works on LTR and adds extra code. adding the left padding to the already available and set style in

/* This is needed to override ul.menu styles in menu.theme.css */
ul.menu {
  margin: 0;
  padding: 0 0 0.25em 0; // change to padding: 0 0 0.25em 1em; 
}

is imho cleaner BUT apart from that, it reverts it back to what it actually was set to in bartik/css/base/elements.css:

ol ol,
ul ul {
  margin: 0;
  padding: 0 0 0.25em 1em; /* LTR */
}
[dir="rtl"] ol ol,
[dir="rtl"] ul ul {
  padding: 0 1em 0.25em 0;
}

So according to what @cottser stated in one of the comments above - ideally we would like it to be reverted back to working order pre-regression and without adding extra code. Looking at Bartik's elements.css - the left padding of 1em in LTR and right 1em padding in RTL is what was intended to be used. Again please see my comment in the duplicate issue for a more in depth explanation - #2794565-10: Multi-level menu block styling in Bartik is not great

So @paintingguy - if we'd like to get this fixed asap - testing the patch and letting us know here in the comments what you results were, would greatly help in getting this commited.

Thanks!

Cottser credited jhodgdon.

star-szr’s picture

Making good on what I said in #2794565-17: Multi-level menu block styling in Bartik is not great by adding credits.

star-szr’s picture

@jordana - sorry we missed each other in Florida! Thanks for your thorough work on this issue :)

paintingguy’s picture

@jordana
sorry for the delay.I'm very happy this is finally being looked at.
I must apologize as I am a novice at any testing or understanding how you all test code.

What exactly would you like me to do?

jordana’s picture

@paintingguy
Basically we'd like you to reproduce the steps; checkout comment #60 - #2607210-60: Visual regression: Sidebar menus are missing indentation styles. or the issue summary I edited.
You would install Drupal 8.4.x, set up the multilevel menu, place it in a block and do a before and after of the patch (preferably with screenshots if possible).
Let me know if you need more info or help and I could maybe walk you through it on Slack, irc or skype.

Thanks so much for your time and help!

star-szr’s picture

Priority: Major » Normal

This was discussed with some of the other core committers and we agreed to downgrade this to a normal bug. It is a visual regression and under the normal bug criteria the following is listed:

Bugs for site visitors that do not interfere with site use, for example, visual layout issues.

Thanks to those who have triaged and worked on this issue recently and not-so-recently. Let's try to keep this moving and get it resolved, I think we are closer than ever!

mikeohara’s picture

Assigned: Unassigned » mikeohara
Issue tags: +Baltimore2017

I am picking this up as part of the Friday Sprints at DC2017

jordana’s picture

Hey Mike!
That's great! Basically all that needs to be done is that it needs to be tested so it can be marked RTBC
It would be great to finally get this done, so thanks so much!

mikeohara’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
27.53 KB

I was able to test and confirm using steps from #60 and the confirmed patch, and this is working as expected.

star-szr’s picture

Assigned: mikeohara » star-szr

Thanks! I should be able to review this one today.

mikeohara’s picture

Version: 8.3.x-dev » 8.4.x-dev

Great! Should it be bumped to 8.4.x?

  • Cottser committed 1e04b36 on 8.4.x
    Issue #2607210 by malavya, Maninders, jwilson3, prateekS, jordana,...

  • Cottser committed b4b87ff on 8.3.x
    Issue #2607210 by malavya, Maninders, jwilson3, prateekS, jordana,...
star-szr’s picture

Version: 8.4.x-dev » 8.3.x-dev
Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs manual testing

I tested this on LTR and RTL and it looks great. I reviewed the CSS again and it makes sense and is a nice minimal change :)

Thanks everyone! Setting the version to 8.3.x-dev because I backported the change there since it's a non-disruptive bug fix to Bartik, an internal theme.

Committed and pushed 1e04b3623f to 8.4.x and b4b87ff4da to 8.3.x from DrupalCon Baltimore. Thanks!

Status: Fixed » Closed (fixed)

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