Problem/Motivation

When we tried to access the navigation links through the tabs, we noticed that the focus outline does not fully cover the longer text in both the secondary and primary navigation bars. The issue needs to be fixed in both navigation bars to ensure the focus outline works properly and covers all the text.

Steps to reproduce

1. Set up the Drupal site, moved on Olivero theme Home page.
2. Edit a menu item in the Primary and secondary navigation to a long text that would take up multiple lines.
3. Access the menu link by tab, focus outline on that long menu item will be show the error.

Proposed resolution

Change the focus outline height with something that would account for longer lengths of texts which require a bigger height for focus.

Remaining tasks

The MR differs from the latest patch. The MR needs to have the correct changes as this is what is used for commit.
Title update. See #51

User interface changes

issue image

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#56 After_3238618.png81.77 KBroshanibhangale
#56 Before_3238618.png83.55 KBroshanibhangale
#40 3238618 after #36 patch .png63.82 KBHarish1688
#40 3238618 before #36 patch .png54.85 KBHarish1688
#40 interdiff_36-40.txt656 bytesHarish1688
#40 3238618-40.patch3.17 KBHarish1688
#36 interdiff-35_36.txt1.22 KBgauravvvv
#36 3238618-36.patch4.13 KBgauravvvv
#35 after-patch-3238618.png218.63 KBlokeshsahu
#35 before-patch-3238618.png175.79 KBlokeshsahu
#35 interdiff_18-35.txt1.16 KBlokeshsahu
#35 3238618-35.patch4.29 KBlokeshsahu
#24 Screenshot-after-cache.png177.76 KBvarun verma
#23 Screenshot 2023-04-28 at 4.08.03 PM.png57.36 KBgauravvvv
#21 after-patch-on-focus#18.png161.97 KBvarun verma
#21 screenshot-after-patch-on-hover#18.png140.29 KBvarun verma
#21 Before-patch-#18.png204.78 KBvarun verma
#21 Before-patch#18.png231.4 KBvarun verma
#20 afterPatch#18.png8.82 KBasha nair
#20 beforePatch#18.png7.2 KBasha nair
#18 interdiff-11_18.txt2.48 KBgauravvvv
#18 3238618-18.patch3.69 KBgauravvvv
#14 secondary_menu_2.png90.52 KBnidhi27
#12 secondary_menu_2.png90.52 KBnidhi27
#11 interdiff-6_11.txt1.22 KBgauravvvv
#11 3238618-11.patch2.46 KBgauravvvv
#10 secondary_menu.png118.68 KBnidhi27
#6 Screenshot 2023-01-09 at 1.31.12 PM.png39.34 KBgauravvvv
#6 Screenshot 2023-01-09 at 1.30.45 PM.png54.05 KBgauravvvv
#6 3238618-6.patch1.24 KBgauravvvv
focus.png28.67 KBhooroomoo

Issue fork drupal-3238618

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

hooroomoo created an issue. See original summary.

hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Issue summary: View changes

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3238618-6.patch1.24 KB
new Screenshot 2023-01-09 at 1.30.45 PM.png54.05 KB
new Screenshot 2023-01-09 at 1.31.12 PM.png39.34 KB

I have attached a patch for same, Please review.

Earlier the focus height was fixed i.e height: var(--sp3) that was causing problem for menu items which are two liner or more than two. I have add height: calc(100% - var(--sp2)), So now it will adjust the height automatically. - var(--sp2) is the padding of span. so that it doesn't cutoff the element itself.

After patch screenshots -

Desktop:

Mobile:

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
nidhi27’s picture

nidhi27
She/Her
commented
Status: Needs review » Needs work

Hi,

I think the changes should be in nav-secondary.css.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Any reason for that @nidhi27?

nidhi27’s picture

nidhi27
She/Her
commented
Status: Needs review » Needs work
StatusFileSize
new secondary_menu.png118.68 KB

Hi @Gauravv,

The changes you have done is for primary menu only. And for that focus is working fine, But original issue description is for secondary menu which contains log out, my account. And for that menu focus is not working as expected.

Check my attached screenshot.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3238618-11.patch2.46 KB
new interdiff-6_11.txt1.22 KB

This is happening in both, primary and secondary menu. I have fixed it for secondary menu also. Attached interdiff with #6.
Please review

nidhi27’s picture

