WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 2 weeks ago

#26350 new defect (bug)

!important audit

Reported by: azaozz Owned by:
Milestone: 5.6 Priority: high
Severity: major Version: 3.8
Component: Administration Keywords: 2nd-opinion has-patch needs-refresh
Focuses: ui, css, performance Cc:

Description

After the MP6 merge many CSS rules became "very !important". Some are so !important that overwrite other !important...

Looking at wp-admin.css: in trunk there are 65 !important compared to 22 in 3.7.

Attachments (3)

26350.diff (61.7 KB) - added by adamsilverstein 6 years ago.
remove all !important statements from css
26350.2.diff (45.1 KB) - added by adamsilverstein 5 years ago.
refresh
26350.3.diff (47.9 KB) - added by MikeHansenMe 5 years ago.

Download all attachments as: .zip

Change History (29)

#1 follow-up: @Rhomanu
7 years ago

66 in 3.8.

Agreed, it's not a good practice : Using these force the use of another !important to overwrite a rule, and then it's expanding... While it's just a simple problem of CSS declaration weight.

But it will be long to fix... I'll try a bit tonight (I must find how to contribute without blocking others' work), but there's many CSS files to check.

#2 in reply to: ↑ 1 @bassgang
7 years ago

Hi Rhomanu,

I also want to look into this but do not want to make the work twice, any proposal how to divide it, any progress at your site so far?

#3 @azaozz
7 years ago

  • Milestone changed from 3.8 to Future Release

Yeah, each use of !important spreads exponentially. Every time it needs to be overridden, new !important has to be used.

As much as I'd like to get rid of all !important for 3.8, there is simply not enough time to do it... Hoping we can do this in 3.9.

#4 @nacin
7 years ago

  • Component changed from General to Performance

#5 @nacin
7 years ago

  • Component changed from Performance to Administration
  • Focuses performance added

@adamsilverstein
6 years ago

remove all !important statements from css

#6 @adamsilverstein
6 years ago

26350.diff: Is anything really that !important? and if so, why? This patch removes all !important declaration, please restore where appropriate, with !justification.

@adamsilverstein
5 years ago

refresh

#7 @adamsilverstein
5 years ago

Refresh against trunk.

Left in one !important where the purpose was indicated in a doc block.

@MikeHansenMe
5 years ago

#8 @MikeHansenMe
5 years ago

26350.3.diff is a refresh of 26350.2.diff and includes the 17 new instances of !important that have been added in the last 4 months.

This ticket was mentioned in Slack in #design by voldemortensen. View the logs.


4 years ago

#10 @FolioVision
4 years ago

  • Keywords needs-testing added

This is great work, @MikeHansenMe and @adamsilverstein, thank you. We'd like to help with any review and revision of !important free CSS for WP Admin. We have quite a bit of experience in writing resilient !important free code for plugins. How can my team help finish this revision for core?

#11 @adamsilverstein
4 years ago

@FolioVision Thanks for your offer of help.

Essentially we need to go thru each use of !important and see how its being used, try removing it and determine any consequences, then remove any that can be and document the reason for the remaining uses.

Using svn or git blame type tools we should be able to track the commit that introduced each usage (although this may be difficult given the wholesale restructure the css has undergone in the past); alternately using inspection and code hunting we should be able to determine where the class in question is used and possibly determine why it was added. Its possible many are useful (since we strive to support older browsers), or that they are extraneous - I really don't know.

My farcical proposal was just to 'remove them all' and see what breaks but in reality we probably need to exercise more diligence. If we can't determine a purpose in each case I think we should remove them and see what breaks, however we still need to spend some time trying to determine why they were added. Where we do determine they have a purpose we should add some inline comments in the CSS to indicate why the use is required or desired.

I know this is a somewhat tedious task, however its something we really should undertake and we can take them out little by little so don't feel like you have to check them all to contribute.

Thanks again for your offer of assistance.

#12 @FolioVision
4 years ago

Hi Adam,

Why don't we try to break this task into segments as @bassgang suggested in the early days? It would be great to remove the !important tags for one section of WP Admin successfully and then move on to another section. I'm worried about the idea of doing a huge amount of work to then have it ignored and slide out of date without action (commit).

Is there a particular WP Admin section which would benefit more than others from the absence of !important tags in your opinion?

Thanks, Alec

#13 @foack
14 months ago

Agreed with @FolioVision, especially since wp-admin.css is now broken into multiple files using @import. Working on a fix for wp-admin/css/common.css on the WCEU contributor day right now.

#14 @foack
14 months ago

For reference, I just created this ticket for wp-admin/css/common.css: #47569

Last edited 14 months ago by SergeyBiryukov (previous) (diff)

#15 @FolioVision
14 months ago

Thanks for picking up the banner on this, @foack. Cleaning up Admin CSS is a noble mission, if a thankless one.

#16 @foack
14 months ago

It was a good starting point for me to get involved in the WP development :) Thanks, @FolioVision!

#17 @sabernhardt
7 months ago

  • Keywords needs-testing removed

It's good to start by focusing on the 21 instances in the common.css file, to edit them on ticket:47569.

They are at the beginning of this list of 331 !important styles in WP 5.3.2 core:
https://docs.google.com/spreadsheets/d/1WCuUXfm_WSx7wYFlZ3h8_yLaLxoganyBcPys0TKfoRY/edit?usp=sharing

Column D gives any explanatory notes that are already in each file. If you have something to add to columns E through G, you can use comments within the spreadsheet.

I already marked "leave" for a few of the ones I think are worth keeping (word-wrap:normal for screen-reader-text and skip link styles, as well as call-to-action links without rounded corners when the no-border-radius class is added).

Bundled plugins and themes (currently Twenty Sixteen through Twenty Twenty) are also included, at the bottom, for review later.

#18 @garrett-eclipse
6 months ago

  • Focuses css added

#19 @valentinbora
6 months ago

  • Keywords 2nd-opinion added
  • Milestone set to 5.5

This ticket was mentioned in Slack in #core by marybaum. View the logs.


4 months ago

#21 @marybaum
4 months ago

@tellthemachines maybe make this part of the #core-css audit?

#22 @davidbaumwald
4 weeks ago

  • Keywords has-patch needs-refresh added

The most recent patch doesn't apply cleanly to trunk anymore. Marking this for a refresh. Also, would like to see this addressed by #core-css in Slack, as they have plans for an audit in #49582.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core by whyisjake. View the logs.


2 weeks ago

#25 @whyisjake
2 weeks ago

  • Milestone changed from 5.5 to 5.6

Going to punt to 5.6 as the beta period is ending.

#26 @SergeyBiryukov
2 weeks ago

Just noting that the instances in wp-admin/css/common.css were addressed in [48129] / #47569, but there are still some instances in other files.

Note: See TracTickets for help on using tickets.