Problem/Motivation

During the usability sessions, some participants did not notice the toolbar on mobile, as struggled to get back to their administrative content area.

When the toolbar was originally designed, mobile devices had limited screen sizes and vertical height was a premium, times have changed maaan.

Proposed resolution

Set position fixed on mobile
Consider setting apple-mobile-web-app-capable meta tag when logged in as well - https://developer.apple.com/library/safari/documentation/AppleApplicatio...

Remaining tasks

Implement the change
Test

User interface changes

Fixed toolbar on mobile which is always visible to the user.

API changes

None

Data model changes

None

CommentFileSizeAuthor
#36 toolbar-fixed-all-time.patch1.1 KBdroplet
#31 2864051-12.patch966 bytestameeshb
#25 fixed_mobile_toolbar-2516938-25.patch6.07 KBGrapefruitJuice
#25 interdiff-2516938-14-25.txt2.26 KBGrapefruitJuice
#23 Patch Applied-Toolbar.png24.47 KBmeenakshi.r
#23 Without Patch-Toolbar.png21.95 KBmeenakshi.r
#21 fixed_mobile_toolbar-2516938-10.patch1004 bytesellizard
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,916 pass(es). View
#21 admin_menu_scroll_before.png23.1 KBellizard
#21 admin_menu_scroll_after.png19.68 KBellizard
#21 admin_menu_open_before.png22.09 KBellizard
#21 admin_menu_open_after.png23.68 KBellizard
#18 fixed_mobile_toolbar-2516938-18.patch6.07 KBGrapefruitJuice
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,584 pass(es). View
#18 interdiff-2516938-14-18.txt2.26 KBGrapefruitJuice
#14 fixed_mobile_toolbar-2516938-14.patch4.52 KBGrapefruitJuice
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,590 pass(es). View
#14 interdiff-2516938-10-14.txt3.57 KBGrapefruitJuice
#10 fixed_mobile_toolbar-2516938-10.patch1.31 KBpetruk.dmitriy
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,474 pass(es). View
#9 toolbar-lost-menu.png23.6 KBMattA
#7 Screen Shot 2015-08-14 at 09.42.58.jpg101.88 KBLewisNyman
#4 Screen Shot 2015-08-13 at 14.44.52.jpg207.82 KBLewisNyman
#4 Screen Shot 2015-08-13 at 14.40.46.jpg894.97 KBLewisNyman
#2 fixed_mobile_toolbar-2516938-2.patch936 bytesmErilainen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,499 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

LewisNyman’s picture

Component: ckeditor.module » toolbar.module
mErilainen’s picture

Status: Active » Needs work
FileSize
936 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,499 pass(es). View

Here is a plain CSS fix for this. Probably the javascript should be updated not to toggle the toolbar-fixed css class, but I'm not familiar enough to change the code and make sure it works with the Barebone library.

LewisNyman’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
894.97 KB
207.82 KB

Ok thanks, this change also affects desktop as well as mobile. I'll ping tkoleary and ask if there was a design decision behind not making it fixed position.

In any case, this introduces a few visual oddities on desktop, we can choose to fix them here or we can choose to make this patch mobile only which doesn't seem to introduce the issues.

JohnAlbin’s picture

I'll ping tkoleary and ask if there was a design decision behind not making it fixed position.

It was because position fixed didn't work in iOS 7.
http://caniuse.com/#feat=css-fixed

Wim Leers’s picture

I think JohnAlbin may be right: I vaguely remember that this was done solely for browser compatibility reasons. It would be good though if Kevin could confirm this.

LewisNyman’s picture

Issue summary: View changes
FileSize
101.88 KB

Ah right, iOS7 had a show stopping bug which meant that the toolbar always stayed fixed even when you zoom in on text inputs, in many cases covering up the text inputs. This was fixed at some point because on iOS8 the behaviour is that the position: fixed is disabled while an input is in focus. It returns when the focus is moved to a non-input element. Here's a screenshot in iOS8 with the current patch.

I can't seem to recreate the original bug on my iOS7 simulator, it's possible it was fixed in a 7.x point release.

Wim Leers’s picture

I can't seem to recreate the original bug on my iOS7 simulator, it's possible it was fixed in a 7.x point release.

Applying Drupalisms to iOS :P

MattA’s picture

Here's another reason to make it fixed:

petruk.dmitriy’s picture

