Prefix all toolbar classes to prevent theme clashes

EDIT: migrated to issue summary

Moving to core first to get broken D8 fixed.

.root is another potential conflicting class

Patch to fix navbar .box and .root. .menu is not yet fixed.

Missed one .box. New D7 patch.

D8 patch

Fixed 80 letters line limit.

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.

The ul.menu problem is not yet fixed in this patch as I have no idea how, but this must be fixed, too. theme_menu_tree() suxxx a lot as there is no context and don"t allow to inject other classes as I know. theme_preprocess_menu_tree() is not better.

The patch above only fix the .root and .box issue yet with zero side effects. I tried adding .menu class back to the UL's in my theme, but this has shown a lot of troubles and I just removed it again. We still need to find a way to remove the ul.menu dependency.

I think I've nailed out a workaround for ul.menu in my themes, until this is fixed in the navbar/toolbar module. theme_menu_tree() don't give you context what suxxx a lot, but THEMENAME_menu_tree__MENU() seems to work. The MENU is the machine name of the menu. In below example it's the management menu that is used by navbar. Use this as a workaround only if someone needs it.

[template.php]

/**
 * Return HTML for D7 navbar module with class 'menu' or navbar fails to render.
 */
function mytheme_menu_tree__management($variables) {
 return '<ul class="menu">' . $variables['tree'] . '</ul>';
}
/**
 * Return HTML for all other menus.
 */
function mytheme_menu_tree($variables) {
 return '<ul class="foo-menu">' . $variables['tree'] . '</ul>';
}

.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.

Yes to all. :-) If we remove .menu from the ul's we also solve #1938742: Navbar should override margin in system.css (ul.menu li) . I will role a patch with all classes prefixed later and maybe with a MODULE_theme_tree__management() if this works... But yesterday this has not worked in a quick test. Could have been a caching issue too, but I'm not sure.

Changing title. I will open a follow up issue to remove .menu class dependency.

New patch attached.

a

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

Screenshot_3_20_13_3_34_PM.png

That one is closer to complete. But something is not yet ready with the horizontal/vertical classes.

Missed 3 class renames

@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.

Ok, I've nailed out the few bugs and removed all hunks. Now it's working properly. Please shoot this in asap.

The last submitted patch, toolbar-class-name-refactoring-1938044.patch, failed testing.

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

The last submitted patch, toolbar-class-name-refactoring-1938044.patch, failed testing.

Updated issue summary.

Remove outdated info

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

The last submitted patch, toolbar-class-name-refactoring-1938044.patch, failed testing.

a

a

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

The last submitted patch, toolbar-class-name-refactoring-1938044.patch, failed testing.

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

The last submitted patch, toolbar-class-name-refactoring-1938044.patch, failed testing.

Suxxx broken robot.

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

The last submitted patch, toolbar-class-name-refactoring-1938044.patch, failed testing.

Follow core changes

Missed a line break

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

Very much... We need to commit this one here first. The other is a followup and touches a lot the lines changed here, too. I have not understood how to theme the menu in a module. Therefore I'm not a good candidate for the followup without some hints.

Needs a quick re-role as #1987066: Rename files to match CSS file naming convention has been committed.

adding tags.

This was a bit more work than only paths. JSLint and some other changes have been committed.

Please review asap.

@jesse: Can we get this committed, please?

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.

The last submitted patch, toolbar-class-name-refactoring-1938044-43.patch, failed testing.

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

#2029187: [meta] Make sure CSS classes are prefixed properly

+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

The last submitted patch, prefix-all-toolbar-classes-to-prevent-theme-clashes-removed-rtl-1938044-58.patch, failed testing.

• 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.

The last submitted patch, prefix-all-toolbar-classes-rebased-on-1847314-1938044-64.patch, failed testing.

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

This case is all about prefixing css class names. We should fix this find issue in a followup.

@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. :-)

@jesse: should we move this case now to navbar module to get this one fixed, too?

@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

We should also get #1944572: Remove "ul.menu" dependency to prevent theme clashes fixed so other modules and theme functions are not cluttering the toolbar like Bartic does.

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

added a dependency