Prefix all toolbar classes to prevent theme clashes

I can confirm that after applying patch core_toolbar_Do+not+expect+ul.menu+and+.box+in+navbar.patch to a fresh installation of Drupal 8.x-dev by hass in #7, the css classes of ul.menu and box were changed with no ill changes that I can see. Tested in Chrome and Firefox on OSX with Bartik and Seven. See screenshot.

Letting Jesse have a peek at this.

.box and .root are just two examples from your particular use case. I had wrongly assumed that a theme would not use a selector like .box {} because it has no context such as .content .box {} or .page-wrapper .box {}. But I was being a little naive. Something like the toolbar needs to be sturdier against stray CSS, even if it makes the code itself more verbose.

So it seems we'll need to prepend toolbar to every class name in the toolbar structure to prevent unqualified selectors in the themes from applying to it. I had had something like this in the first iterations of the module, but I cleaned it out because it seemed verbose. It seems the verbosity also grants it a necessary uniqueness. Is this something you could add to your patch in this issue?

I would rather do that in this issue and open a new issue for the theme_menu_tree issue. Bringing this up also helped me figure out where a stray clearfix was coming from, so thank you!

/**
 * Implements theme_menu_tree().
 */
function bartik_menu_tree($variables) {
  return '<ul class="menu clearfix">' . $variables['tree'] . '</ul>';
}

Toolbar definitely needs to rely on something other than the .menu class since that seems to be too volatile. Or maybe we need to update theme_menu_tree to accept a $variables parameter and pass in attributes through it that could be augmented in a preprocess function. Hass, what do you think about that for a followup issue? That would make it easier I think to alter this code in your themes as well.

It looks like some elements weren't updated, specifically .tab and the menu items.

@hass, sorry, the core code has changed, and your patch doesn't apply.
I couldn't find issue related to that, so started my own.
If you provide this patch for latest drupal core changes, i'll help reviewing it and testing

error: patch failed: core/modules/contextual/contextual.toolbar.css:4
error: core/modules/contextual/contextual.toolbar.css: patch does not apply
error: patch failed: core/modules/toolbar/css/toolbar.base.css:82
error: core/modules/toolbar/css/toolbar.base.css: patch does not apply
...

Reroll.

I don't endorse all the changes so far, but we are making good progress, so I rerolled to get it to apply without any changes from #17.

I'm sorry, your patch breaks everything.
I did a patch here : http://drupal.org/node/1963824, probably you can use the patch from there and add other changes ( I think i pretty much covered all the CSS in the toolbar module. )

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Working on issue summary update at Portland sprint…

a

Updated issue summary.

How much does this issue overlap with #1944572: Remove "ul.menu" dependency to prevent theme clashes ?

adding tags.

From my point of view, this is RTBC! Any other who can review and make it RTBC?

@hass: can you reroll? It doesn't apply anymore.

#43: toolbar-class-name-refactoring-1938044-43.patch queued for re-testing.

+1 to namespacing classes. This really should follow the CSS coding standards though. For example, I would refactor this:

+++ b/core/modules/contextual/css/contextual.toolbar-rtl.cssundefined
@@ -3,9 +3,9 @@
+.js .toolbar .toolbar-bar .contextual-toolbar-tab.toolbar-tab {
   float: left;
 }

to something (tentatively) like this:

.js .toolbar__tab--contextual {
  float: left;
}

Let me say clearly that my intention is not to derail this; applying the CSS standards could be a follow-up, preferably as part of #1921610: [Meta] Architect our CSS, work on which is being done in the Mobile Initiative sandbox: https://drupal.org/project/1488942/.

Can we get this committed now, please? I do not like to reroll again.

@hass, thank you for the diligent rerolls! I just went through #1847314: Reduce the dependency on JavaScript for the toolbar to display properly today. I'm going to be looking at this issue tomorrow. I want to get this committed before July 1st!

.js .toolbar__tab--contextual

@ry5n, I'm not sure how I would realistically construct this class. Here's my dilemma:

.toolbar-bar -> added in PHP in toolbar_element_info() for the toolbar element in the Toolbar module.
.contextual-toolbar-tab -> added in PHP in hook_toolbar() in the Contextual module.
.toolbar-tab -> added in PHP in toolbar_pre_render_item(), invoked by the Contextual module's specific call to hook_toolbar().

SMACSS is meant for building neat, isolated components. What we're building here are layers of semantic input that all get combined and output on HTML elements. I haven't yet wrapped my head around how we take a CSS architecture approach like SMACSS that assumes modularity at an HTML component level and square that with the reality of Drupal's hightly-alterable HTML rendering model. I'm very honestly just expressing the gap in my understanding. Is this something that the Mobile initiative folks have addressed?

I haven't yet wrapped my head around how we take a CSS architecture approach like SMACSS that assumes modularity at an HTML component level and square that with the reality of Drupal's hightly-alterable HTML rendering model

Well said; to be honest, you’re not alone :) The Mobile folks are working on applying the new approach to core in a sandbox (linked above). Ultimately practicality is what SMACSS is all about, so applying those ideas to Drupal will require some creativity and maybe some unique solutions. Again, I don’t think anything along those lines has to happen here, but I couldn’t help calling attention to the new CSS approach, since it does have a lot of overlap with this issue, and I think it’s still not received a lot of attention.

Re-rolling due to #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute has been commited.

@rteijeiro are you working on it? If so please assign it to yourself.

Applying changes related to #2015789

• Removed empty lines.
• Renamed some remained classes.
• Merged with the latest 8.x.

Some little changes:
[dir=rtl] must be [dir="rtl"] according to #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute

In file core/modules/toolbar/css/toolbar.theme.css I guess [dir=rtl] .toolbar .vertical > .lining:before has been wrongly changed to [dir=rtl] .toolbar-toolbar .toolbar-vertical > .toolbar-lining:before

[dir=rtl] .toolbar .toolbar-horizontal .toggle-orientation must be [dir=rtl] .toolbar .toolbar-horizontal .toolbar-toggle-orientation

.toolbar-icon-user.active:before must be .toolbar-icon-user.toolbar-active:before

Will not change

.toolbar-icon-user.active:before must be .toolbar-icon-user.toolbar-active:before

according #2031641: Change active class to is-active

Will not change

.toolbar-icon-user.active:before must be .toolbar-icon-user.toolbar-active:before
according #2031641: Change active class in l() function to is-active

We don't need to prefix classes like .active and .open. These classes indicate a state, not an element of the toolbar component. No library or framework should be targeting these classes without qualification, unlike .toolbar, .bar or .tray which might be targeted without qualification. By qualification, I mean without a scoping component class like .bootstrap.toolbar. Plus, as @Dragan Eror notes in #63, these state classes will probably become something like .is-active, so we want to keep state free of namespacing from any particular module.

[dir=rtl] must be [dir="rtl"] according to #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute

Let's handle this in #2030925: quote rtl attribute selector. The selectors that won't be addressed in #2030925 because they are new have been updated to include the quotations.

In file core/modules/toolbar/css/toolbar.theme.css I guess [dir=rtl] .toolbar .vertical > .lining:before has been wrongly changed to [dir=rtl] .toolbar-toolbar .toolbar-vertical > .toolbar-lining:before

[dir=rtl] .toolbar .toolbar-horizontal .toggle-orientation must be [dir=rtl] .toolbar .toolbar-horizontal .toolbar-toggle-orientation

Fixed.

Other than not prefixing the state classes (.active and .open), I've taken #60 and rebased it on #1847314-24: Reduce the dependency on JavaScript for the toolbar to display properly. We need to get that CSS/JS issue reviewed and committed first, then we can commit this prefixing issue. Then we can commit #1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors, which is next on my list to redo today; I will be basing #1860434 on this issue. It's a chain of dependencies, but it will finally clean up all the toolbar issues that have been lingering.

I included an interdiff, but it's a little disingenuous because it includes the changes from #1847314.

Built on:
0329b221e3c812de72c76a9a9205dc53d63ded93
#1847314-24: Reduce the dependency on JavaScript for the toolbar to display properly

go bot, go.

I'll reroll.

Oh, never mind, this is rolled on top of #1847314: Reduce the dependency on JavaScript for the toolbar to display properly, that's why it doesn't apply. So instead, a review!

Review

No noticeable differences while using, as expected.

Code nitpicks:

+++ b/core/modules/toolbar/css/toolbar.icons.cssundefined
@@ -1,15 +1,15 @@
- * @file toolbar.icons.css
+ * @file toolbar.toolbar-icons.css

