lauriii → credited claireristow → .
Ready for review!
Working on this now!
Ready for review!
To test, visit a create node page (eg. https://mr121-92dvfb1kuzo3pucqszjvehwaiwtuwlfm.tugboatqa.com/en/node/add...). Ensure the breadcrumb's "back to site" arrow and pipe are showing.
@ckrina, if you remember what proposed resolution #2 was about, let me know and I can push a fix for that as well.
Working on this now!
Confirmed in slack, the breadcrumb's "back to site" link is missing the back arrow on the left and the pipe on the right (see screenshot). Currently unsure what "Fix issues with current typing" was about so I'll leave that piece of feedback for now.
Merged!
This is ready for review!
Noting that I didn't follow the proposed resolution in the issue description exactly because I was seeing the accessibility error on all screen sizes. I believe this css solution is sufficient but please let me know if that's not the case.
Working on this now
Ready for review!
I tested by switching to Urdu in the Configuration > Region and language > Languages settings.
Working on this now
I also love this, thanks @saschaeggi! The CSS solution is cleaner and much less buggy.
The shadow itself looks a bit different but that can be adjusted in a follow-up issue if needed. Looks good to merge on my end!
This is looking so great @finnky!! Just noticed these two small things which can be seen in my screenshot → as well:
- The second level dropdown arrows have lost their right padding so they're showing tight to the background color.
- In the designs, it looks like we're hiding the "collapse/expand sidebar" item in the sticky menu in favour of the close button in the top right corner of the sidebar. Is that still accurate or did we decide to keep it?
claireristow → made their first commit to this issue’s fork.
Just created a new issue for that menu ordering bug here: https://www.drupal.org/project/navigation/issues/3390478 🐛 Menu rendering order is inconsistent Active
Merging these changes!
claireristow → created an issue.
I have a working example up at the tugboat link with two bugs that I need to spend more time on. Just noting them here for myself or in case others are reviewing and see these:
- When the sidebar is collapsed, if you slowly mouseover an icon from the right, it will flicker open and then closed.
- The "autoExpandToActiveMenuItem" function is being called before a "closeAllSubmenus" call which closes the active trail in the flyout. I think closeAllSubmenus is being used in too many places so some re-factoring may be needed there.
I haven't yet started on the keyboard nav and focus handling now that we've messed with the DOM structure. I would like some guidance an what is expected for this. Here are my questions:
- What is expected when you tab to a sidebar icon (in the collapsed state). Should the flyouts/tooltips show on focus, on "enter", or other. Maybe different experiences between flyouts and tooltips since one is a button and one is a link?
- Should flyouts have a focus trap?
Oops, false alarm. Sadly the "fixed" method doesn't work in all browsers. Darn Safari! I'm going to keep working on my refactor flyouts branch.
Hey @finnsky, thank you so much for the suggestion!
I just did a quick test on my local and it worked! There's even an explanation about our exact problem in the floatingUI docs with suggestions to used fixed positioning instead. This method will likely save a lot of js work and future headaches! I didn't see your comment until after I pushed up some WIP stuff on my new `3380251-scroll-refactor-flyouts` branch but I'm going to start a third branch with the "fixed" method.
I'm going to start working on this again, starting with a new branch and implementing @mherchel's suggestion:
we'll need to dynamically append the flyout submenus to the end of the DOM (before ), and absolutely position them via FloatingUI
mherchel → credited claireristow → .
Code-wise, I don't have enough module experience to approve but functionality-wise, this is looking great!
claireristow → made their first commit to this issue’s fork.
mherchel → credited claireristow → .
claireristow → created an issue.
claireristow → created an issue.
Working on this now
claireristow → created an issue.
claireristow → made their first commit to this issue’s fork.
I really like this solution Cristina! Working on this now.
Working on this now
Working on this now
Hey @ckrina, I added a delay on the flyout close. Do you mind testing it again and letting me know what you think?
From conversation with @ckrina:
Add a delay on the close of the flyout so that if there's a quick out/in on the hover of the flyout, it doesn't close. This will be especially useful in the cases where the flyout is quite tall and the user is more likely to navigate out of the parent icon and into the flyout on a more indirect/diagonal path. It is currently still closing accidentally.
claireristow → created an issue. See original summary → .
claireristow → created an issue.
Merged!
Working on this now
Hey @ckrina, do you mind doing a quick review to confirm that my fix is sufficient?
Working on this now
Merged!
Working on this now
Merged updates to BEM-style classes and setup CSS variables for icons!
Thanks for the feedback @ckrina and @mherchel, I'll work on those changes!
This solution is not currently working on Safari because Safari doesn't focus on button elements when clicked. Therefore the issue in the code is with my use of document.activeElement which returns the expected element in all browsers except for safari.
I can work on an alternative method next week when I'm back from PTO but if someone else wants to work on this in the meantime, feel free!
Thanks for catching that safari issue @hooroomoo!
I will point out that the scrollable elements don't include the Drupal icon and Bookmarks after the Safari fix since the fix was to add overflow: y to the element which excludes the Drupal icon and Bookmarks.
I'm not sure if this is the right solution, as it's not visually apparent what area can be scrolled. Shadows would help but we might want some input for @ckrina if we go this route.
The overarching issue here is that "setting one axis to visible (the default) while setting the other to a different value results in visible behaving as auto" (source: MDN). So some browsers are allowing the overflow: visible for the flyouts and tooltips but others (eg. safari) are not.
I wonder if we need to look into taking the markup for the flyouts out of the sidebar altogether but that could introduce a whole other set of issues, likely including accessibility issues since the DOM order wouldn't be logical.
@ckrina or @mherchel, I'd love to get your thoughts on this.
For some reason tugboat isn't working on this issue so I've uploaded a quick recording of it's current state.
Unassiging in case someone else wants to pick it up
Suggestion from @mherchel on slack:
"... a way around this would be to check for focus inside of the submenu before you close. TBH, if focus is inside of the submenu, it should remain opened because of 2.4.7 focus visible."
Taking this issue back to implement the above ^
This is a tough one, I'd love some input from others!
The flyout is closing only when you click the last list item's dropdown (shown in the video) because you are technically no longer "hovering" over the flyout. An easy way to illustrate this is to open the "search and metadata" dropdown, then open the "web services" dropdown. The issue won't happen in this case because "search and metadata" and "web services" each only have one child list item (meaning no flyout height change), whereas "development" has 4 child list items and therefore there is a drastic height change for the flyout as it's closes.
If we were to remove the functionality where opening one dropdown closes another, this issue would go away because there will be no sudden height change on the flyout. This is really the only solution I can see to this problem, but welcome other thoughts.
Working on this now
I believe the Firefox issue is resolved now. Ready for another review!
I'll take another crack at this!
Merged!
Working on this now
Hey @ckrina, do you mind doing a quick review of my work? I'm wondering if you're ok with the mask-image approach I took for hover color changes or if you'd prefer to have multiple color versions of the icons, or if there's another option entirely!
Thanks @hooroomoo, I'd say that's good to merge!
Working on this now
claireristow → created an issue. See original summary → .
That's looking great @hooroomoo! I'm noticing the sticky menu's shadow is not working anymore - that's added by the checkOverflow() function. After a quick code review I can't immediately see why it's not running... maybe we just need to add that function call to your new expandActiveMenu() function?
I am going to work on that remaining bug that @mherchel flagged in comment #6
Here is a summary of our Zoom discussion about using the drupal-off-canvas-main-canvas class for the shifting of main page content.
We have decided to move forward with this method. We discussed adding padding to the body element as an alternative but agreed that this would likely cause more headaches than the off-canvas approach. The worst case scenario if a developer removes the off-canvas class would be that the sidebar overlaps with some content when logged in.
We agreed the following should be done to help communicate the importance of keeping the off-canvas class when customizing themes:
- We will add a comment in HTML explaining the use of the off-canvas class and what will happen if it is removed (sidebar overlap with main page content).
- We will add this as a note in the changelog.
- If/when an article is written about changes to the Drupal sidebar, this should be mentioned as something devs should look out for when customizing themes.
Haha @ckrina looks like we commented at the same time!
Hey @Moni_10 I could replicate the issue when the thead became fixed to the top of the screen on scroll. In your screen recording, you don't have many items in that table so the table header may never become fixed but when it does, that z-index issue pops up.
I am going to merge now, since @ckrina has confirmed the fix :)
Merged!
Working on this
Thanks @ckrina! Do you mind testing the tugboat to confirm the issue has been fixed?
Working on this now!
Those changes look great @hooroomoo, thanks so much for taking this! Just approved the MR, I think it's safe to merge :)
Hey @mherchel, I'm struggling to find a way to make Drupal.displace work with CSS transitions. Since the sidebar width transitions when collapsed/expanded, the main page content position can't be calculated until the transition is complete, causing a delay/glitch. Check out the tugboat for the visual. I'm wondering if you have any insight on this one?
Is it safe to assume the `dialog-off-canvas-main-canvas` will always exist?
Working on this now!
I'm struggling to find the solution for this one. Adding overflow-y: scroll; to the sidebar when it's collapsed means the tooltips and flyouts are hidden. I am going to unassign myself for now in case someone else can pick this up.
Working on this now!
Merged!
Thanks for your work on this @Utkarsh_33 and @hooroomoo, it's looking great!
I noticed the following two items that still need fixing:
- This item from the issue details still hasn't been addressed: "When changing from wide to narrow: The icons get horizontally centered (bad)"
- A new bug has been introduced. The dropdown arrows in the expanded state are right next to the list item (see screenshot). Instead, they should be right aligned to the edge of the sidebar container.
Working on this one now!
Merged!
We'll still want the follow up tickets that Mike mentioned here: https://www.drupal.org/project/navigation/issues/3376222#comment-15180353 📌 Collapsed toolbar flyout positioning Fixed
However, the console error and general Floating UI functionality should be all fixed!
Working on this now!