Followup issue to #2546196: Toolbar's orientation-toggling arrows broken by #2173527, which fixed a vertical positioning issue, but not when the browser is zoomed out.

One small followup is that when you zoom out (using the browser's zoom function - for me, it happened when I hit 50%) the toggle isn't centered vertically anymore. Not sure what screens/users this might affect, but it can be fixed with the following css.

.toolbar .toolbar-tray-horizontal .toolbar-toggle-orientation > .toolbar-lining {
position: relative;
top: 50%;
transform: translateY(-50%);
-ms-transform: translateY(-50%);
-webkit-transform: translateY(-50%);
}
Here are screenshots showing before and after:

Without Patch

Arrow Without Patch

With Patch

Arrow With Patch

CommentFileSizeAuthor
#19 firefox.gif45.17 KBemma.maria
#19 chrome.gif36.91 KBemma.maria
#18 menu-move-icon.mov4.95 MBManjit.Singh
#14 toolbar-toggle-2572833-14.patch754 bytesbjlewis2
#14 Arrow2.gif87.08 KBbjlewis2
#14 Arrow.gif76.72 KBbjlewis2
#8 Patch Applied-33%.png32.82 KBmeenakshi.r
#8 Patch Applied-50%.png59.8 KBmeenakshi.r
#8 Without Patch-50%.png59.79 KBmeenakshi.r
#7 toggle.png1.13 KBshwetaneelsharma
#6 interdiff.txt504 byteslanchez
#6 toolbar_s-2572833-6.patch631 byteslanchez
#2 toolbar_s-2572833-1.patch555 bytesbjlewis2
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bjlewis2 created an issue. See original summary.

bjlewis2’s picture

bjlewis2 CreditAttribution: bjlewis2 at Modules Unraveled commented
FileSize
toolbar_s-2572833-1.patch555 bytes

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

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Active » Needs review
andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld commented
Status: Needs review » Needs work 
+++ b/core/modules/toolbar/css/toolbar.theme.css
@@ -156,6 +156,11 @@
+  transform: translateY(-50%);

Please add vendor prefixes for this property

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +CSS
lanchez’s picture

lanchez CreditAttribution: lanchez at Druid commented
Status: Needs work » Needs review
FileSize
toolbar_s-2572833-6.patch631 bytes
interdiff.txt504 bytes

Added vendor prefixes.

shwetaneelsharma’s picture

shwetaneelsharma CreditAttribution: shwetaneelsharma at Axelerant commented
FileSize
toggle.png1.13 KB

I could reproduce the original issue on Chrome when I zoomed out to 33%. Patch "toolbar_s-2572833-6.patch" resolves the central align issue.
Patch "toolbar_s-2572833-6.patch" passes testing. Attaching screenshot after applying the patch on chrome.

meenakshi.r’s picture

meenakshi.r CreditAttribution: meenakshi.r as a volunteer and at Srijan | A Material+ Company commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
Without Patch-50%.png59.79 KB
Patch Applied-50%.png59.8 KB
Patch Applied-33%.png32.82 KB

Issue is resolved when browser is zoomed in to 50 % after applying patch.

I have attached the screenshots when browser is zoomed in to 50 %

before applying patch .

after applying patch.

Also,I am not able to reproduce the issue when browser is zoomed in to 33%.

after applying patch

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: toolbar_s-2572833-6.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Testbot problems.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

Could we also get a theme or CSS maintainer to confirm the CSS for this fix? (Since the fix has a good bit of specificity.) Thanks all for the issue report, patch, and manual testing.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Closed (cannot reproduce)

Can't reproduce, must have been fixed somewhere else.

bjlewis2’s picture

bjlewis2 CreditAttribution: bjlewis2 at Modules Unraveled commented
Issue summary: View changes
Status: Closed (cannot reproduce) » Needs review
FileSize
Arrow.gif76.72 KB
Arrow2.gif87.08 KB
toolbar-toggle-2572833-14.patch754 bytes

I'm still seeing this with 8.0.4-dev.

However, the css is now being read from the "stable" theme, instead of the toolbar module. So, this is an updated patch to change the stable theme instead.

Without the patch

Arrow Without Patch

With the patch

Arrow With Patch

bjlewis2’s picture

bjlewis2 CreditAttribution: bjlewis2 at Modules Unraveled commented
Issue summary: View changes

Updated issue summary

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.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria at TES Global commented
Assigned: Unassigned » emma.maria

I am available to give a final review as a theme maintainer. Assigning to myself so I can track this issue and review ASAP.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
menu-move-icon.mov4.95 MB

Toggling orientation is working as expected. Please check screencast.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria at TES Global commented
Assigned: emma.maria » star-szr
Issue summary: View changes
Issue tags: +DrupalCampES
FileSize
chrome.gif36.91 KB
firefox.gif45.17 KB

I have a few concerns...

This fix is for the layout of the icon so it should be in the toolbar.icons.theme.css file?
The toggle layout code already sits within that file starting at line 257.

I also tried out a simple fix on the icon markup directly instead and this code seemed to fix the horizontal icon on small zoom issue...

/*.toolbar .toolbar-icon-toggle-vertical {
  position: static;
}*/

Are we allowed to add fixes to the Stable theme at this point? This one does seems to be non-disruptive.
Should this fix also go into the core module CSS as well? We have theme icon styling in the core module CSS also.

Passing this to @cottser to answer the Stable and the "which file?" questions...

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Assigned: star-szr » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs subsystem maintainer review

As a rule we don't touch Stable or Classy. This one seems justifiable but it's a bit of a slippery slope in my opinion and hard to tell what we might be breaking even if it seems safe.

What we had in mind when creating Stable was that the kind of bug fixes we would accept would be things that are obviously broken/wrong like malformed markup or something that is functionally broken. Below are some example issues that IMO made acceptable yet somewhat functional changes to Classy or Stable (funny they're mostly about progress indicators):

#2642362: Animation of throbber-active.gif image is broken
#2662778: Ajax progress is rendered via JavaScript theme function; does not include progress library
#2643636: dialog.css: ajax-progress-throbber URL is incorrect
#2638410: Views overview page doesn't filter on tags (this one would have gotten a closer look if it wasn't admin UI and particularly Views UI which is a beast to theme anyway)

Regardless of whether we fix it in Stable, yes we should definitely adjust the core module CSS. We could also consider fixing it in Seven and Bartik if we think it's worth it. Thanks!

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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.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.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.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.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Smile commented
Version: 8.8.x-dev » 9.1.x-dev
Status: Needs work » Closed (cannot reproduce)

Don't know when or how but it's fixed in 9.1.x