Make WordPress Core

Opened 10 years ago

Closed 3 years ago

#26350 closed defect (bug) (fixed)

!important audit

Reported by: azaozz's profile azaozz Owned by:
Milestone: 5.8 Priority: high
Severity: major Version: 3.8
Component: Administration Keywords:
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 (4)

26350.diff (61.7 KB) - added by adamsilverstein 9 years ago.
remove all !important statements from css
26350.2.diff (45.1 KB) - added by adamsilverstein 9 years ago.
refresh
26350.3.diff (47.9 KB) - added by MikeHansenMe 9 years ago.
important-override.png (40.2 KB) - added by dingo_d 3 years ago.
Image that shows how impoortant in the core css overrides the color of the icon in the editor

Download all attachments as: .zip

Change History (48)

#1 follow-up: @Rhomanu
10 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
10 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
10 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
10 years ago

  • Component changed from General to Performance

#5 @nacin
10 years ago

  • Component changed from Performance to Administration
  • Focuses performance added

@adamsilverstein
9 years ago

remove all !important statements from css

#6 @adamsilverstein
9 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
9 years ago

refresh

#7 @adamsilverstein
9 years ago

Refresh against trunk.

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

@MikeHansenMe
9 years ago

#8 @MikeHansenMe
9 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.


8 years ago

#10 @FolioVision
8 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
8 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
8 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
5 years 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
5 years ago

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

Last edited 5 years ago by SergeyBiryukov (previous) (diff)

#15 @FolioVision
5 years ago

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

#16 @foack
5 years ago

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

#17 @sabernhardt
4 years 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
4 years ago

  • Focuses css added

#19 @valentinbora
4 years ago

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

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


4 years ago

#21 @marybaum
4 years ago

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

#22 @davidbaumwald
4 years 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 years ago

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


4 years ago

#25 @whyisjake
4 years ago

  • Milestone changed from 5.5 to 5.6

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

#26 @SergeyBiryukov
4 years 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.

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-css by hellofromtonya. View the logs.


3 years ago

#30 @sabernhardt
3 years ago

  • Keywords needs-patch added; 2nd-opinion has-patch needs-refresh removed
  • Milestone changed from 5.6 to 5.7

This is part of the #core-css team's audit, and another fix should be achievable in the 5.7 cycle.

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-css by danfarrow. View the logs.


3 years ago

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


3 years ago

This ticket was mentioned in Slack in #core-css by danfarrow. View the logs.


3 years ago

#35 @danfarrow
3 years ago

In #core-css much progress has been made on the automated CSS audit tool.

The tool generates an HTML report of the results of several audits run on WP core, including an audit of all uses of !important.

The `!important` audit report can be viewed here.

It would be great to get feedback on this part of the audit in relation to this ticket.

Thank you!

This ticket was mentioned in Slack in #core-css by danfarrow. View the logs.


3 years ago

#37 @dingo_d
3 years ago

In the wp-includes/css/dist/block-editor-style.min.css there is a

.components-button.block-editor-block-switcher__no-switcher-icon:disabled .block-editor-block-icon.has-colors{color:#1e1e1e!important}

Which turns all the icons in the editor for blocks black (dark gray). Not sure why, as I don't see this in the GB on github 🤷🏼‍♂️

This is grating because when you are building your own blocks and icons, they are turned black, even though you set the color to be white for instance (when you want to switch one block to another).

We have fills in the icons set to currentColor so that the background of the icon can come through and it looks seamless in the block picker.

Last edited 3 years ago by dingo_d (previous) (diff)

@dingo_d
3 years ago

Image that shows how impoortant in the core css overrides the color of the icon in the editor

#38 @sabernhardt
3 years ago

  • Milestone changed from 5.7 to 5.8

@dingo_d Could you create a GitHub issue for the custom icons (or should I)? The !important had been necessary to change the style when those buttons are in the disabled state. GB16390 added a gray background color and then GB19344 removed it.

Milestone edited to 5.8 for creating any patches after the audit conducted during 5.7.

Last edited 3 years ago by sabernhardt (previous) (diff)

#39 @dingo_d
3 years ago

Sure, go ahead and open the issue, I'm a bit busy atm with work stuff so I don't have that much time unfortunately :/

#40 @sabernhardt
3 years ago

Editor block switcher issue: GB29130

#41 @JeffPaul
3 years ago

@sabernhardt it looks like GB29130 was resolved, are there any additional tasks to fully resolve the audit that @azaozz opened in this ticket?

#42 @sabernhardt
3 years ago

GB21930 was a separate issue, but this is the new location for the CSS audit:
https://wordpress.github.io/css-audit/public/wp-admin#important

That audit does not identify further specific changes to make.

If this ticket should stay open, it might be better in the Future Release milestone. Either open or closed, it would be good to add a comment here whenever a ticket is opened for a smaller related task.

#43 follow-up: @JeffPaul
3 years ago

@azaozz sounds like this is probably safe to close now that the related audit has completed and no further changes have been identified unless you feel there's still more to track here?

#44 in reply to: ↑ 43 @azaozz
3 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from new to closed

Replying to JeffPaul:

Right, time to close this ticket. Fixing/removing redundant !important would be better in separate tickets, case by case.

The WordPress admin CSS audit can be found at https://wordpress.github.io/css-audit/public/wp-admin. Thanks to everybody that made this possible!

Note: See TracTickets for help on using tickets.