Status: Needs work » Needs review
Issue tags: +dcuacs2015
FileSize
1.31 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,474 pass(es). View

Hi, all!

If we will decide to keep toolbar menu fixed. I suggest this patch.
I've changed style for toolbar and removed unnecessary js code.

petruk.dmitriy’s picture

Also, about zoom bug.

https://www.drupal.org/files/issues/prevent_auto_zoom_in_mobile-2516918-...
This patch can fix many issues with zoom on mobile. But it might not be so comfortable for people with bad eyesight.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I manually tested this patch and I'm happy with the changes here. We don't have a formally list of mobile devices to test. As this bug was only ever reported for iOS, I feel safe marking this as RTBC without introducing regressions.

Wim Leers’s picture

Title: Set the toolbar to position fixed on mobile. » Set the toolbar to position fixed on mobile
Status: Reviewed & tested by the community » Needs work
Issue tags: +JavaScript
+++ b/core/modules/toolbar/js/views/BodyVisualView.js
@@ -38,8 +38,6 @@
-        .toggleClass('toolbar-fixed', (isViewportOverflowConstrained || this.model.get('isFixed')))

We should also remove the isFixed property from the model, and the toolbar-fixed class from the CSS and JS.

GrapefruitJuice’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
4.52 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,590 pass(es). View

Removed "isFixed" from model and also removed "toolbar-fixed" class from the CSS and JS.

Wim Leers’s picture

Issue tags: +Needs manual testing

Looks good, but this does mean we need another round of manual testing IMO.

nod_’s picture

Status: Needs review » Needs work

Things related to isViewportOverflowConstrained must go too, that would solve the eslint error too.

And now there is a zindex issue, some header links show up on top of the toolbar (like my account, home, log out).

GrapefruitJuice’s picture

Assigned: Unassigned » GrapefruitJuice
GrapefruitJuice’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
6.07 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,584 pass(es). View

Removed things related to isViewportOverflowConstrained, added position: static for #toolbar-administration to prevent zindex issue.

droplet’s picture

It's a bit buggy from my last test in #14 when you scroll the mobile down and scroll back to top very fast (like pull to fresh, out of screen size).

the debounce function in Drupal.drupalDisplace caused the wrong calculate. (leaves a big grey gap)
$(window).on('resize.drupalDisplace', debounce(displace, 200));

MattA’s picture

Assigned: GrapefruitJuice » Unassigned

@droplet That sounds a lot like the issue I reported in #2551777: Scrolling creates an empty space on mobile browsers a while back. It is actually a separate bug since it happens regardless of the position:fixed property.

ellizard’s picture

FileSize
23.68 KB
22.09 KB
19.68 KB
23.1 KB
1004 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,916 pass(es). View
lucastockmann’s picture

Assigned: Unassigned » lucastockmann

I'm going to review this.

meenakshi.r’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
21.95 KB
24.47 KB

Issue is fixed & tested.

Before applying patch

After applying patch

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

#21 lost all JavaScript changes. It was a bad reroll.

GrapefruitJuice’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
6.07 KB

What about patch from #18 ? Wich includes javascript changes and zindex fixes.
Just re-added.

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.

droplet’s picture

Status: Needs review » Needs work

I like some changes in this patch. But I'd say to keep it clean and let it move forward. Dead code / improves can be cleanup in other issues.

Above JS changes are uncertain (from my EXP on #2542050: Toolbar implementation creates super annoying re-rendering.. I can't tell the exact problem now but it's a bit tricky.)

.toolbar-fixed was designed to set Toolbar to be fixed. But now it is not present in mobile. Here's the problem we have to address.

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.

lauriii’s picture

tameeshb’s picture

Status: Needs work » Needs review
FileSize
966 bytes

Status: Needs review » Needs work

The last submitted patch, 31: 2864051-12.patch, failed testing.

Vj’s picture

Applied #31 patch on 8.4.x. Worked for me. Just need to clear cache after patch

AaronChristian’s picture

Patch works well for me too. The duplicates @laurii has noted were RTBC and have been closed already.

There are 4 coding standards messages but are unrelated to this patch.

Setting this to RTBC unless anyone else objects.

AaronChristian’s picture

Status: Needs work » Reviewed & tested by the community
droplet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.1 KB

It has a wired implementation in Toolbar's MQ handler

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.