Problem/Motivation

I'm using .box in my theme and the .box style is now applied to the boxes, what shouldn't be the case, too. Navbar need to use .navbar-box class as example. Other classes are also potentially conflicting. All classes need to be prefixed with it's module name.

If this happens the navbar looks like:

2013-03-09_115538_0.png

Proposed resolution

Resolution proposed in #12 is to prepend toolbar to each of the class names in Toolbar module.

Remaining tasks

None

This issue is a dependency for:

#1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors

CommentFileSizeAuthor
#76 interdiff_72-to-75.txt380 bytesjessebeach
#75 prefix-all-toolbar-classes-1938044-75.patch45.07 KBjessebeach
#72 prefix-all-toolbar-classes-rebased-on-1847314-1938044-72.patch45.16 KBjessebeach
#64 prefix-all-toolbar-classes-rebased-on-1847314-1938044-64.patch45.2 KBjessebeach
#64 interdiff_60-to-64.txt46.74 KBjessebeach
#60 prefix-all-toolbar-classes-to-prevent-theme-clashes-removed-rtl-1938044-60.patch46.85 KBDragan Eror
#58 prefix-all-toolbar-classes-to-prevent-theme-clashes-removed-rtl-1938044-58.patch46.62 KBDragan Eror
#49 toolbar_Prefix+all+toolbar+classes+to+prevent+theme+clashes-49.patch46.48 KBhass
#50 toolbar_Prefix+all+toolbar+classes+to+prevent+theme+clashes-49.patch46.48 KBhass
#43 toolbar-class-name-refactoring-1938044-43.patch46.76 KBhass
#38 toolbar-class-name-refactoring-1938044-37.patch46.06 KBhass
#37 toolbar-class-name-refactoring-1938044-37.patch46.05 KBhass
#24 toolbar-class-name-refactoring-1938044.patch46.53 KBhass
#20 toolbar-class-name-refactoring-1938044-20.patch47.3 KBjessebeach
#17 toolbar_Prefix+all+toolbar+classes+to+prevent+theme+clashes3.patch45.29 KBhass
#16 toolbar_Prefix+all+toolbar+classes+to+prevent+theme+clashes2.patch45.26 KBhass
#15 Screenshot_3_20_13_3_34_PM.png77.21 KBjessebeach
#1 2013-03-09_115538.png9.45 KBhass
#14 toolbar_Prefix+all+toolbar+classes+to+prevent+theme+clashes.patch37.11 KBhass
#8 core_toolbar_Do+not+expect+ul.menu+and+.box+in+navbar-after.png48.37 KBandymartha
#7 core_toolbar_Do+not+expect+ul.menu+and+.box+in+navbar.patch3.46 KBhass
#5 navbar_+Do+not+expect+ul.menu+and+.box+in+navbar-D7.patch3.28 KBhass
#6 core_toolbar_Do+not+expect+ul.menu+and+.box+in+navbar.patch3.38 KBhass
#4 navbar_+Do+not+expect+ul.menu+and+.box+in+navbar-D7.patch3.05 KBhass
2013-03-09_115538.png9.45 KBhass
2013-03-09_114904.png31.5 KBhass
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Title: Prefix all toolbar classes to prevent theme clashes » Do not expect "ul.menu" and ".box"
Project: Drupal core » Navbar
Version: 8.x-dev » 7.x-1.0-alpha4
Component: toolbar.module » Code
Assigned: jessebeach » Unassigned
Priority: Major » Critical
Status: Needs review » Active
FileSize
9.45 KB

EDIT: migrated to issue summary

hass’s picture

Title: Do not expect "ul.menu" and ".box" » Do not expect "ul.menu" and ".box" in navbar
Project: Navbar » Drupal core
Version: 7.x-1.0-alpha4 » 8.x-dev
Component: Code » toolbar.module
Priority: Critical » Major

Moving to core first to get broken D8 fixed.

hass’s picture

.root is another potential conflicting class

hass’s picture

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

hass’s picture

Status: Needs review » Active
FileSize
3.28 KB

Missed one .box. New D7 patch.

hass’s picture

Status: Active » Needs review
FileSize
3.38 KB

D8 patch

hass’s picture

Status: Active » Needs review
FileSize
3.46 KB

Fixed 80 letters line limit.

andymartha’s picture

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

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.

webchick’s picture

