In reviewing #2173527: The Menu-Toolbar area should be refactored so that Tray items are nested in the same container as their associated parent Toolbar item. I realized that the :focus behavior isn't applied to the push-up.svg like it is to the push-left.svg.

With the horizontal bar you get both :hover & :focus applied:

.toolbar .toolbar-toggle-orientation [value="vertical"]:hover::before, .toolbar .toolbar-toggle-orientation [value="vertical"]:focus::before {
  background-image: url("https://d9tv.ply.st/core/misc/icons/787878/push-left.svg")
}

With the vertical bar you just get the :hover applies:

.toolbar .toolbar-toggle-orientation [value="horizontal"]:hover::before {
  url("https://d9tv.ply.st/core/misc/icons/787878/push-up.svg")
}

This is somewhere in the JS for the toolbar, but haven't been able to figure out where yet.

As it is right now, for a keyboard only user, there is no indication that they are on the push-up.svg button.

CommentFileSizeAuthor
#23 toolbar_s-2546196-23.patch555 bytesbjlewis2
#22 Screen Shot 2015-09-22 at 11.32.40 AM.png12.48 KBbjlewis2
#22 Screen Shot 2015-09-22 at 11.29.22 AM.png12.69 KBbjlewis2
#22 toolbar_s-2546196-22.patch494 bytesbjlewis2
#14 10329775-14-toolbar-icon-rtl.png26.73 KBandriyun
#14 10329775-14-toolbar-icon-ltr.png35.27 KBandriyun
#14 toolbar_s-2546196-14.patch2.28 KBandriyun
#12 toolbar_s-2546196-12.patch1.55 KBandriyun
#9 toolbar_s-2546196-9.patch1.17 KBneclimdul
#6 toolbar_orientation-2546196-6.patch702 byteshussainweb
#3 Screen Shot 2015-08-10 at 10.56.05.png7.4 KBWim Leers
push-up-no-focus.png204.33 KBmgifford
push-up-hover.png185.82 KBmgifford
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford created an issue. See original summary.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer and at Acquia commented
Title: push-up.svg doesn't work on focus » Toolbar's orientation-toggling arrows broken by #2173527
Issue summary: View changes
Related issues: +#2173527: The Menu-Toolbar area should be refactored so that Tray items are nested in the same container as their associated parent Toolbar item.
FileSize
Screen Shot 2015-08-10 at 10.56.05.png7.4 KB

I think this is related:

TR’s picture

TR CreditAttribution: TR commented
Related issues: +#2563807: Admin menu misaligned toggle icon

I can confirm that the icon is still in the wrong place as demonstrated in #3, and git bisect shows #2173527: The Menu-Toolbar area should be refactored so that Tray items are nested in the same container as their associated parent Toolbar item. caused the problem. It would be nice to fix this before 8.0.0

TR’s picture

TR CreditAttribution: TR commented
Related issues: +#2267037: Toolbar tab icons without title are not aligned

Note there is a patch for the icon misalignment in #2563807: Admin menu misaligned toggle icon , but that patch just reverts #2267037: Toolbar tab icons without title are not aligned. I've added both these to the related issues list.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Active » Needs review
Issue tags: +Needs issue summary update
FileSize
toolbar_orientation-2546196-6.patch702 bytes

If I understand the problem correctly, the problem is not in JS but in CSS. When cycling through elements with tab, focus was not visible on the push-up icon, but on push-left. The attached patch fixes that. I think the issue summary could use an update for clarity.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Patch doesn't affect the location problem documented in #3, just changes the color when icon is focused.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Issue tags: +CSS
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
FileSize
toolbar_s-2546196-9.patch1.17 KB

I'm not sure what the markup people want to do hear but without mucking with the markup here is a fix for the positioning as well.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Needs work

actually looking at it further, there will have to be more work on it. This fixes the background but the click area doesn't line up.

andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld commented
Related issues: +#1663198: Clean up toolbar css
andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld commented
FileSize
toolbar_s-2546196-12.patch1.55 KB

#9 patch with some adjustments and fixed clickable mouse area.

andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld commented
Status: Needs work » Needs review
andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld commented
FileSize
toolbar_s-2546196-14.patch2.28 KB
10329775-14-toolbar-icon-ltr.png35.27 KB
10329775-14-toolbar-icon-rtl.png26.73 KB

#12 upgraded patch with some adjustments and better alignment

Steps for review:

  1. open drupal with patch (local or simplytest.me) logged as admin
  2. check toolbar icon. It should be displayed like on screenshot
  3. check clickable area
  4. check toolbar icon in left-vertical mode

Check steps 2-4 for rtl mode. It could be changed on /admin/config/regional/language/edit/en (required Language module).
In rtl-mode it should be displayed like on rtl-screenshot

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Looks actually ready

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Awesome, thanks Andy. I'd been hesitating RTBCing in case this was conflicting with changes from the other issue but if someone else from that issue is good with it +1 especially since the path on that seems unclear and its NW.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer and at Acquia commented

This also makes the CSS quite a bit simpler, so yay!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

OH YES! This has been driving me absolutely nuts.

Committed and pushed to 8.0.x. Thanks!!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

looks not pushed?!

bjlewis2’s picture

bjlewis2 CreditAttribution: bjlewis2 at Modules Unraveled commented

Not seeing it either...

  • webchick committed b89752c on 8.0.x 
    Issue #2546196 by andriyun, neclimdul, hussainweb, mgifford, Wim Leers,...
bjlewis2’s picture

bjlewis2 CreditAttribution: bjlewis2 at Modules Unraveled commented
FileSize
toolbar_s-2546196-22.patch494 bytes
Screen Shot 2015-09-22 at 11.29.22 AM.png12.69 KB
Screen Shot 2015-09-22 at 11.32.40 AM.png12.48 KB

Looks good now. One small followup is that when you scale down (for me, it happened when I hit 50%) the toggle isn't centered vertically anymore. Not sure what screens/users this might affect, but I fixed it with three lines of css.

position: relative;
top: 50%;
transform: translateY(-50%);

I've attached a patch. If it's not worthy of a commit, then no worries, if so, then yay! :)

Here are screenshots showing before and after:

Before

Before

After

After

bjlewis2’s picture

bjlewis2 CreditAttribution: bjlewis2 at Modules Unraveled commented
FileSize
toolbar_s-2546196-23.patch555 bytes

Ah, that broke the vertical toolbar. Need to be more specific and only target the horizontal one. This patch should fix that.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Putting patches on a fixed issue isn't going to do much. you'll have to change the status or probably better in this case open a follow up and explain the issue.

bjlewis2’s picture

bjlewis2 CreditAttribution: bjlewis2 at Modules Unraveled commented

Good Call.

Created new issue for the followup: #2572833: Toolbar's orientation-toggling arrow not centered when zoomed out

Status: Fixed » Closed (fixed)

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