Defining CSS reset within body.active_admin clobbers third party gem styles #1852

pietschy · 2013-01-14T23:30:12Z

From what I can see, since #1343 AA targets it's CSS resets styles to body.active_admin. This is giving them higher specificity than styles included later in the asset chain.

This is a big problem when using third party plugins such as https://github.com/bastiaanterhorst/rich. The the resets completely clobbering things and I have to do various ugly hacks to make things work.

A quick fix of moving the reset out of body.active_admin might work but I think the larger problem is that the common pattern '... is to include all css files together.' (mentioned in the pull request) isn't really going to work here.

I'm not a rails expert but I would have thought it was possible to include AA specific assets on only AA pages so the body.active_admin selector can be dropped???

The text was updated successfully, but these errors were encountered:

lmb · 2013-01-21T11:01:54Z

I've had a look at this issue and unfortunately the fix isn't entirely trivial, since active admin as well as upstream vendors will have to make changes.

Specifically, have a look at lmb/active_admin@5f4811fab9f056f4b916932c42c1b16814e1d7a6. Making AA's reset less specific fixes some of the issues:

Unfortunately, margin and padding is missing on some elements, since CKEditor does not sufficiently scope their .cke_top and .cke_toolgroup classes (at least for the new Moono skin).

I'm not opening a pull request yet, since I'd like some more input on whether this is the right way to go. Specifically, does somebody know if .active_admin is used on elements other than <body>? Also, I'm not certain if the weird lines around the top and bottom of the CKEditor icons are the same problem or if it's a different issue.

seanlinsley · 2013-01-21T14:51:55Z

Just confirming, you tried this, right?

body.active_admin {
  // Mixin from Active Admin: _buttons.css.scss
  a.cancel, a.clear { @include light-button }
}

By that I mean, are you including the CKEditor CSS inside the body.active_admin selector?

lmb · 2013-01-21T15:15:08Z

I'm not sure what you intend with this code snippet, but if I understand correctly you're referring to the fix at the bottom of this document? I went that route, which has two drawbacks for me:

I have to copy over all .css to .scss in my application folder everytime the galetahub/ckeditor gem is updated, which is a) cumbersome and b) not really the place they belong.
It breaks full-screen editing (using the maximize plugin). Which is partially the plugins fault.

The problem with this issue is, that both AA and CKE can fix this. Which makes it no one's problem.

seanlinsley · 2013-01-21T17:25:46Z

Specific to your situation, this is what I meant.

body.active_admin {
  @import "ckeditor/application";
}

Though I agree we should remove the body specification. Reason being, it resulted in this CKEditor rule:

body.active_admin body {
  padding: 0;
  margin: 0;
  font-family: Arial, Helvetica, sans-serif;
}

Which obviously wouldn't match anything.

pietschy · 2013-01-25T00:53:30Z

Hi guys,

Thanks for the input.

@daxter
Yep, I have been fixing this by putting the styles under body.active_admin but it's a pretty ugly hack and is completely non DRY. That and I have to do it for every 3rd party widget I'm using.

My main point for the issue is that I think using the body.active_admin scope is not the best way to solve the problem for which it appears to have been added (i.e. to stop AA styles from clobbering styles in non AA portions the websites). I think a better approach is to not include AA styles on non AA pages.

@imb
I just tried the nested @import "ckeditor/application" but it didn't seem to work for me. I didn't look too hard as to why though since as you mentioned it's can create non sensical style rules (and I've gone with the approach of manually re-defining the clobbered styles for now) . Also prior to SASS 3.2 that line would break if the import defined any mixins.

lmb · 2013-01-27T22:00:05Z

@daxter
I was under the impression that importing non-scss files in SASS is currently not possible? That was one solution I was thinking of. It would make a workaround for these kinds of bugs easy and very low maintenance. (Put css in vendor/ and @import them in a custom stylesheet in lib/ or sth.)

@pietschy
See my comment above. Just had a look, and this as well as this issue is what I am talking about.

In the meantime, any chance that the CSS 'fix' I referenced will go into mainline? I think it'd be a good idea regardless.

seanlinsley · 2013-01-27T22:34:40Z

@lmb, ~~CKEditor uses SASS~~ the rules are included nonetheless. If you open up the generated CSS file from your server and search for "ckeditor", you'll see what I mean.

pietschy · 2013-01-29T05:32:34Z

Sorry, I clicked close accidentally.

lmb · 2013-01-29T11:15:02Z

@daxter
Thanks for the info, I'll check this out. Is that some undocumented behaviour then?

@tinynumbers
From what I understand, the problem doesn't lie with ActiveAdmin. It's the default sprockets config that includes AA's reset stylesheet. You'll have to either tell everyone that uses AA to change that default config or try to scope the reset as to be ineffective on non AA pages. Which I think is reasonable.

pietschy · 2013-01-30T02:02:19Z

@imb
I'm fairly new to rails, but I don't think the styles are included automatically. The AA reset is contained in /active_admin/_base.css.scss which is imported by app/assets/stylesheets/active_admin.css.scss. If I remove that line from my app all the AA styles go away. From what I understand sprockets only includes a file (or files) if it's included via a manifest or sass import directive.

Hence my contention that the body.active_admin is not required since the premise of the original but report #1343 was wrong. If you don't want the AA styles you shouldn't include them.

Unless I'm wrong of course (c:

tinynumbers · 2013-01-30T14:25:42Z

Hence my contention that the body.active_admin is not required since the premise of the original but report #1343 was wrong. If you don't want the AA styles you shouldn't include them.

I guess I'm agreeing here - and corollary to this, if you want the Active Admin styles scoped by body.active_admin, then do it yourself, but I don't believe the project itself should impose this, since wrapping everything in a body.active_admin tag gives every style in the AA stylesheets a higher precedence than any style of any third party plugins (e.g. CKEditor) that we might want to use in conjunction with AA.