Assigned: Unassigned » jessebeach
Status: Reviewed & tested by the community » Needs review

Letting Jesse have a peek at this.

hass’s picture

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.

hass’s picture

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>';
}
jessebeach’s picture

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

hass’s picture

Status: Needs review » Needs work

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.

hass’s picture

Title: Do not expect "ul.menu" and ".box" in navbar » Prefix all toolbar classes to prevent theme clashes
Status: Needs work » Needs review
FileSize
37.11 KB

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

New patch attached.

hass’s picture

Issue summary: View changes

a

jessebeach’s picture

Title: Do not expect "ul.menu" and ".box" » Prefix all toolbar classes to prevent theme clashes
Project: Navbar » Drupal core
Version: 7.x-1.0-alpha4 » 8.x-dev
Component: Code » toolbar.module
Assigned: Unassigned » jessebeach
Priority: Critical » Major
Status: Active » Needs review
FileSize
77.21 KB

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

Screenshot_3_20_13_3_34_PM.png

hass’s picture

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

hass’s picture

hass’s picture

Status: Needs review » Needs work
oresh’s picture

@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
...
jessebeach’s picture

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.

oresh’s picture

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

dcam’s picture

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.

jason.bell’s picture

Working on issue summary update at Portland sprint…

jason.bell’s picture

Issue summary: View changes

a

jason.bell’s picture

Issue summary: View changes

Updated issue summary.

hass’s picture

Status: Needs work » Needs review
FileSize
46.53 KB

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

Status: Needs review » Needs work
Issue tags: -Needs issue summary update

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

hass’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

hass’s picture

Issue summary: View changes

Remove outdated info

hass’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Anonymous’s picture

Issue summary: View changes

a

hass’s picture

Issue summary: View changes

a

hass’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

hass’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

hass’s picture

Suxxx broken robot.

hass’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

hass’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
46.05 KB

Follow core changes

hass’s picture

Missed a line break

yoroy’s picture

hass’s picture

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.

hass’s picture

Status: Needs review » Needs work

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

jessebeach’s picture

Issue tags: +sprint, +Spark, +CSS novice

adding tags.

hass’s picture

Status: Needs work » Needs review
FileSize
46.76 KB

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

Please review asap.

hass’s picture

@jesse: Can we get this committed, please?

xmacinfo’s picture

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

Wim Leers’s picture

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

xmacinfo’s picture

Status: Needs review » Needs work
Issue tags: +sprint, +Spark, +CSS novice

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

hass’s picture

hass’s picture

Status: Needs work » Needs review
FileSize
46.48 KB

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

hass’s picture

ry5n’s picture

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

jessebeach’s picture

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?

ry5n’s picture

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.

rteijeiro’s picture

Dragan Eror’s picture

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

Dragan Eror’s picture

Assigned: jessebeach » Dragan Eror
Dragan Eror’s picture

Applying changes related to #2015789

Status: Needs review » Needs work
Dragan Eror’s picture

  • Removed empty lines.
  • Renamed some remained classes.
  • Merged with the latest 8.x.
Dragan Eror’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Status: Needs review » Needs work

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

Dragan Eror’s picture

Will not change

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

according #2031641: Change active class to is-active

jessebeach’s picture

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

jessebeach’s picture

Status: Needs work » Needs review
Issue tags: -sprint, -Spark

go bot, go.

Status: Needs review » Needs work

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

Wim Leers’s picture

Issue tags: +sprint, +Spark

I'll reroll.

Wim Leers’s picture

Status: Needs work » Postponed

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.

Wim Leers’s picture

Issue summary: View changes

a

hass’s picture

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

xmacinfo’s picture

@Wim Leers: Why the postpone?

jessebeach’s picture

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

jessebeach’s picture

Status: Postponed » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work

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?

jessebeach’s picture

Status: Needs work » Needs review
FileSize
45.07 KB
+++ 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 */
jessebeach’s picture

FileSize
380 bytes

Interdiff for #75.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +blocker

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.

markhalliwell’s picture

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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -sprint
catch’s picture

Issue tags: +sprint

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Re-remove tag.

dsnopek’s picture

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

hass’s picture

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

xmacinfo’s picture

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

jessebeach’s picture

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

nod_’s picture

Issue tags: +JavaScript

tag

hass’s picture

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.

Anonymous’s picture

Issue summary: View changes

added a dependency