Closed Bug 6625 Opened 26 years ago Closed 24 years ago

Redundant rules in ua.css

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: ian, Assigned: ian)

References

()

Details

(Keywords: perf)

Attachments

(1 file)

The following rules are redundant in ua.css: Lines 192, 198, 216, 221: The "a" in the selector is redundant and should be removed. This is especially important for future proofing (eventually, elements other than "a" will be able to be links). Lines 204 and 226: The "a" in the selector should be changed for ":link" and ":visited", so that the selectors read :link:active, :visited:active ...instead of a:active This is also for future proofing (see first comment). Lines 192-203: the "display", "text-decoration" and "cursor" declarations of the :link and :visited rules could be merged, like this: :link, :visited { display: inline; text-decoration: underline; cursor: pointer; } Lines 204-209: the "display", "text-decoration" and "cursor" declarations can be completely removed as they are specified by the :link and :visited rules, because the :active pseudo-class is concurrent with the :link and :visited pseudo-classes. Lines 219, 224, 229, 234: the "text-decoration:none" declarations will have no effect as the underlining from the surrounding links would "override" it anyway. Lines 216-235: All these rules can actually be collapsed into a single rule: :link img, :visited img { border: 2px solid; } This is because the :active pseudo-class is concurrent with the :link and :visited pseudo-classes, and the other declarations can be removed as described in the comments above. Many lines: The "display:inline" declarations on many of the lines are completely superfluous as that is the default value of the display property.
Some other changes that could be made to increase the efficiency and decrease the size of the ua.css: 1. line-height:normal does not need to be defined in the body; it is the default 2. Instead of defining every hx element separately, like h1 { display: block; font-size: xx-large; font-weight: bold; margin: 1em 0; } h2 { display: block; font-size: x-large; font-weight: bold; margin: 1em 0; } it would be better to use h1, h2, h3, h4, h5, h6 { display: block; margin: 1em 0; font-weight: bold; } and then define the font size for each header separately. 3. Elements without any styles applied to them don't need to be included at all. This would include abbr, acronym, del, dfn, ins, q, span; spacer, wbr, :- moz-text, img; :-moz-line-frame, and :-moz-letter-frame. 4. Instead of defining each to be displayed as a block-level element seperately, group :cell-content, :fieldset-content, :button-content, :label- content together with {display: block}. Do the same with {display: none} elements. 5. Use shorthand. Instead of {padding-left : 2px; padding-right : 2px; padding-top : 1px; padding-bottom: 1px;} use {padding: 1px 2px;} 6. For input[type=hidden], dont use {border-none}. The {visibility: none} takes care of the border. (Another thing: a 1px border on iframe looks better than a 2px border. The entire interface of apprunner looks much cleaner.)
Another thing: There are two iframe rules. They should be combined to read iframe { background-color: white; border: 2px solid black; } or even better, border: 1px solid black.
Target Milestone: M10 → M11
Target Milestone: M11 → M20
Deferring this. These are all good suggestions, but as time goes by the file will get messy again. I'll make a cleanup pass shortly before ship.
Regarding comment no 2 from Additional Comments From bloviate@yahoo.com 05/29/99 19:13: In CSS1 specification it is noted that the em is a relative unit and will be inherited as the computed value therefor also the margin setting should be in the seperate H1, H2 etc. rules
Reassigning peterl's bugs to myself.
Accepting peterl's bugs that have a Target Milestone
Let's consider this a Time&Space problem... Reassigned to attinasi. FYI, we have 270 rules in html.css and 78 in xul.css.
Assignee: pierre → attinasi
Status: ASSIGNED → NEW
I believe that Peter's comment that this need to be cleaned-up before shipping is valid, however I also think we should make any valid changes incrementally, as we come across them, so we are getting the most possible testing time with the most relevent rules in place (reduced risk). Thus, I'll start applying these changes soon after beta. NOTE: reducing the number and size of rules helps lower the memory impact of the StyleSytem and can thus be considered an optimization, which is a benefit in addition to that of increasing the maintainability of the document by reducing redundancy.
Status: NEW → ASSIGNED
Target Milestone: M20 → M16
Target Milestone: M16 → M20
Marc: Would you prefer if I went through the html.css file and filed bugs with diffs one change at a time, or would you rather I went through html.css and quirks.css and neatened them both up in one go, and then attached the new files to this bug (or the quirks.css bug) directly?
If you find quirk-rules that should be fixed for PR2 then I suggest you pull them out into separate (new) bugs. General cleanup can go into one big change for this bug. My reasoning is that the PDT team is more likely to approve changes that are small and discrete, and some of the changes are not related to quirk vs. standard mode rule migrations (i.e. they are general cleanup). Sound OK Ian? Thanks!
OK - I guess we shouldn't put this off any longer, it is time to clean the mess up and watch the changes that get checked in from now through RTM
Keywords: nsbeta3
The easiest way to do this would be to lock that file, then I could spend a day cleaning it up and browsing to make sure nothing broke, and then we could get the file checked in. But this requires the file to have absolutely no changes made to it, otherwise they will be lost. Alternatively, we can do this one patch at a time, but that would take ages...
For performance, the key is to reduce the number of selectors.
Do we want to optimize the rules (selectors) as well as remove the redundant ones? That seems like a higher risk proposal, and maybe that should be a seperate task done after the redundancy is eliminated. Ian, I'll look into locking a file: presumable html.css and quirk.css, or do you want to attack xu.css too? Even if we cannot lock them, I think the rate of change is very low now (last change to html.css was mid-June) so we should be able to manage the update with little more than a heads-up email and posting to the community.
I don't know enough about the magic behind the xul selectors to edit xul.css as well, so best leave that one alone. Similarly for parts of html.css like this: :-moz-line-frame { } :-moz-letter-frame { } I assume I either just leave those alone or put them in a comment block at the end of the file?
Hmmm. What good is an empty rule? Maybe it is a reminder? In that case a comment-block is probably best. Also, since Hyatt has considerable expertiese, and since he is best equipped to handle the xul.css file, I'm CC'ing him on this (I believe he has already tuned xul.css anyway).
Keywords: perf
Approving for nsbeta3: this needs to be doen prior to beta3 completion and then locked down for FCS.
Whiteboard: [nsbeta3+]
Marc - I suggest we postpone this until just after nsbeta2 ships. Until then I will be very busy blessing builds and so on. Once nsbeta2 ships, we'll need to announce the ua.css|html.css|quirks.css lockdown and once that is done I can start going through the stylesheets. Also, when you have a moment could you list the best ways of optimising the stylesheets? David mentioned using grouping as much as possible, for example.
Ian, I agree that this should wait a while - in fact, we could wait until the very end of the beta3 cycle (we should leave a week for testing though). I really don't think we should be optimizing too much - the focus should be on removing redundancy and eliminating rules that have no affect. That will be a performance gain too. The more we restructure rules the more risk we take on, so we should minimize that activity and focus on size reduction by elimination of cruft and duplication.
Even though the risk level is low, it is wide ranging since this affects _all_ HTML content. So I'd rather we did it nearer the start of the beta3 cycle. Also, if we leave it to the end then I'll be doing beta3 blessing so we'll have run out of time again. ;-) Question: Is it a bad thing to have more stylesheets linked in from ua.css? I was considering moving the viewsource styles out into another file (they are a bit out of place in html.css). Would that be a bad thing?
Each imported stylesheet has to be file-opened, but other than that I don't think that there is a problem with importing viewsource.css into us.css (for example). As for the timing comments - makes perfect sense :)
Priority: P3 → P2
I'm on it.
Assignee: attinasi → py8ieh=bugzilla
Status: ASSIGNED → NEW
Target Milestone: M20 → M18
I'm also trying to move lots of the quirky rules in html.css to quirk.css. We'll see how that goes.
Status: NEW → ASSIGNED
Priority: P2 → P1
PDT would like to know: is this going to make a real impact on performance or footprint? Or are we just beautifying? Cleanup can wait until after seamonkey.
It will have some impact; how much I do not know, it would need measuring. The work is almost all done anyway, I just need to check it through the top100 and the various CSS test suites before passing it back to Marc for checkin.
The potential for speed and size improvements are good. Rules take up a lot of memory, and processing rules takes a lot of time in page layout and event handling, so by lowering the number of rules we should be able to get both a speed-up and a memory savings, proportional to the percentage decrease in the number of rules - roughly speaking.
There are some very inefficient rules in html.css that would have an impact on style resolution performance of form elements... e.g., stuff like td input { } slows down all input controls. This is bad.
Part of my fix has been to move these rules into quirk.css. Thus only pages using compat mode will be affected. I may remove _some_ of those rules altogether, depending on whether I can find why they were put in in the first place.
Depends on: 3935
No longer depends on: 3935
Ok, I'm about to mail out the proposed new files to everyone who has claimed they want to review the changes before this is checked in. ;-)
No longer blocks: 6625
No longer depends on: 6625
Whiteboard: [nsbeta3+] → [nsbeta3+] [FIX IN HAND]
Depends on: 41252
Ian, ua.css is at risk of not making the commercial product. We really need the changes to be stable and landed very, very soon. Once we start stabilizing for RTM, only super serious P1's will make the cut.
Priority: P1 → P2
Whiteboard: [nsbeta3+] [FIX IN HAND] → [nsbeta3+] [FIX IN HAND][PDTP2]
Ian, wasn't this checked in? Please reopen if I'm mistaken, thanks.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
blake: Yes, it was checked in. However, I had to check that what was checked in was what I had written before marking it fixed. Since you did not know what my final files looked like, you could not check that, so should not have marked it fixed. Having said that: FIXED by halving the size of html.css. Bloat is down a not inconsiderable amount according to Marc's figures. ChrisD: To verify, examine the ua.css, quirk.css and html.css files that are present in the distribution and check that they are now considerably smaller than in previous days.
Per Ian's suggestion, verified that html.css is approximately cut in half. Marking bug verified
Status: RESOLVED → VERIFIED
Whiteboard: [nsbeta3+] [FIX IN HAND][PDTP2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: