Closed Bug 6625 Opened 25 years ago Closed 24 years ago

Redundant rules in ua.css

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: ian, Assigned: ian)

References

(
URL
)

Details

(Keywords: perf)

Attachments

(1 file)

Cleaned-up ua.css (3k smaller than the current one!)
13.56 KB, text/plain
Details 
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: