As a followup from #3153260: Standardize Olivero's focus states across all non-form controls. Olivero's mobile menu focus outline is offset and looks weird. It should be symetrical.

CommentFileSizeAuthor
#18 Screenshot 2021-03-04 at 17.03.44.png20.71 KBgauravvvv
#17 interdiff-17-14.txt1.12 KBimalabya
#17 3200595-17.patch1.62 KBimalabya
#14 3200595-14.patch1.6 KBgauravvvv
#14 Screenshot 2021-03-03 at 21.13.10.png40.57 KBgauravvvv
#10 Screenshot 2021-03-03 at 10.11.01.png38.63 KBgauravvvv
#10 3200595-10.patch1.3 KBgauravvvv
#7 3200595-after_2.png48.38 KBabhijith s
#7 3200595-after_1.png51.18 KBabhijith s
#7 3200595-before_2.png23.73 KBabhijith s
#7 3200595-before_1.png23.98 KBabhijith s
#5 Screenshot 2021-03-02 at 10.47.46.png47.31 KBgauravvvv
#5 Screenshot 2021-03-02 at 10.49.34.png30.5 KBgauravvvv
#5 3200595-5.patch1.12 KBgauravvvv
#3 3200595-3.jpg30.62 KBkleiton_rodrigues
#2 Screenshot 2021-03-01 at 13.38.25.png12.53 KBgauravvvv
mobile-menu-focus-offset.png37.24 KBmherchel

Comments

mherchel created an issue. See original summary.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new Screenshot 2021-03-01 at 13.38.25.png12.53 KB

Hi @mherchel, The on-focus styling seems fine to me in chrome and safari browser. I have added a screenshot for reference.

kleiton_rodrigues’s picture

kleiton_rodrigues commented
Status: Active » Reviewed & tested by the community
StatusFileSize
new 3200595-3.jpg30.62 KB

Seems working finein Chrome Version 87.0.4280.88 (Official version) 64 bits. Moving to RTBC.
Added an after patch screenshot for reference.

mherchel’s picture

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

I'm still seeing this. Make sure you have the latest 9.2.x pulled

You can also view it at https://lb.cm/olivero

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3200595-5.patch1.12 KB
new Screenshot 2021-03-02 at 10.49.34.png30.5 KB
new Screenshot 2021-03-02 at 10.47.46.png47.31 KB

Hi @mherchel, This is an issue with v9.2. This is fixed in Drupal 9.1. I have added the patch for the same with the after patch screenshot.
Note: Drupal 9.2 screenshot is after the patch.
no alteration has done on 9.1 screenshot.

gauravvvv’s picture

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

abhijith s commented
StatusFileSize
new 3200595-before_1.png23.98 KB
new 3200595-before_2.png23.73 KB
new 3200595-after_1.png51.18 KB
new 3200595-after_2.png48.38 KB

Applied patch #5 and it works fine.

Before patch:

before

before2

After patch:

after

after2

RTBC +1

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

This looks better, but I'd also like to have the same horizontal spacing as we do vertical spacing.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3200595-10.patch1.3 KB
new Screenshot 2021-03-03 at 10.11.01.png38.63 KB

Hi Mherchel, I have provided a patch as you suggested in #8. Adding an after-patch screenshot for reference, please review.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
siddhant.bhosale’s picture

siddhant.bhosale commented
Assigned: Unassigned » siddhant.bhosale
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

This is looking great, but needs some minor changes.

  • The current implementation removes the horizontal padding, and then relies on the browser's default styles (which may change in the future). Let's be explicit with the horizontal padding and set it to 6px (no need to use variables here)
  • There's a negative margin applied to the button that moves it toward the right so that the icon lines up with the content. This needs to be equal to the padding value.

Thanks for the work on this!

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new Screenshot 2021-03-03 at 21.13.10.png40.57 KB
new 3200595-14.patch1.6 KB

Hi @mherchel, Please review the patch. padding updated to 6px and margin from right is 0. content and button both have same padding and 0 margin.

gauravvvv’s picture

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

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

@Gauravmahlawat Thanks for the work on this. Please change the right margin to -6px. The reason is that I want the icon to align with the content below (and that padding pushes the icon to the left).

imalabya’s picture

imalabya
Primary language English
Location Bangalore
commented
Assigned: siddhant.bhosale » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3200595-17.patch1.62 KB
new interdiff-17-14.txt1.12 KB

Sorry if I am hijacking the issue. Seemed like a minor fix so adding a patch as per #16

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new Screenshot 2021-03-04 at 17.03.44.png20.71 KB

Patch #17, works for me. Moving to RTBC.

gauravvvv’s picture

gauravvvv commented
Status: Needs review » Reviewed & tested by the community
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed 160947c and pushed to 9.2.x. Thanks!

  • lauriii committed 160947c on 9.2.x 
    Issue #3200595 by Gauravmahlawat, imalabya, mherchel: Olivero's mobile...

Status: Fixed » Closed (fixed)

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