Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seutje’s picture

Status: Active » Needs work
FileSize
992 bytes
seutje’s picture

Status: Needs work » Needs review
FileSize
2.41 KB

think I'm about there, I will thoroughly test this to make sure I didn't forget anything

I'd like others to test consistency before & after as well

mgifford’s picture

subscribe

cosmicdreams’s picture

Tested with most recent CVS + Firefox 3.5

patch #2 changes the way #overlay=admin/appearance appears. Before it was a set of divs that float left, after the patch those divs appeared one per line. Was a clear:both applied?

cosmicdreams’s picture

Status: Needs review » Needs work

In my humble opinion the comment I made in #4 means the status of this issue should go to needs work

seutje’s picture

FileSize
4.82 KB

hmm yea, there's also a problem with forms

forms stuff isn't a lot, but the themes page stuff is,

that + the fact that styles got re-arranged in core, it might be interesting not to drop system.css

will have to investigate, in the mean time, here's a patch that copies over the form and themes page stuff from system.css, as u can see... it's quite a lot

thx for the review btw <3

cosmicdreams’s picture

Edit

Found some more issues with patch:

  • on /admin/people the column headings are centered now
  • on /admin/config the two columns of links to configuration settings is now one column
  • on /admin/config/regional/date-time column headers are centered
  • on /admin/config/search/path column headers are centered
  • on /admin/config/search/settings column headers are centered
  • on /admin/config/media/image-styles column headers are centered
  • on /admin/config/system/actions column headers are centered
  • on /admin/config/user-interface/shortcut column headers are centered
  • on /admin/config/people/accounts/fields column headers are centered
  • on /admin/config/people/ip-blocking column headers are centered
  • on /admin/config/content/formats column headers are centered

As you can tell the majority of what seems wrong are that tables have their headers centered. But there seems as if there may be other issues caused by this patch.

However the issue I found on the appearance page seems to be fixed.

cosmicdreams’s picture

Is this patch at a dead end? has other work provided the restraint on Seven's reset.css that you wanted seutje?

seutje’s picture

Assigned: seutje » Unassigned

sorry, focussing more on critical issues right now, feel free to work on this if you want

Jeff Burnz’s picture

Status: Needs work » Needs review
FileSize
4.3 KB

I've had a crack at this. I did not look at the previous patches at all and simply started by removing the entire reset to see what happened - upshot, not a lot.

The way I see this is that the reset is doing a lot of what I might consider "front end" resetting - blockquote, ins, del etc etc - this has no place in Seven, its an admin theme, using this for a front end theme is not our consideration, and some of those will affect perhaps one browser, like Safari, now we're in real edge case territory.

I'd ask this to be taken quite seriously, it removes a tonne of what I think is just pure cruft from the reset, and after a fair few hours I can't see any major regressions. yes I had to add back in some stuff, but IMO those styles are going where they should be, applied to the specific selectors and elements.

I have only tested in FF and IE8, however I will continue testing in IE*, Opera and so on, but would be great to get some feedback, would really like the smoke this out of Seven once and forever.

If you think I'm nuts tell me so, I tend to go off the rails sometimes...

mgifford’s picture

You got rid of a lot of CSS. What do we need to do to evaluate that this doesn't have an adverse effect on Seven? Who should review it?

webchick’s picture

While this patch looks nice, this seems like polish that's going to require extensive testing to ensure we didn't break anything, and likely a flurry of follow-up patches afterwards when we inevitably do.

I think this is D8 at this point, unless you disagree with this assessment.

Jeff Burnz’s picture

Version: 7.x-dev » 8.x-dev

@webchick, I agree - I was throwing this out there to show that it can be done, but I would rather play it safe given the stage we're in - its a very difficult patch to review, too hard for right now imo.

Lets bump it to D8 and come back to it really early in the cycle so we have a long time to account for any regressions.

Mike - this will be very time consuming to review, in essence its one huge before/after comparison, both visual and calculated values. Of course that's unlikely to ever happen (to big task) so we'd go with a "good enough" at some point and clean up regressions.

mgifford’s picture

That's fine with me. I do think that we may well have to retire both Garland & Seven in D8.. Just doesn't make sense to have a theme named Seven in Drupal 8.

Anyways, that's totally a side issue.

markabur’s picture

I'll be up for reviewing this when we get to d8. I love getting rid of unused stuff!

kscheirer’s picture

Well, I make no claims of being a CSS pro (in fact, I claim the opposite), but I rerolled this patch for you against HEAD, so let the reviews and debate begin! :)

cosmicdreams’s picture

Forgive my ignorance here but i have to ask, Why are we including any modifications to seven's style.css here? Isn't this issue solely about reset.css?

JohnAlbin’s picture

Just to throw this out there, but I really like normalizing instead of resetting the styles of HTML elements. http://necolas.github.com/normalize.css/

That'd be one way to tame the reset.css. By replacing it with something better.

mgifford’s picture

Seems like an interesting approach. Any themes presently using this?

seutje’s picture

I've been using this in all production themes since a couple months.

It is a lot less brutal than any reset I know of

Jeff Burnz’s picture

For sure there is some interesting stuff in normalize we could use, however those seem more like feature requests/additions, rather than a strait out fix for this issue.

@17, because in some places the reset actually does have some influence and needs to be accounted for when removed.

JohnAlbin’s picture

however those seem more like feature requests/additions, rather than a strait out fix for this issue.

I'm totally fine with going forward with the approach in the current patch in this issue. I just wanted to gauge interest in Normalize. I don't want to bikeshed this issue; we can move Normalize to another issue.

Any themes presently using this?

Zen 7.x-5.0. :-) See also http://drupal.org/project/normalize

webchick’s picture

I can't evaluate normalize from a technical POV, but it sounds like it would let me as a non-CSS guru start with something sensible and work from there, rather than start from _nothing_ and have to build up from there. +100 to that.

seutje’s picture

That's exactly what it tries to do, instead of trying to reset everything to 0, it normalizes things across the various browsers.
You can easily try it out by visiting http://necolas.github.com/normalize.css/demo.html in different browsers and comparing the results.
The only discrepancies you might see are things the browser simply doesn't support (like range inputs, inline SVG, etc.).

ry5n’s picture

I'm about to start modifying Seven's style.css to support http://drupal.org/node/1510532 and want to work on top of a reset that I know is stable. I'd like to resolve this issue either beforehand, or in parallel. I'm fine with either the patch re-rolled in #16 or with normalize.css (which I have been using in production for a while). I'm seeking some consensus, judged pretty much by how far we can get before feature freeze.

Jeff Burnz’s picture

If someone can roll a patch that uses normalize and we don't get any glaring issues (minor stuff we can fix of course), then all power to that, I'll be happy just to see the reset removed.

ry5n, if you can roll such a patch I'm happy to jump in and test it strait away so this is not blocking you.

ry5n’s picture

FileSize
10.67 KB

@Jeff Burnz Patch attached. It removes the existing reset completely and replaces it with normalize, tweaked in the following ways:

  • Comments and whitespace follow Drupal coding guidelines;
  • `abbr[title]` rule removed as it clobbers abbr.form-required from system.theme.css, and the latter is probably worth keeping over the former in an admin theme;
  • A stub section is added at the bottom for resetting Drupal core styles.

After applying this, I did some cursory testing (Chrome only) and there are minor reversions, but nothing major. I wager that's worth dealing with.

mgifford’s picture

#27: 723392-27.patch queued for re-testing.

mgifford’s picture

So at this time we just need some screenshots (I'm not sure of what or how many) so that we can verify that this works and can come into core, right?

ry5n’s picture

Attaching updated patch which brings in some changes from http://drupal.org/node/1747868 (especially loading the reset first, before all other stylesheets). Also updates normalize (which this basically is) to the current 2.0.1 version. This no longer supports IE 6/7 which is consistent with release support for D8.

Images attached of screenshots in Chrome (latest stable, Mac). A few representative screens are attached directly, the rest zipped. I can provide screens from other browsers if necessary, just not right this moment.

Screens show obvious regressions only with the secondary tabs. Not sure what to do with regressions – should they be fixed in this patch?

ry5n’s picture

Actually this is what is looks like in 8.x HEAD, too, so actually no regressions in Chrome Mac AFAICT.

ry5n’s picture

Issue tags: +CSS, +Usability, +Seven, +d8ux, +d8dx, +#theme, +d8mux

Adding tags.

joelpittet’s picture

Nice suggestion #18 JohnAlbin to replace reset with normalize! And ry5n for making the patch.

The tabs spacing evened out after applying it, and didn't notice anything go awry.

I also use it for all my production sites and drupal production themes.

ry5n’s picture

Some additional screenshots of this patch in IE9 (Win 7). Everything seems consistent with current state.

markabur’s picture

Is it intentional to have extra borders around the secondary tab buttons? Currently in d7 and d8, Seven doesn't have those. Also, the active button in the screenshots above looks incorrect, with that fat bottom border.

ry5n’s picture

@markabur The secondary tabs don't look right, but this is how they appear with the *existing* reset, as well. This regression isn't due to this patch, AFAICT.

mrfelton’s picture

Applied the patch, had good look around comparing against the current D7. Didn't notice any issues.

ry5n’s picture

Do we need more reviews to mark this RTBC?

ParisLiakos’s picture

Status: Needs review » Needs work

hmm can we have a reroll?
patch no longer applies

ry5n’s picture

Rerolled.

ry5n’s picture

Status: Needs work » Needs review
FileSize
9.16 KB

Back to needs review, so it will test. Also should this be major, since it is holding up http://drupal.org/node/763720 (which is major)? This patch is also needed for http://drupal.org/node/763720. Please correct me if I'm wrong on priority.

cosmicdreams’s picture

Code looks good to me, will test this tonight.

ParisLiakos’s picture

not a css expert but tested it on both firefox and chrome and i dont see anything broken

ry5n’s picture

@cosmicdreams Did you have a chance to test this?

sun’s picture

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

Seven's utterly insane reset.css always bugged me like hell, so I couldn't agree more with this issue.

While that reset.css is insane, the fact that Seven loads it after all base and module CSS is what ultimately drives you into Maniac Mansion™.

That's the most critical bug, and it is exciting to see that this gets finally fixed here.

That said:

I had to download the original normalize.css from http://git.io/normalize to see why the reset.css initially says:

* Based on normalize.css v2.0.1

(emphasis mine)

Fact is, this file is not based on it, it is the original normalize.css. It only has tons of CSSDoc and coding style adjustments all over the place. But the actual CSS is identical.

We do not want to do that. Instead, we want to be able to plug in a newer version of this file whenever a newer version is available. It is an external CSS library. Like any other.

And apparently, this library could be extremely helpful for themers in general. I don't get why we're obfuscating and hiding it away in Seven theme.

Thus, I went ahead and did this:

  1. Added normalize.css as external library.
  2. Replaced reset.css with normalize library.

Since that only involves a technical/architectural change, but no change to the actual front-end result, I also went ahead and double-checked whether Seven still looks correct. And it does.

I'm therefore marking this patch directly RTBC.

Thanks for the awesome work, everyone!

ry5n’s picture

@sun Excellent! EDIT: Let me review this and sum up the changes in the patch for committers. "Excellent" is not a patch review.

sun’s picture

Let me also amend that we still have ~1 year to fix any possible glitches and flaws in themes.

Contrary to that, this change here can be considered to be bound to feature freeze, since it essentially is code thaw material, as it revamps the fundamental styling (reset) of Seven theme.

Let's make sure to get these kind of changes into core before feature freeze (Dec 1st). After that, there's little hope/chances for getting such changes in, and our work will have to focus on correcting (NOT: improving) the front-end experience where needed.

That's an important differentiation to make, and we apparently do not have clearly documented guidelines for theme changes/improvements with regard to feature freeze and code freeze. Thus, from my perspective, this is the most reasonable and sensible interpretation that could be applied; i.e., changes/improvements before feature freeze, only fixes are allowed after.

So let's bear in mind that we have almost an entire year for polishing of theme stuff.

ry5n’s picture

So, being relatively new to Drupal core development, I kept thinking: "normalize should be available to all themes", but I did not know how. The latest patch does it. Thanks @sun!

The details of the changes in #45 are:
- Add the complete, unaltered normalize.css library to /core/misc/normalize/ just like any other library. The CSS itself is the same as in all patches since #30.
- Delete core/themes/seven/reset.css
- Remove mention of reset.css from Seven's theme.info file.
- Register the normalize library in system.module (system_library_info()) with a weight of -10.
- Actually add the library's css to the Seven theme (Seven's template.php)

Normalize itself has been carefully written, tested and validated by the front-end community at large. I've been using it as my reset with Drupal for a year now.

As far as the supporting code for Drupal, everything works correctly in my testing with Normalize added first, before all other css including system.css, and only in Seven. The Seven theme continues to show no visual regressions. The patch as a whole solves the heavy-handedness of Seven's original reset and creates a great asset for other themes at the same time.

Disclaimer: I was not familiar with the process of adding external libraries in module code, but after consulting the docs for the few lines of code I needed to check, this looks good. Leaving as RTBC.

seutje’s picture

@#45: Excellent idea, why didn't I think of that?

JohnAlbin’s picture

Hell, yes! I would love to deprecate my contrib module. http://drupal.org/project/normalize

I've reviewed the patch and its RTBC by me, except for one complaint. This hunk:

--- a/core/themes/seven/template.php
+++ b/core/themes/seven/template.php
@@ -21,6 +21,7 @@ function seven_preprocess_maintenance_page(&$vars) {
  * Implements hook_preprocess_HOOK() for html.tpl.php.
  */
 function seven_preprocess_html(&$vars) {
+  drupal_add_library('system', 'normalize');
   // Add conditional CSS for IE8 and below.
   drupal_add_css(path_to_theme() . '/ie.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE), 'weight' => 999, 'preprocess' => FALSE));
 }

I would much prefer to add normalize.css before every other CSS file at a system module level. (Using CSS_SYSTEM with a follow-up to change CSS_SYSTEM to CSS_MODULE with a negative weight.) In other words, normalize.css is added by the system and used (by default) by all themes, not just Seven.

What do people think of that idea? And do we want to do that now or as a follow-up?

sun’s picture

@JohnAlbin: Definitely a separate follow-up issue. (And I will object to that. ;))

Also, to clarify on CSS_SYSTEM, I explicitly tested that normalize.css is in fact output as the very first library on the page. It is output before System module's base CSS files, but in the same group. If aggregation is enabled, those files are bundled into a single aggregate.

ry5n’s picture

@JohnAlbin I'm open to discussing it, but definitely in a follow-up.

webchick’s picture

Title: Tame seven's reset.css » Change notice: Tame seven's reset.css
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. This was ry5n's first core patch. :) Let's all give him high fives at BADCamp! :D

We should get a change notice that normalize.css is now available to theme authors. I believe I also land on this being "opt-in" since there could be other similar libraries that pop up over the next 3-5 years, and that would allow themers to choose their solution without undoing core assumptions.

sun’s picture

Title: Change notice: Tame seven's reset.css » Tame seven's reset.css
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record
tim.plunkett’s picture

Priority: Normal » Critical
Status: Fixed » Needs work

This causes major visual regressions in the following places:

  • Menu UI
  • Block UI
  • Views UI
  • All form elements everywhere

We can keep the library, but we should restore the reset.css until this is actually tested.

ry5n’s picture

@tim.plunkett Oh, crap. I clearly missed some important screens in testing. I did test (Chrome and IE9, screenshots above), so I am not sure how I missed global form regressions… But that's beside the point. I apologize for the mess.

I will post a patch that puts back the old reset, leaving Normalize in. I'll then start work on fixing the CSS in the UIs above so we can switch back to using Normalize. Does that sound good?

tim.plunkett’s picture

@ry5n, yes that sounds perfect.
Sorry for my terse post, it was from my phone.
By "until this is actually tested" I didn't mean to imply that this wasn't tested at all, but rather more fully on some more complex screens.

The main thing I noticed was that .form-item, especially radio buttons, went from having 0px padding to 9px or more. The other thing was weird borders on pages with tabledrag (Menu, Block).

ry5n’s picture

Assigned: Unassigned » ry5n

(My turn to post from my phone.) @tim.plunkett Thanks for the additional details, that's helpful. I will post the initial fix tonight.

ry5n’s picture

Assigned: ry5n » Unassigned
Status: Needs work » Needs review
FileSize
4.39 KB

This patch restores Seven's previous reset.css and switches back to using it instead of Normalize, leaving the library itself in Core. I will start work right away on fixing visual regressions so that we can use Normalize in Seven as soon as possible.

tim.plunkett’s picture

Priority: Critical » Normal
Status: Needs review » Fixed

Ahhh! I take it all back.

It seems the Block UI was messed up in #1813792: Remove ugly default CSS styles for table
And Views UI in #1826574: Move Views theme-specific CSS to their respective themes

ry5n’s picture

@tim.plunkett I checked the before/after on the core commit before I posted #56, and the Blocks UI does get a bit worse. I missed it in testing. I didn't immediately see any other regressions (in forms, Views or Menu UI), but I figured your suggestion was a wise idea. What do you think? Should we still apply #59, or just do a thorough re-test and fix stuff in follow-up?

ry5n’s picture

@tim.plunkett Actually just to make sure I'm not seeing things, could you confirm that Blocks UI is worse than before the Normalize commit? Thanks.

sun’s picture

The Block admin UI looks fine to me.

I just wanted to provide a patch for the form items in Seven, which indeed have too much padding/spacing...

...but it turns out that the patch in #1168246: Freedom For Fieldsets! Long Live The DETAILS. will remove those styles entirely in favor of the system defaults and to achieve a correct styling of form elements in details.

So all we need to do is to get the patch over there in core — and I'm *desperately* waiting for an RTBC + commit.

ry5n’s picture

I created a follow-up issue to take care of some of the minor visual regressions caused by this patch. The major one is for blocks UI, and I will be posting a patch for that shortly.

Status: Fixed » Closed (fixed)
Issue tags: -CSS, -Usability, -Seven, -d8ux, -d8dx, -#theme, -d8mux

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