This is not in sync with the actual filename.

+++ b/core/modules/toolbar/js/toolbar.menu.jsundefined
@@ -46,7 +46,7 @@ var activeItem = drupalSettings.basePath + drupalSettings.currentPath;
+      var $toggle = $item.find('> .toolbar-box > .toolbar-handle');

Here you use find('> .something'), later in this hunk y ou use children('.something'). We should standardize on one.

a

@Wim Leers: Why the postpone?

I'll address the comments in a followup, coming shortly.

@xmacinfo, because this patch is dependent on #1847314: Reduce the dependency on JavaScript for the toolbar to display properly

Rebased on #1847314-37: Reduce the dependency on JavaScript for the toolbar to display properly.

#1847314: Reduce the dependency on JavaScript for the toolbar to display properly was committed. Setting this issue to needs review.

I only found two unrelated bits in the patch, the comment filename one was already found above by Wim and was not yet fixed.

+++ b/core/modules/shortcut/css/shortcut.theme.cssundefined
@@ -6,34 +6,25 @@
-[dir="rtl"] .toolbar .horizontal .edit-shortcuts {
-  border-left: 0 none;
-  border-right: 1px solid #d9d9d9;
+[dir="rtl"] .toolbar .toolbar-tray-horizontal .edit-shortcuts {
   float: right;
-  margin-left: 0;
-  margin-right: 0.3333em;
-  padding-left: 0.3333em;
-  padding-right: 0.6667em;

This does not seem to be related to prefixing?

+++ b/core/modules/shortcut/shortcut.moduleundefined
index 3175b0b..bd6b9b7 100644
--- a/core/modules/toolbar/css/toolbar.icons.css

--- a/core/modules/toolbar/css/toolbar.icons.css
+++ b/core/modules/toolbar/css/toolbar.icons.cssundefined

+++ b/core/modules/toolbar/css/toolbar.icons.cssundefined
+++ b/core/modules/toolbar/css/toolbar.icons.cssundefined
@@ -1,15 +1,15 @@

@@ -1,15 +1,15 @@
 /**
- * @file toolbar.icons.css
+ * @file toolbar.toolbar-icons.css
  */

I'm not sure why the file name in the comment would need changing if the file name stay the same?

+++ b/core/modules/shortcut/css/shortcut.theme.cssundefined
@@ -6,34 +6,25 @@
-[dir="rtl"] .toolbar .horizontal .edit-shortcuts {
-  border-left: 0 none;
-  border-right: 1px solid #d9d9d9;
+[dir="rtl"] .toolbar .toolbar-tray-horizontal .edit-shortcuts {
   float: right;
-  margin-left: 0;
-  margin-right: 0.3333em;
-  padding-left: 0.3333em;
-  padding-right: 0.6667em;

This does not seem to be related to prefixing?

Correct, it's not. It's an RTL styling that was incorrect. The LTR version had changed, but not the RTL. I noticed this while testing the patch.

I'm not sure why the file name in the comment would need changing if the file name stay the same?

Greedy regex :) I changed the text in the comment back. Diff is below:

diff --git a/core/modules/toolbar/css/toolbar.icons.css b/core/modules/toolbar/css/toolbar.icons.css
index bd6b9b7..0aee152 100644
--- a/core/modules/toolbar/css/toolbar.icons.css
+++ b/core/modules/toolbar/css/toolbar.icons.css
@@ -1,5 +1,5 @@
 /**
- * @file toolbar.toolbar-icons.css
+ * @file toolbar.icons.css
  */
 .toolbar .toolbar-icon {
   padding-left: 2.75em; /* LTR */

Interdiff for #75.

Looks all good now to me! Its a search and replace of class names + that single outdated RTL style fixed. This also blocks #1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors, so marking as blocker.

Can't wait for this to get in. I've been converting Bootstrap to 8.x and the toolbar is affected because of .icon classes, would be nice to have these prefixed.

@catch committed this at http://drupalcode.org/project/drupal.git/commit/788cb06d60155575dda548aa... Yay!

Committed/pushed to 8.x, thanks!

Re-remove tag.

Hurrah! Thanks to hass, jessebeach, Dragan Eror, Gabor and everone else involved. :-)

@hass: I would suggest you to create a new issue.

@hass, yes! Now is the time. Took us a little while to get there, but we did :)

tag