Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently the toolbar module relies heavily on javascript to display well. We should reduce this dependency so that the toolbar displays well in a non-javascript context.
The presentation of the toolbar in a non-javascript context is to be determined. I think the best answer is, as nicely as possible.
Remaining tasks
Code review
User interface changes
None
API changes
None.
Related Issues
This issue is a dependency for:
#1938044: Prefix all toolbar classes to prevent theme clashes
#1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors
Comment | File | Size | Author |
---|---|---|---|
#37 | core-toolbar-nojs-1847314-37.patch | 22.39 KB | jessebeach |
#32 | Chrome JS Narrow.png | 127.24 KB | ekl1773 |
#32 | Chrome JS standard.png | 142.38 KB | ekl1773 |
#32 | Chrome JS wide.png | 157.29 KB | ekl1773 |
#32 | Chrome no-JS narrow.png | 85.9 KB | ekl1773 |
Comments
Comment #1
stevectorHere's a related issue that slightly reduces the amount of javascript. #1847574: Toolbar sets the active-trail to the last clicked menu item, not the current menu item.
Comment #2
Shyamala CreditAttribution: Shyamala commentedediting tags
Comment #3
nod_moving to bug since with js disabled it's pretty much unusable.
Comment #4
hello@melmcdougall.com CreditAttribution: hello@melmcdougall.com commentedHere are some very small CSS changes to make a very basic version of the toolbar still work with javascript disabled. Have tested in Safari, FF and Chrome, but would appreciate some testing and feedback if i've approached this wrong.
Basically have just focused on hiding the tray elements entirely, and making sure the key menu items are clickable and in their correct position. Not happy with how i've had to re-hide the 'edit' button (which is no use without javascript), so some feedback there would be appreciated too.
Comment #5
nod_Sorry haven't looked to closely at your changes. I've made the toolbar work without JS also, and try to get some more involved clean-up going. First go at the patch, haven't touched the JS yet. I guess it achieve sensibly the same thing :)
When js is not enabled, the toolbar is in the page flow so we don't have to guess the height of the toolbar to offset the body content with. It disappear on scroll, that's not a problem since toolbar without trays is pretty useless.
toolbar.base.css apears to be needing some simplification.
Comment #6
nod_not in a state to be reviewed.
Comment #7
jessebeach CreditAttribution: jessebeach commentedThe one thing I will ask you all to consider is that much of the redundant CSS is there to give sensible styling to IE8, since it doesn't understand media queries. It was written before be had modernizr in core, so perhaps we could eliminate much of it by adding a few feature tests and keying the CSS off those classes.
Maybe we could do something with CSS :target and anchor tags to do the show/hide of the trays?
Comment #8
jessebeach CreditAttribution: jessebeach commentedOh, and the "sensible styling" for IE8 has been torn to shreds by bit rot. The non-js version should probably just be the IE8 version as well for what it's worth.
Comment #9
oresh CreditAttribution: oresh commentedI've added css styles (not supported by ie8) for tray to be shown on .bar hover when the js is not loaded.
I was couldn't make it work like you hover on shortcuts and get the shortcuts dropdown - css can't handle it yet.
But still at least showing at least first tray for menu already gives more functionality for nojs users.
*patch includes #5 patch
Comment #10
oresh CreditAttribution: oresh commentedComment #11
oresh CreditAttribution: oresh commentedOoops. I failed with class name. Plese ignore #9, use this instead.
Comment #12
leochid CreditAttribution: leochid commentedHover on Menu works perfectly as tested below.
Tested in browser: Chrome
1. Disabled javascript in the browser, the toolbar at the top becomes a list. see image below
2. Applied patch in #11, now the menu list becomes toolbar and showing the first submenu but not the submenus for shortcut and admin
Needs work:
The other menus must show their respective menu tray.
Comment #13
oresh CreditAttribution: oresh commentedOk. so I've changed the behavior:
If you hover the icon - you get the dropdown, if you hover the text (on bar) the dropdown will be hidden ( so you can click the link itself).
It's was a hard one - not sure if possible to make it better.
*sorted with csscomb. works only when 'toolbar-processed' class is not added.
Comment #14
oresh CreditAttribution: oresh commentedOk, so after a lot of tests - found a sad thing - patch didn't work very well when js is working. And the dropdown was to sharp.
Fixed both, second by adding small css transition and a small white triangle at the bottom. Should look nicer now.
Comment #15
nod_That works surprisingly well :)
Not sure about some of the CSS but I'm no CSS guy so that'd need to be reviewed by someone else.
Comment #16
LewisNymanhmm, is there a reason why we're using :not(.js)? Don't we have a .no-js class?
The rest of the CSS seems fine
Comment #17
oresh CreditAttribution: oresh commented@LewisNyman, I did not find a no-js class when i disabled js. There is no way you can see, if js is disabled or not.
As .js class is added direct to html - that's the first element which will be rendered and displayed showing that js is enabled.
So html:not(.js) will work only when js is disabled, independently from page load time. I treid width .toolbar-processed before and understood it's a bad idea, cause adding this class to toolbar can take a lot of time. adding .js to html is almost instant.
Hope this answers your question. Thanks.
Comment #18
nod_#14: core-toolbar-nojs-1847314-14.patch queued for re-testing.
Comment #19
hass CreditAttribution: hass commentedNeeds re-role post #1938044: Prefix all toolbar classes to prevent theme clashes
Comment #20
nod_leaving NR for now, we'll put it back to NW after the other one has landed, no reason to hold up work on this one.
Comment #21
hass CreditAttribution: hass commentedI do not like to re-role the other monster patch. So this is to prevent commit conflict.
Comment #22
nod_this one is pretty far from rtbc anyway so there shouldn't be a problem.
Comment #23
jessebeach CreditAttribution: jessebeach commentedSorry for the long absence here. We've finally gotten through the bulk of issues with in-place edit and this has popped to the top of my queue again.
Thank you everyone for that patches this far. Here are a few comments.
I wish we could use
:not()
, but it is not supported by IE8.http://caniuse.com/#search=%3Anot
For this issue, the primary purpose of support a no-js presentation is to at least make the experience on IE8 and its ilk acceptable. We do this so that if the JavaScript evolves past IE8's abilities, we won't have broken the experience for the unfortunates who are still stuck with Internet Exploder.
Also, we can't target the trays by their machine-generated IDs, e.g.
#toolbar-tray--2
because these values are not stable. They are used by the JavaScript and the association between tab and tray is made automatically.updated patch
I'm taking the approach that we don't need to provide the full toolbar experience of trays to a non-js UI. We're approaching D8 from the perspective of progressive enhancement, not complete feature fidelity even in User Agents that don't provide basic feature support. Without the trays, when a user clicks on the Menu tab or the Shortcuts tab, they are taken to the page for this menu item. That is perfectly acceptable. This is why those elements are anchor tags instead of buttons. Sure, it's more clicking. But more clicking is what you get when JavaScript is turned off. JavaScript enhances the experience. It makes things faster and more efficient.
So I've hidden the trays by default. They stay hidden without JavaScript. The top bar is available and its position remains static. A user can click Admin and get to the
/admin
path. All of the admin links are available from there. The same is true for the Shortcut and User tabs.I was able to remove all of the .js class scoping declarations.
Positioning of the toolbar must be static without JavaScript, because we use JS to determine how far to pad the top of the body from the top to accomodate the visual space that the toolbar consumes. A position of fixed or absolute pulls the toolbar out of the document flow, so we need JS to calculate its height and adjust the page contents placement.
I coded and tested with the following combos in mind:
no-JS/no-MediaQuery - IE8 without the ie8 module
In this case, the user simply sees the black administration bar, there are no trays. Menu items are found by following the admin menu tree.
no-JS/MediaQuery - Chrome with JS disabled
In this case, the user simply sees the black administration bar, there are no trays. Menu items are found by following the admin menu tree.
JS/no-MediaQuery - IE8 with the ie8 module
In this case, the user simply sees the black administration bar, there are no trays. Menu items are found by following the admin menu tree.
JS/MediaQuery - Chrome
Only when JS is enabled and media queries are supported does the user get access to the the full flexibility of the toolbar layout and positioning. In order to reduce the subtle complexities between JavaScript and CSS, the CSS is now driven from classes added to the page by the toolbar JavaScript. The JavaScript is driven from event handlers bound to window.matchMedia checks fed by breakpoint configurations. So now, the placement of the breakpoints can be adjusted t through configuration rather than overriding CSS.
I was able to greatly reduce the size and complexity of toolbar.module.css. Although I had to add some JavaScript, those changes will be incorporated into the patch posted by droplet in #1860434-39: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors. I will rerolling that patch after this one, based on this one.
Comment #24
jessebeach CreditAttribution: jessebeach commentedRerolled because of #2015789-71: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute.
Comment #25
georgestephanis CreditAttribution: georgestephanis commentedFirst thought is that I'm a bit concerned with the efficiency of how the CSS rules will be applied to the DOM regarding how some of the selectors have changed from
.js[dir="rtl"] .something {}
to
[dir="rtl"] .something {}
This is bumping the speed at which the selector can fail from being class-based to being universal attribute-based.
Reference: https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient...
As these are all (by my understanding) based off the
html
element, I'd recommend swapping it to:html[dir="rtl"] .something {}
so the CSS rendering engine doesn't have to scan every element in the DOM for having
dir="rtl"
(Or, if tweaking the dom is an option, adding an ID to html, to make the queries against it super-speedy)
(Yes, unrelated to the actual thrust of the ticket, but it can be a fairly sizable performance hit if you look at profiling the CSS rendering -- sorry for the digression, though)
EDIT: Ignore me, https://drupal.org/node/2015789
Comment #26
jessebeach CreditAttribution: jessebeach commented@georgestephanis, that's a legit point in #25, but it's not germane to this issue. I'd leave a comment in #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute.
Comment #27
jessebeach CreditAttribution: jessebeach commentedWhat we want to test in this issue is that the toolbar works in the following scenarios:
And in each of those 4 states, that the toolbar displays well in the following viewport size ranges:
Comment #28
ry5n CreditAttribution: ry5n commentedI’m not able to do patch testing right now, but I’ve reviewed the CSS. Although in theory it could be improved as part of #1921610: [Meta] Architect our CSS, I don’t see anything that should prevent commit now. In fact, there’s less CSS on the whole and most selectors are simpler. The only thing that stands out is this:
There has to be a better solution for styling nested menus, but – again – IMO this isn’t a blocker; it can be done as part of post-freeze cleanup.
Thumbs up from me.
Comment #29
nod_tested on mobile. chrome and ff no js. works as expected.
Comment #30
Wim LeersComment #31
jessebeach CreditAttribution: jessebeach commented@ekl1773 noted that the toolbar break in IE8- without JavaScript. Further investigation revealed that this is because the HTMLShiv isn't loaded with JS, so the
<nav>
around the toolbar is prematurely closed by IE. Not much we can do about this, since a lot of stuff in our themes break without the shiv. I don't consider this an issue. Ancient browser? No JavaScript? Ya, the internet is going to look broken. Note, this is just the experience for authenticated users with permission to use the toolbar on IE8- with JavaScript turned off. It's still usable, just not pretty.Without JS on IE8-, using a div element instead of a nav element
Without JS on IE8-, using a nav element instead of a div element
We're not going to sacrifice semantic HTML for this extreme and fading corner case.
Comment #32
ekl1773Confirmed pre-patch behavior with JS off in Chrome and IE, menu displays as raw list as above. Here are my results with the patch applied:
no-js/no-media-query - IE8, JavaScript disabled
Tested, double checked with jbeach as above in #31, IE8 behaves very badly without JS at any size.
js/no-media-query - IE8, js
narrow: Tabs pile up in bar, trays open as pages in tree.
standard: Tabs spread out in horizontal bar, trays open as pages
wide: "
no-js/media-query - Chrome, JavaScript disabled
narrow: Bar scrolls with page. No tray, new page appears instead.
standard: Bar scrolls with page
wide: Bar scrolls with page.
js/media-query - Chrome
narrow: Toolbar and tray scroll with the page. Tray is single column. Content slides under tray.
standard: Bar and tray are fixed, scrolling inside vertical tray scrolls content as well but scrolling content doesn't budge tray.
wide: Bar and tray are fixed to viewport in both cases. Tray does scroll internally in vertical.
Comment #33
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #34
jessebeach CreditAttribution: jessebeach commentedThis issue is a dependency for:
#1938044: Prefix all toolbar classes to prevent theme clashes
#1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors
Comment #34.0
jessebeach CreditAttribution: jessebeach commenteda little grammar fix
Comment #35
hass CreditAttribution: hass commentedWhat's holding the commit back?
Comment #36
Gábor HojtsyThis issue is a dependency for:
#1938044: Prefix all toolbar classes to prevent theme clashes
#1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors
Elevating so we can make progress there.
Comment #37
jessebeach CreditAttribution: jessebeach commentedRerolled on current HEAD (e0f675f3b24735df93b03a6983aa7cfc23235322)
No code was changed in this patch. There were many conflict with the change from
[dir=rtl]
to[dir="rtl"]
. This can be set back to RTBC once it comes back green. See #33.Comment #38
dsnopekRTBC'd! I'd love to see this go in. :-)
Comment #39
jessebeach CreditAttribution: jessebeach commentedThis was committed: 1cb65032844cb12907ed484e3c17ded1528ea44f
Updated the dependent patches in the chain above this one.
#1938044: Prefix all toolbar classes to prevent theme clashes
#1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors
Comment #40
hass CreditAttribution: hass commentedThis means we have status fixed here?
Comment #41
Gábor HojtsyYup, this should be fixed :) http://drupalcode.org/project/drupal.git/commit/1cb65032844cb12907ed484e...
Comment #43
tim.plunkettPlease see #2056173: Regression: Two-column layout is broken by vertical toolbar
Comment #43.0
tim.plunkettput in the template
Comment #44
Wim LeersComment #45
droplet CreditAttribution: droplet commented