nidhi27
She/Her
commented
Status: Needs review » Needs work
StatusFileSize
new secondary_menu_2.png90.52 KB

With the latest patch #11, secondary menu focus is showing as a line.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
nidhi27’s picture

nidhi27
She/Her
commented
StatusFileSize
new secondary_menu_2.png90.52 KB

With the patch #11, focus on the secondary menu is showing as a dark line.

I have attached the screen shot.

nidhi27’s picture

nidhi27
She/Her
commented

With the patch #11, focus of secondary menu is showing as a dark line.
I have attached the screenshot.

nidhi27’s picture

nidhi27
She/Her
commented
Status: Needs review » Needs work
mgifford’s picture

mgifford
he/him
Primary language English
commented
Issue tags: +wcag144

I think this is tied to WCAG SC 1.4.4.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3238618-18.patch3.69 KB
new interdiff-11_18.txt2.48 KB

Addressed #15, attached patch and interdiff. please review

varun verma’s picture

varun verma commented
Assigned: Unassigned » varun verma
asha nair’s picture

asha nair commented
StatusFileSize
new beforePatch#18.png7.2 KB
new afterPatch#18.png8.82 KB

Applied patch #18 successfully and it fixes the issue. Adding screenshots for reference.

varun verma’s picture

varun verma commented
Status: Needs review » Needs work
StatusFileSize
new Before-patch#18.png231.4 KB
new Before-patch-#18.png204.78 KB
new screenshot-after-patch-on-hover#18.png140.29 KB
new after-patch-on-focus#18.png161.97 KB

I have reviewed Patch #18 but It's not working for me on hover and on focus, Adding screenshots for reference.

varun verma’s picture

varun verma commented
Assigned: varun verma » Unassigned
gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-04-28 at 4.08.03 PM.png57.36 KB

After patch:

I have added after patch screenshot.

I have reviewed Patch #18 but It's not working for me on hover and on focus, Adding screenshots for reference.

You have to clear the cache. This might be a cache issue in your screenshot.

varun verma’s picture

varun verma commented
StatusFileSize
new Screenshot-after-cache.png177.76 KB

Yes @Gauravvvv,
Now working fine Patch #18 its was cache issue. Attached screenshot after clear cache.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Verified issue and that patch #18 addresses issue.

Not attaching screenshots as that was done in #20 and #21

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

#18 No longer applies, probably due to changes in HEAD since the last RTBC

The reroll can be self-rtbc'd without another review cycle, just spot check that the same files are changed and the file size is similar.

rpayanm made their first commit to this issue’s fork.

rpayanm opened merge request !3986

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Please review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Reroll seems fine.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

There's some changes to the primary menu in here, also. Not sure if that's intentional.

I'm going to look through this a bit more thoroughly tomorrow.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs work

This patch is looking pretty good, but has a couple issues:

  • The comment above. I'm not sure what that rule accomplishes.
  • This issue fixes the same problem on both the primary and secondary menus. The issue summary says it just fixes it on the secondary menu. We need to update it.
  • Previously the height of the focus outlines between the primary and secondary menus were equal. Now the secondary menu's focus state has a shorter height.

Thanks for all the work on this!

lokeshsahu’s picture

lokeshsahu commented
Title: Olivero: Focus outline does not accommodate long text in secondary nav bar » Olivero: Focus outline does not accommodate long text in primary & secondary nav bar

Updated the issue title as per #33 comment.

lokeshsahu’s picture

lokeshsahu commented
Status: Needs work » Needs review
StatusFileSize
new 3238618-35.patch4.29 KB
new interdiff_18-35.txt1.16 KB
new before-patch-3238618.png175.79 KB
new after-patch-3238618.png218.63 KB

As per comment #33, I have created another patch to address the above issue. I am attaching a screenshot for reference, showing the state before and after applying the patch.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3238618-36.patch4.13 KB
new interdiff-35_36.txt1.22 KB

Fixed build error, attached interdiff for same. please review

Bushra Shaikh’s picture

Bushra Shaikh commented
Assigned: Unassigned » Bushra Shaikh
smustgrave’s picture

smustgrave commented
Assigned: Bushra Shaikh » Unassigned

Please don’t assign tickets to yourself unless a maintainer. A simple comment should be good.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems not all the points from #33 were addressed or answered to not be included.