So ideally #1343 would be rolled back and replaced with tweaks to the CSS that allow all of the AA styles to be encapsulated by anyone who wants to do so, but does not impose this.

In the meantime I'm going to be using my fork at https://github.com/tinynumbers/active_admin/tree/unencapsulate-css-global-reset until this gets resolved. Anyone else is free to do so as well. I'll be doing my best to keep it in sync with the master (other than the CSS encapsulation).

pietschy · 2013-01-30T23:02:18Z

@tinynumbers,
Thanks for the fork. I'm stuck with using my own as I have other tweaks. Mine only removes the class around the reset but that worked enough for me. If I can figure out how to pull your changes in I'll give it a go. Here's how Ckeditor looks now.

@pcreux, @gregbell
What would be required to get this changed back?

ArthurN · 2013-02-16T20:56:01Z

Thank you for that fork, @tinynumbers. That did the trick for me, and I haven't seen it affect anything else (including my custom styles and other 3rd party styles). Cheers.

Mayar · 2013-02-18T09:16:47Z

+1 to remove body.active_admin! So annoying...

markterm · 2013-02-21T02:41:27Z

Another +1 from me, using @tinynumbers fork for now ... (thanks for providing it @tinynumbers )

tinynumbers · 2013-03-14T14:41:48Z

For those using my fork to get around the body CSS encapsulation, note that I just merged in the latest changes from the main repository master branch. A number of new features in there, so be warned...

brodock · 2013-03-25T02:57:48Z

👍

brendanstennett · 2013-04-10T19:04:14Z

Why don't we just remove the body.active_admin encapsulation all together and then mention in the Readme that require_tree in application.css will cause conflict with active admin. require_tree is not good practise anyways and shouldn't be used; there is no sense including the entire active_admin js/css files into the main application js/css just for convenience of setup.

seanlinsley · 2013-04-10T19:13:36Z

The require_tree problem has always been a problem, regardless to this issue.

We're currently waiting on feedback from @gregbell over in #1952, so this is on hold for now.

brodock · 2013-04-23T18:19:11Z

👍 I'm using it for sometime and it works pretty well

mwlang · 2013-07-05T23:06:49Z

I ported @tinynumbers patches to rails4 branch. Should definitely avoid polluting the body tag as I struggled with ckeditor, tinymce editor, rich editor, and a few others I was trying before I realized it was Active Admin that was messing things up rather than the wysiwyg editors themselves.

seanlinsley · 2013-07-08T19:41:26Z

This has now been resolved on master. Everyone, sorry that it took so long to fix this.

mwlang · 2013-07-10T01:50:53Z

Excellent! This fixes the issues I was having.

pietschy mentioned this issue Jan 25, 2013

ActiveAdmin 0.5.0 clobbering editor styles kreativgebiet/rich#62

Closed

tinynumbers mentioned this issue Jan 28, 2013

Removed body.active_admin encapsulation for global-reset. #1888

Closed

pietschy closed this as completed Jan 29, 2013

pietschy reopened this Jan 29, 2013

tinynumbers mentioned this issue Feb 28, 2013

Unencapsulate css global reset #1952

Merged

mwlang mentioned this issue Jul 5, 2013

ActiveAdmin + Rails 4 => Overzealous expansion of ckeditor galetahub/ckeditor#286

Closed

seanlinsley closed this as completed in #1952 Jul 8, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defining CSS reset within body.active_admin clobbers third party gem styles #1852

Defining CSS reset within body.active_admin clobbers third party gem styles #1852

pietschy commented Jan 14, 2013

lmb commented Jan 21, 2013

seanlinsley commented Jan 21, 2013

lmb commented Jan 21, 2013

seanlinsley commented Jan 21, 2013

pietschy commented Jan 25, 2013

lmb commented Jan 27, 2013

seanlinsley commented Jan 27, 2013

pietschy commented Jan 29, 2013

lmb commented Jan 29, 2013

pietschy commented Jan 30, 2013

tinynumbers commented Jan 30, 2013

pietschy commented Jan 30, 2013

ArthurN commented Feb 16, 2013

Mayar commented Feb 18, 2013

markterm commented Feb 21, 2013

tinynumbers commented Mar 14, 2013

brodock commented Mar 25, 2013

brendanstennett commented Apr 10, 2013

seanlinsley commented Apr 10, 2013

brodock commented Apr 23, 2013

mwlang commented Jul 5, 2013

seanlinsley commented Jul 8, 2013

mwlang commented Jul 10, 2013

Defining CSS reset within body.active_admin clobbers third party gem styles #1852

Defining CSS reset within body.active_admin clobbers third party gem styles #1852

Comments

pietschy commented Jan 14, 2013

lmb commented Jan 21, 2013

seanlinsley commented Jan 21, 2013

lmb commented Jan 21, 2013

seanlinsley commented Jan 21, 2013

pietschy commented Jan 25, 2013

lmb commented Jan 27, 2013

seanlinsley commented Jan 27, 2013

pietschy commented Jan 29, 2013

lmb commented Jan 29, 2013

pietschy commented Jan 30, 2013

tinynumbers commented Jan 30, 2013

pietschy commented Jan 30, 2013

ArthurN commented Feb 16, 2013

Mayar commented Feb 18, 2013

markterm commented Feb 21, 2013

tinynumbers commented Mar 14, 2013

brodock commented Mar 25, 2013

brendanstennett commented Apr 10, 2013

seanlinsley commented Apr 10, 2013

brodock commented Apr 23, 2013

mwlang commented Jul 5, 2013

seanlinsley commented Jul 8, 2013

mwlang commented Jul 10, 2013