Harish1688’s picture

Harish1688 commented
Title: Olivero: Focus outline does not accommodate long text in primary & secondary nav bar » Olivero: Focus outline does not accommodate on long text in primary & secondary navigation
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 3238618-40.patch3.17 KB
new interdiff_36-40.txt656 bytes
new 3238618 before #36 patch .png54.85 KB
new 3238618 after #36 patch .png63.82 KB

Hi

Based on comment #33, I tested all three points using the patch provided in comment #36 and found the following:

1. In the Merge Request !3986 and the patch #36, the 'align-items: center' property was added to the 'region-secondary-menu.pcss.css' file. However, after applying the patch, there were no visible changes in the user interface (UI). so removing this in 3238618-40 patch.

2. The issue summary needed an update, but only the issue title was updated in comment #34, the issue summary was also updated with the comment.

3. The height of the focus outlines between the primary and secondary menus was unequal before, but the issue has been resolved by applying the patch provided in comment #36. screenshot was attached to the comment for reference.

Before:
issue image

After:
issue image

Harish1688’s picture

Harish1688 commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 40: 3238618-40.patch, failed testing. View results

Harish1688’s picture

Harish1688 commented
Status: Needs work » Needs review

patch test is passed in #40, so restore the status to Need Review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

believe points from #33 have been addressed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 3238618-40.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Unrelated failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 3238618-40.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Seems unrelated

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 3238618-40.patch, failed testing. View results

shweta__sharma’s picture

shweta__sharma commented
Status: Needs work » Reviewed & tested by the community

Restoring the status as the patch has unrelated failures.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs title update

I'm triaging RTBC issues. I read the IS, the comments and the MR.

Not all the points in #33 have been addressed. See https://git.drupalcode.org/project/drupal/-/merge_requests/3986/diffs#no.... I went back and skimmed the comments. The problem is that the MR and the latest patch are not the same. The MR needs to be updated with the latest, correct changes.

This also needs a title change that is not simply a problem statement. Something like, "Allow primary & secondary navigation focus outline to work with long text"

I hid all files except this initial image and the latest before/after screenshots.

I restored the issue summary so that a Remaining tasks section was available. Please keep all sections of the issue summary template to help reviewers and everyone keep track of the work on an issue. Thanks.

So, just a few things to tidy up here.

tauoms made their first commit to this issue’s fork.

tauoms’s picture

tauoms commented
Title: Olivero: Focus outline does not accommodate on long text in primary & secondary navigation » Olivero: Allow primary & secondary navigation focus outline to work with long text
Issue summary: View changes
tauoms’s picture

tauoms commented

Committed changes from patch 40 to the MR.
Updated issue title.

Please let me know if there are any problems with these changes.

tauoms’s picture

tauoms commented
Status: Needs work » Needs review
Issue tags: -Needs title update
roshanibhangale’s picture

roshanibhangale commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Before_3238618.png83.55 KB
new After_3238618.png81.77 KB

Hi

I have tested the issue on the Drupal 11.x

Steps:

  1. Set up the Drupal site, moved on Olivero theme Home page.
  2. Edit a menu item in the Primary and secondary navigation to a long text that would take up multiple lines.
  3. Access the menu link by tab, focus outline on that long menu item will be show the error.

After MR3986 is applied successfully, able to see that the secondary navigation for long text is looking good.

Hence this can be move to RTBC +1

Attaching screenshot for reference.

nod_ closed merge request !3986

  • nod_ committed 95d051ff on 10.5.x 
    Issue #3238618 by gauravvvv, rpayanm, Harish1688, tauoms,...

  • nod_ committed ec78aeca on 10.6.x 
    Issue #3238618 by gauravvvv, rpayanm, Harish1688, tauoms,...

  • nod_ committed 3a32d7c0 on 11.x 
    Issue #3238618 by gauravvvv, rpayanm, Harish1688, tauoms,...

  • nod_ committed bf5a77c8 on 11.2.x 
    Issue #3238618 by gauravvvv, rpayanm, Harish1688, tauoms,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed eb0e5f464c5 to 11.x and da5cd09f6f9 to 11.2.x and ec78aecacb1 to 10.6.x and 95d051ff301 to 10.5.x. Thanks!

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.5.x-dev

Status: Fixed » Closed (fixed)

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