43220 - [CASCADE]author !important rules override user !important rules in user.css

Status: VERIFIED FIXED

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

Description

[fantasai]

Overview:

     !important style rules in the author stylesheet override !important style
     rules in the user stylesheet ([profile]/chrome/user.css); a direct
     contradiction of the CSS spec (CSS2:6.4)

Steps to Reproduce:

     1.) Copy 'Sample user.css' to the 'chrome' subdirectory in your profile
         directory for Mozilla. Rename it to 'user.css'.
     2.) View 'Testcase' in Mozilla.

     * Both to be attached shortly

Tested with build id=2000061720 on Windows 2000

Comment 1

[fantasai]

Attachment: Sample user.css

Comment 2

[fantasai]

Attachment: Testcase

Comment 3

[Stephen Walker]

confirming using 2000062008 nightly build

Comment 4

[Tim Larson]

Recommend that Severity and Priority be bumped upward (Major?, P2?).  If authors 
can override user preferences, we're going to have alot of sites becoming 
inaccessible to those with special viewing requirements.  (Funky color schemes 
and teeny-tiny fonts, for instance.)

Comment 5

[fantasai]

Bug 22963 may be related to this. I'm not familiar with how the color 
preferences filter into the style system, so I can't be sure.

Comment 6

[Hixie (not reading bugmail)]

Possibly I shouldn't say this, but since the rules changed between CSS1 and
CSS2, and we are committed to supporting CSS1 and not CSS2, standards compliance
is not a big reason to fix this.

However, accessibility _is_ a reason to fix this. (There *may* even be legal
implications to not fixing this, I don't know.)

Pierre: Any idea how difficult this would be to fix?

Comment 7

[Marc Attinasi]

From looking at the code that loads the user.css file I see that it treats it 
(user.css) just like any other user agent stylesheet and not like a user 
stylesheet. That's why the cascade is wrong.

nsChromeRegistry::ConvertChromeURL calls
nsChromeRegistry::LoadStyleSheet which calls
nsChromeRegistry::LoadStyleSheetWithURL which calls
CSSLoaderImpl::LoadAgentSheet

What really needs to happen is that the user.css needs to be loaded and then 
appended to the styleset as an Override stylesheet.

The bug is that user.css is loaded as an agent stylesheet (see nsChromeRegistry) 
and it needs to load it as a Override stylesheet. This should probably go to 
Hyatt since he implemented user.css loading.

Comment 8

[Marc Attinasi]

An example of loading a stylesheet as an Override sheet is in the HTMLEditor:

http://lxr.mozilla.org/seamonkey/source/editor/base/nsHTMLEditor.cpp#4128

Comment 9

[David Baron :dbaron:]

Are you sure it's that simple?  The non-important rules in the user stylesheet 
should still be overridden by the author stylesheet.  (Even if changes to the 
style system are required, they shouldn't be that hard...)

Comment 10

[Marc Attinasi]

My guess is that the cascading is already implemented correctly for override 
stylesheets... That is a guess, but since we have three classes of stylesheets, 
Doc, Override and Backstop, I was kinda thinking they mapped to Author, User and 
Agent stylesheets and would cascade accordingly. It could be that Peter never 
really implemented user stylesheets, or never completed the implementation, and 
if that is the case then we will have some bugs to fix. Hopefully David is 
correct and they won't be very hard.

Comment 11

[Pierre Saslawsky]

Marc is right. The mapping is:
      Override    User
      Doc         Author
      Backstop    User Agent

And this is how they are currently used:
      Override    EditorOverride.css + EditorContent.css +
                  EditorAllTags.css + EditorParagraphMarks.css
      Doc         in documents
      Backstop    UA.css + user.css + skins

I guess user.css should be loaded as an override style sheet.

(Marc: we can't say that Peter never completed the implementation. He did what 
the style system is supposed to do. It is up to the application to provide 
mechanisms to load and enable/disable style sheets.)

Comment 12

[Marc Attinasi]

I didn't mean to suggest that Peter left user stylesheets incomplete. I was 
merely expressing my concern that since their was no user style sheet being 
loaded while Peter was here that maybe there were some loose-ends in the 
implementation. My hope is that he implemented it like he did most things, 
correctly and efficiently.

Comment 13

[Erik van der Poel]

Keep in mind that for !important rules, the following precedence holds:

  user > author > UA

But for rules that are not marked !important, it is:

  author > user > UA

So, the user style sheet does not always override. It depends on the absence and
presence of "!important" in the individual rules. I have no idea whether the
style system implements this.

Comment 14

[fantasai]

See also bug 45850 - user stylesheet is not overriding ua.css
and bug 45852 - !important in ua.css overrides normal rules in author style

This verifies that user.css is located in the User Agent level, and should not 
be there.

How are the normal and !important author rules holding themselves on different 
levels? They both fall under Doc, don't they? Here, you say there are three 
labeled cascading levels. But there are more than that; the !important rules 
each get their own level (sublevel?), and it's not just an increase in 
specificity, as the User Agent level demonstrates by tossing its !important 
rules in between the Author styles.
The whole thing baffles me. (Probably because I can only infer what's going on.)

AFAICT (which isn't much), there should be six distinct cascading levels; seven, 
if you include editor override. The problem here is that we don't have that.

BTW, this is where I'm pulling most of these bug reports from:
news:[e-mail]
fantasai. "Re: user stylesheet". June 26, 2000. n.p.m.style

Comment 15

[Peter Trudelle]

need info: Marc, why did you reassign this to hyatt?  Why do we need to do this
in N6, and who should own it?

Comment 16

[David Hyatt]

I'm the one that implemented user stylesheets. :)

Comment 17

[David Baron :dbaron:]

The bulk of the work to fix this *may* still be at the style system end...

Comment 18

[Marc Attinasi]

I reassigned this to Dave because he put in the user stylesheet loading code, 
and there is a small problem there in that we need to actually load the user 
stylesheet as an OVERRIDE sheet, not a doc sheet (see comment Marc Attinasi 
2000-07-07 16:20). 

As to David Baron's comment: can you explain what you are alluding to in your 
comment (David Baron 2000-08-04 09:03)?  I can only guess with elliptical 
comments like that...

I am not sure if we need this for N6. Since user.css has to be edited by hand 
anyway it is not clear that very many users will even utilize a user stylesheet.  
If we are going to have user.css, then it should be loaded as a user stylesheet 
and not a doc sheet, and !important rules should cascade correctly. If we do not 
fix the loading and/or cascade problems then we should remove the loading of 
user.css - better nothing than blatently wrong.

Hyatt, I'm gonna look into how hard it is to load the user.css as an override 
sheet - I bet it is pretty simple since most of the loading code is in place 
already, and we probably just need to flag to indicate which type of sheet ot 
load it as.

Comment 19

[David Baron :dbaron:]

What I'm alluding to (as pointed out in earlier comments) is that Override
stylesheet *sounds* like it's going to do the wrong thing for non-!important
rules (where we are doing the right thing now).  That sounds like an even more
serious problem than this one.

Has anyone tested or looked at the code to see how override stylesheets work?

Comment 20

[David Hyatt]

Two comments:

(1) I only really intended the chrome/user.css file to be for chrome docshells.  
As others have pointed out, there really should be two user.css files, one for 
chrome and one for content.  The chrome registry only has the job of giving you 
the chrome user stylesheets.  We should just have a user.css that sits 
immediately under the profile dir that is used for content.

(2) The chrome registry is giving back two stylesheets that it wants inserted, a 
scrollbars stylesheet and the user stylesheet.  One of these is an agent sheet 
(like ua.css) and the other is a user sheet.  The same method is used to get 
back an array that contains both of these from the chrome registry.  They're 
both then inserted as agent sheets.  This is wrong.  The chrome registry should 
either have two methods, or you should query the type of the stylesheets you get 
back from it and use that to put the sheets in the appropriate array (backstop, 
override, etc.).

Comment 21

[Pierre Saslawsky]

David, do you need some help? I'd vote to store the stylesheet type in the chrome 
registry. It would be nice to change the filenames too (userChrome.css + 
userContent.css instead of 2 user.css).

Comment 22

[David Hyatt]

David is right.  Look in nsStyleSet.cpp at the function GatherRuleProcessors.  
It states that the ordering is from least significant to most significant.  It 
clearly considers the ordering for non-important rules to be (from least to most 
significant)...

Backstop --> Document --> Override

which translates to 

Agent --> Author --> User

So for non-important rules, this would be wrong.  This is why I made user.css a 
backsotp sheet, since it's better for the non-important rules to work out of the 
gate than the other way around.

Comment 23

[Pierre Saslawsky]

Hyatt: I suggest to leave the stylesheet where it is and reassign this bug to 
myself in order to fix the problems with the cascade between stylesheets and 
important vs normal rules. We have 6 or 7 bugs related to that topic, approved en 
bloc for beta3.

On your side, you can open a new bug in order to implement 2 user-modifiable 
stylesheets: 1 for the chrome, 1 for the content.

Comment 24

[Peter Trudelle]

nsbeta3+. Dave will do his part,then hand off to Pierre

Comment 25

[Daniel Veditz [:dveditz]]

Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so 
the queries don't get all screwed up.

Comment 26

[David Hyatt]

Mass-accepting.

Comment 27

[fantasai]

> On your side, you can open a new bug in order to implement 2 user-modifiable 
> stylesheets: 1 for the chrome, 1 for the content.

Already done: bug 45567

Comment 28

[fantasai]

The user styles and the ua styles are currently both loaded into the same 
cascading mechanism. As such, Mozilla not only treats their !important rules 
identically, but also cascades the rules within the same level. This is 
demonstrated by this bug, bug 45852, and bug 45850. I believe that the style 
rule handling in Mozilla is inadequate; it's lacking a separate user stylesheet 
loading/cascading mechanism. If I was any more certain (i.e. actually understood 
what's going on), I would file a bug on the whole problem and mark dependencies 
instead of just reporting the symptoms.

FYI, this is the current primary sort order:

	1. UA & User normal rules             = Backstop  normal
	2. Author normal rules                = Document  normal
	3. UA & User !important rules         = Backstop !important
	4. Author !important rules            = Document !importnat
	5. Override normal rules              = Override  normal
	6. Override !important rules          = Override !important

Could someone post a list of what it ought to be?

Comment 29

[Hixie (not reading bugmail)]

The order should, as I understand it, be:

        [least important]
        1. UA normal rules                    = Backstop  normal
        2. User normal rules                  = User      normal
        3. HTML formatting implied rules      = HTML
	4. Author normal rules                = Document  normal
	5. Author !important rules            = Document !importnat
	6. User !important rules              = User     !important
	7. Override normal rules              = Override  normal
	8. Override !important rules          = Override !important
        9. UA !important rules                = Backstop !important
        [most important]

User rules may be user.css or chrome-user.css depending on what the target is.
In addition, XBL introduces extra rules at all points in the cascade depending
on where the relevant '-moz-binding' property was set (this will probably only 
apply to XBL 1.1 -- Hyatt?).

The HTML formatting rules are the implied rules that <font>, bgcolor=, border=,
and so on, introduce. Since we define <b>, <u>, <h1>...<h6>, etc, in the ua.css
stylesheet, I guess the dividing line is a little fuzzy. ;-)

The 'style' attribute falls into the "Author normal rules" category.

Question: What were 'override' sheets originally supposed to be?

David: Could you check all that please? ;-)

Comment 30

[David Hyatt]

Could override sheets have to do with the CSS DOM?  I see them mentioned in the 
DOM Level 2 draft.

Hixie, yes, for now XBL will only treat its sheets as scoped author-level 
sheets.

I am now reassigning this bug to pierre.  I have modified the chrome registry to 
have a new API, GetUserSheets(PRBool aGetChromeSheets, nsISupportsArray** 
aResult).  You'll get either resource:/chrome/userContent.css or userChrome.css 
depending on the bool.

I've also patched the code in nsDocumentViewer.cpp that originally only asked 
for backstop sheets to also ask for user sheets.  For now, I've preserved the 
way the code originally worked, i.e., the user sheets are also inserted as 
additional backstops.  Pierre, you should be able to just tweak the one line 
of code that inserts them as backstops and insert them as something else 
instead (once we've all worked out what that something else should be of 
course!).

Enjoy!

Comment 31

[karnaze (gone)]

Changing to nsbeta3- due to high risk.

Comment 32

[Pierre Saslawsky]

Block moved M18 bugs to mozilla0.9.1

Comment 33

[fantasai]

I made a mistake in testing the override stylesheets. The current cascade list 
should read as follows:

        1. UA & User normal rules             = Backstop  normal
        2. Author normal rules                = Document  normal
        3. Override normal rules              = Override  normal
        4. UA & User !important rules         = Backstop !important
        5. Author !important rules            = Document !importnat
        6. Override !important rules          = Override !important

I think I know why. In WalkRuleProcessors, the stylesheets are loaded into an 
array of matching 'rules' in the order Backstop, Doc, Override. The calling 
function then sorts the 'rules' by importance in a separate sort function, 
SortRulesByStrength. The function goes through the 'rules' array in forward 
order and relegates any "strength > 0" (which I'm guessing means !important) 
rules to the end. Since the rules are in the order Backstop, Doc, Override to 
begin with, that order is duplicated with the !important rules, giving the 
result described above.

Comment 34

[Hixie (not reading bugmail)]

My list above is wrong. It should be:

   [least important]
    1. UA normal rules                    = Backstop  normal
    2. User normal rules                  = User      normal
    3. HTML formatting implied rules      = Mapped Attributes
    4. Author normal rules                = Document  normal
    5. Style attrs (in quirks)            = style=""  normal
    5. Style !important attrs (in quirks) = style="" !important
    7. Author !important rules            = Document !important
    8. Override normal rules              = Override  normal
    9. Override !important rules          = Override !important
   10. User !important rules              = User     !important
   11. UA !important rules                = Backstop !important
   [most important]

Comment 35

[Hixie (not reading bugmail)]

Gah, got it wrong _again_!

   [least important]
    1. UA normal rules                    = Backstop  normal
    2. User normal rules                  = User      normal
    3. HTML formatting implied rules      = Mapped Attributes
    4. Author normal rules                = Document  normal
   (5) Style attrs                        = style=""  normal
    6. Author !important rules            = Document !important
   (6) Style !important attrs             = style="" !important
    8. Override normal rules              = Override  normal
    9. Override !important rules          = Override !important
   10. User !important rules              = User     !important
   11. UA !important rules                = Backstop !important
   [most important]

...where 5 and 6 are only considered separate levels (which can also be viewed
as saying they have "infinite specificity") in quirks mode.

Comment 36

[karnaze (gone)]

Moving to m0.9.2

Comment 37

[karnaze (gone)]

Moving to m1.0

Comment 38

[fantasai]

I'm going to try to fix this within the next two months. Can't guarantee I'll 
come up with something usable, but we'll see..

What's the [need info] for?

Comment 39

[fantasai]

Attachment: patch?

Comment 40

[fantasai]

Attachment: Extended Testcase

Comment 41

[fantasai]

The patch fixes this bug, bug 45850, and bug 45852 by implementing the cascade 
order specified by Ian Hickson above and commented in FileRules. It properly 
renders the Extended Testcase. I request review, but I warn you that my 
understanding of programming is sketchy.

Comment 42

[Marc Attinasi]

Pretty good patch foe someone who claims little knowledge of coding...

I'll try to review this properly, but Pierre really should check this over, he 
is the module owner of Style. I'll post my comments here as I review.

Also, what testing have you done besides the extended testcase?

Comment 43

[fantasai]

>Pretty good patch foe someone who claims little knowledge of coding...

Made possible by David Hyatt's generous contribution on 2001-05-31. Most of what 
I wrote is copy/paste.

I have done approximately no testing other than the two testcases above. I would 
have run the table and block regression tests, but I kept getting asserts when I 
tried to run baseline. Since then I have downloaded and built on a clean 
directory, but when I ran the table tests for another patch, I found that it 
would not report style data mismatches.

Comment 44

[Marc Attinasi]

I thought David put the style data back in already??? I'll check that out. You 
have been running with the changes though, right?

Comment 45

[fantasai]

I built code downloaded last Friday/Saturday, without the above changes. The 
style data was correctly printed into the rgd files, but the mismatches weren't 
reported.

I ran with the changes only for a few days. Then, as I said, I wiped out the 
entire directory. Although I compile on Linux, I generally use Windows, so no, 
the patch didn't see much use.

Comment 46

[fantasai]

Attachment: net regression test mismatches (style differences only)

Comment 47

[Marc Attinasi]

The patch looks fine to me. We need to run through Ian and David's tests - maybe
Ian can help? Also, we need to make sure the chrome (modern particularly) is
looking correct and good - assuming the testing goes well, this should be
checked in - it is nice work.

Comment 48

[Hixie (not reading bugmail)]

Make me a win32 test build and I'll look into it... what's the time frame for 
this? 0.9.2? nsBranch? 0.9.3? 0.9.4?

Comment 49

[fantasai]

I couldn't find an archive of Ian or David's tests, so I downloaded a zip of  
Glazman's CSS2 test suite and ran that. I don't think it helped much; I found a 
considerable number of bugs in Mozilla's CSS2 implementation, and a considerable 
number of bugs in the test suite, but nothing seemed to indicate a problem with 
nsStyleSet. IMO, the regression tests are more suited for this sort of thing.

I applied the Modern chrome and got a large window with menus and an 
unresponsive blue-gray gradient background. No toolbars, no viewport. Corners 
were missing off buttons in the dialogs, and several other things were off. I 
don't know whether that's a result of my changes or just problems with my 
particular build (wouldn't surprise me; I have a rather absurd setup for 
acquiring and compiling Mozilla).
Classic works fine.

> Make me a win32 test build

I can't build on Windows.

> time frame for this?

This bug's target is Mozilla1.0. Bug 45852's target is 0.9.3. Bug 45850 doesn't 
have a target milestone.

Comment 50

[Marc Attinasi]

Doesn't sound good. I guess I should apply the patch myself and see what I see...

Comment 51

[Marc Attinasi]

I have applied this patch on my Mac and have been running with it - no apparent
problems. I am running the browser and mail and composer (modern skin). I have
not performed the tests in the extended testcase yet, but I wanted to post this
now since fantasai reported some fundamental problems after his patch was
applied - I'm not seeing the same problems in the Modern skin.

Comment 52

[Pierre Saslawsky]

I don't like having UA-important rules overriding User-important rules.  In the 
current code, ua.css links html.css, quirk.css and xul.css.  The first 2 are 
provided as text files with the browser but xul.css is stored as a resource.  It 
means that users cannot override important rules from xul.css.  In my opinion, 
users should always have a way to prevail.

Comment 53

[Pierre Saslawsky]

Apparently, Ian Hickson disagrees.  In bug 45852, on [2000-10-30 19:15], he 
wrote: "!important rules in ua.css should be un-overridable by author and user 
rules".  What's the rationale behind it?  I thought that for accessibility 
reasons we always had to leave the user free to change the interface.  If Ian 
meant that the user can always change ua.css then the problem is that xul.css is 
stored in a resource, not that it is higher in the cascade.

Comment 54

[Braden]

Why do we have UA-important rules, anyway?

Comment 55

[Marc Attinasi]

UA important rules are strictly for internal implementation-specific needs. We
only make rule !important in the UA stylesheets when those rules are required by
our implementation, and overriding them would break something and must therefore
be prevented. For example, some of the elements (form elements come to mind)
cannot have their display property changed - they will stop working if they do.
So, we make the rules !important.

CSS2 does not specify UA !important semantics - so, I consider it the equiv. of
putting the rule in C++ code. In other words, it should be immutable.

Comment 56

[Braden]

Sounds reasonable to me. It is important, though, that this rationale be
well-publicized, to avoid spurious checkins of !important rules in UA stylesheets.

Comment 57

[Marc Attinasi]

Yes, I agree. Also, there is some risk I think. If CSS defines UA-important
rules in the future, and they define the cascade differently, then we will
probably have to change...

Comment 58

[Braden]

I think it's very unlikely that a future CSS spec would do that. The UA
stylesheet seems to be little more than a conceptual construct to enforce the
idea that the underlying implementation should itself act like CSS in order to
ensure behavioral consistency in the cascade. There is no requirement that the
implementation actually use CSS syntax for its resource files; Mozilla's use of
such is just a convenience of its architecture.

That said, we have folks equipped to take the temperature of the CSS WG on
matters like this, and their opinions would be more informative than mine.

Comment 59

[Pierre Saslawsky]

I disagree, the US stylesheet is not just a conceptual construct.  Even though 
the sample stylesheet (http://www.w3.org/TR/REC-CSS2/sample.html) is 
"informative, not normative", "developers are encouraged to use it as a default 
style sheet in their implementations".  I would like to see it evolve toward a 
recommendation and if this happens, I think it will be a requirement that User-
important rules override UA-important rules.

Comment 60

[David Baron :dbaron:]

So you'd prefer that we write additional code in C++ when we need to ignore
properties?  We could just as well add a !-moz-ua-override that would do the
same thing, except that !important has no defined meaning in UA stylesheets so
we may as well use it.

Comment 61

[Braden]

I don't think that statement in the appendix conflicts with what I stated; my
impression is that the author's objective with that suggestion is to encourage
consistency in the default display of documents. (Surely you would agree that
the stylesheet in Appendix A is unsuitable for actual use as a UA default
stylesheet on several counts.)

Note 6.4:

"Conforming user agents must apply a default style sheet (or behave as if they
did) prior to all other style sheets for a document."

So on this point, leeway has been explicitly included in the spec.

The basic notion of fixing certain elements' attributes immutably has some
conformance risks associated with it, but at least in the case of "display" the
CSS2 spec provides an exemption.

Clearly UA-important rules (as they are currently implemented) are a means of
last resort: a way of accommodating limitations of Mozilla or the medium itself.
In that context, it makes no sense to let users override them.

But suppose, for a moment, that UA-important rules *could* be overridden in the
user stylesheet. What would then be the use of a UA-important rule?

Comment 62

[Pierre Saslawsky]

To make it clear (for the third time): I would like important rules in user 
stylesheets to have the highest priority.  That's what the spec says 
(http://www.w3.org/TR/REC-CSS2/cascade.html#cascade) and, afaik, we never 
intended to change this behavior.  Users must prevail even if it gives them the 
opportunity to break things.  The parallel between UA-important rules and C++ 
code doesn't stand.  There is nothing we can carve in stone anyhow: as long as we 
give the users the opportunity to make changes, at any level of priority, they 
can break things - C++ code as well as CSS declarations.

If we make user-important rules the higest priority and users mess up with 
important declarations, they will break their own browser and I'm fine with it.  
If we make UA-important rules the highest priority and we mess up with it, we may 
prevent some unfortunate users from customizing something they need.

Comment 63

[Braden]

Pierre is right about the spec here. From 6.4.1, step 2:

"For '!important' declarations, user style sheets override author style sheets
which override the default style sheet."

Now, I happen to think that's silly and poorly considered (this appears to make
UA-important rules Totally Useless), but there it is. David, are your or Hixie
interested in taking this up with the WG?

Comment 64

[Pierre Saslawsky]

In fact my fears come from 2 possibilities:
- misuse of the 'important' keyword in UA stylesheets
- declarations in xul.css of things that don't belong there

On your side, what you describe ("important rules == C++ code") might be better 
implemented by removing the important declarations from UA.css and moving them to 
a separate text file (resource.css?) that clearly cannot be overriden. This way 
we wouldn't even need to extend or interpret the current specification, but just 
follow the current rules (see Erik Van der Poel [2000-07-10 15:34]).

Comment 65

[Pierre Saslawsky]

The cascade would then be
1) Non-important rules:  author > user   > UA
2) Important rules:      user   > author > UA
3) Internal rules:       resource.css

In resource.css, I guess we would mostly have XBL bindings

Comment 66

[Marc Attinasi]

Section 6.4.2 of CSS2 states:

Both author and user style sheets may contain "!important" declarations, and
user  "!important" rules override author "!important" rules.

This sentance is what made me think that !important in the UA stylesheet
(default stylesheet) was unspecified. That said, the sentance pointed out in
section 6.4.1 does seem to conflict...

Comment 67

[Hixie (not reading bugmail)]

I posted about this to the working group just now. I'll report back when we have
some sort of concensus, but when I last brought this issue up (either Oslo or
San Fransisco, Pierre was there if I recall correctly) there was tentative 
agreement to change the spec so that UA !important rules should override all
other rules. 

W3C members: http://lists.w3.org/Archives/Member/w3c-css-wg/2001JulSep/0203.html

Comment 68

[Marc Attinasi]

... actually, I think 6.4.2 step 2, when read in the context of step 1, is not
really saying anything about !important declarations in the UA. I think it is
saying that _everything_ overrides the rules in the default style sheet (note
that it refers the !important declarations in the user and document sheets, but
only refers to the 'default style sheet', exactly mimicing the prose used in
step 1).

Anyway, clarrification from the WG would be good here, but I want to address
Pierre's desire to have the user's !important declarations override everything.
In general, I think that this is the right idea, to allow accessability concerns
to be addressed via a very potent user stylesheet. However, in the context of
the UA stylesheets, including XUL, !important needs to be immutable to prevent
users from accidentally breaking the UI (or even basic HTML) because of the
internals of our implementation. It would be far better if we made !important in
the UA stylesheet immutable so that they could put !important rules in their
user stylesheet and not worry about it breaking the basic rendering. If we let
!important in the user stylesheet beat out !important in the UA stylesheet then
the user may accidentally make Mozilla unusable, whereas that same user
stylesheet would work fine in another browser where there is no default
stylesheet (like Mac IE). So, using !important or !moz-immutable, I think we
need a way to protect ourselves and, by extension, the user, from what the user
stylesheet may unintentinonally cause.

Comment 69

[Hixie (not reading bugmail)]

agreed. This was the basis for my proposal to the UA.

Comment 70

[Hixie (not reading bugmail)]

er. WG.

Comment 71

[Pierre Saslawsky]

Two points in Mozilla make this issue trickier than in other apps:

1) Our UI is affected by the UA and User stylesheets, and because of this we have 
a need for declarations that cannot be overridden by the content.  It is a 
problem of separation between application and content but the solution you are 
proposing is to limit the flexibility offered to the user.  To work around that 
problem we started to do bad things like loading User stylesheets as Backstop 
sheets, loading some Chrome sheets (namely the Editor sheets - another tricky 
thing) as Override sheets, and now trying to make UA-important rules immutable 
(even lobbying the WG to make the specs fit better our architecture).

2) We don't use native widgets, and because of this we must give the user the 
possibility to adapt the UI, both content and chrome.  I might not be entirely 
correct here: I never played with accessibility tools but I think that the way 
they work is by overriding the native widget set.  It doesn't apply to Mozilla so 
we have to let the users plug whatever they want into the chrome.

These 2 points are contradictory (we wouldn't be arguing otherwise).  The more 
open is the app, the more fragile it becomes. The bottom line is we have two 
constraints:
a) The document should never be able to break the chrome.
b) The user should be able to change (almost) everything but without breaking the 
chrome by inadvertance (eg. as Marc mentioned, by transfering a user stylesheet 
from another browser).

Point (a) was solved a while ago but it was at the price of a non-respect of the 
standards as described above.

We started to solve point (b) by supporting two User stylesheets: userChrome.css 
and userContent.css.  Maybe we need to go further in this idea and support two 
sets of UA stylesheets: one for the chrome, one for the content.  The chrome 
would be rendered using a set of 2 stylesheets (User and UA) while the content 
would have a perfectly standard-compliant set of 3 stylesheets (User, Author and 
UA).  I don't know whether it would really work.  Did I make a fool of myself 
again?  If it works, as a side-effect, we might be able to strip down the UA 
stylesheet used by the chrome because apparently we don't make much use of 
html.css there (and even less quirks.css).  Reducing the number of rules could 
speed up the chrome a bit.

Comment 72

[Braden]

"... and now trying to make UA-important rules immutable 
(even lobbying the WG to make the specs fit better our architecture)."

Pierre, I posed the question earlier and so far no one has offered an answer:

"But suppose, for a moment, that UA-important rules *could* be overridden in the
user stylesheet. What would then be the use of a UA-important rule?"

I really think this is a design flaw in the spec, irrespective of Mozilla's
architecture. (Of course, I also think it's a design flaw that !important is
available in author style sheets.)

Comment 73

[Hixie (not reading bugmail)]

Braden: agreed.

Pierre: We should not have any !important rules in ua.css and related files
except for things that MUST NOT EVER be overriden, i.e. things that are only
in ua.css because it is easier for them to be there than in C++ code. Ergo, your
argument is null: for any bug where we don't allow the userChrome.css file to
override the ua.css file, we just have to remove the !important. In fact, a user
can do this even without us releasing a new version if they need it that badly.

Comment 74

[Pierre Saslawsky]

I get your point but what I am saying is that if we have something that we prefer 
to define in CSS rather than C++ but which is so crucial to a correct execution 
of the application that it should never be overridden, then IMO it belongs to a 
separate Mozilla-only stylesheet, preferably a separate style set.  My reason for 
thinking this is that these declarations are very likely to be internal to 
Mozilla and I regret that we "pollute" the standard stylesheets (UA/Author/User) 
with things that don't belong there.  Marc mentioned the question of 
interoperability of user stylesheets across browsers: the more we store vital 
declarations in there, the more difficult it will be for users to import a 
generic stylesheet and still get what they need.

What I'm proposing is a better separation than the presence or not of the 
!important keyword between what we consider user-modifiable declarations and 
mozilla internals. And if this results into a cleaner code and a better 
performance because of simpler stylesheets for both the chrome and the content, 
it might be worth looking into it.

What's the point of keeping a unique UA stylesheet anyhow?  The chrome hardly 
uses it anymore.  Remove html.css and launch the app: the chrome is displayed 
almost correctly.

Comment 75

[Hixie (not reading bugmail)]

> if we have something that we prefer to define in CSS rather than C++ but 
> which is so crucial to a correct execution of the application that it should 
> never be overridden, then IMO it belongs to a separate Mozilla-only 
> stylesheet, preferably a separate style set. 

In CSS terms, this is called a "UA Stylesheet". Ours is called "ua.css" and it
links to at least five other stylesheets: html.css, xul.css, mathml.css, 
quirk.css, and viewsource.css.

I have nothing against adding another one to that list, say "vital.css", but 
that is really a separate issue.


> My reason for thinking this is that these declarations are very likely to be 
> internal to Mozilla and I regret that we "pollute" the standard stylesheets 
> (UA/Author/User) with things that don't belong there.

This is *exactly* what the UA stylesheet concept is for.


> Marc mentioned the question of interoperability of user stylesheets across 
> browsers: the more we store vital declarations in there [...]

Nobody is suggesting changing the contents of user stylesheets. Only UA 
stylesheets. They are very distinct concepts.


> What's the point of keeping a unique UA stylesheet anyhow?  The chrome hardly 
> uses it anymore.  Remove html.css and launch the app: the chrome is displayed 
> almost correctly.

html.css is not the UA stylesheet, it is merely a small subset of the UA 
stylesheet in Mozilla. Remove "ua.css" and try again.

Comment 76

[Pierre Saslawsky]

> I have nothing against adding another one to that list, say "vital.css", but 
> that is really a separate issue.

And especially, that would not be a solution.

> This is *exactly* what the UA stylesheet concept is for.

When the spec was designed, nobody had imagined that someday some developers 
would be foolhardy enough to expose in the UA stylesheet some declarations that 
are fundamental to a correct execution of their program. And even if someone had 
imagined it, the spec would not have recommended anything on that matter because 
the focus of the spec is simply the content. The concept of UA stylesheet never 
implied "if you write an application with a CSS-based chrome, you should expose 
absolutely everything into the same UA stylesheet that authors and users can 
override to display the content".

> > Marc mentioned the question of interoperability of user stylesheets across 
> > browsers: the more we store vital declarations in there [...]
> 
> Nobody is suggesting changing the contents of user stylesheets. Only UA 
> stylesheets. They are very distinct concepts.

I slipped. Please read "...the more we store vital declarations in the UA 
stylesheets...". The paragraph becomes "Marc mentioned the question of 
interoperability of user stylesheets across browsers: the more we store vital 
declarations in the UA stylesheets, the more difficult it will be for users to 
import a generic user stylesheet and still get what they need."

> html.css is not the UA stylesheet, it is merely a small subset of the UA 
> stylesheet in Mozilla. Remove "ua.css" and try again.

About removing html.css, my point is that the chrome uses xul.css and almost 
nothing else, so why not separate this stylesheet from the other UA sheets if it 
makes things simpler and the code more efficient?

About the rest, paradoxically you strenghten my position that the UA stylesheet 
is growing out of control and it becomes more difficult for users to override 
safely, and for us to work with.  Let's take for instance the Editor (simple 
problem, as always).  When listing the UA stylesheets, you did not mention the 
ones used by the Editor.  As of last year, I counted four: EditorOverride.css, 
EditorContent.css, EditorAllTags.css and EditorParagraphMarks.css, maybe there 
are more now.  All these stylesheets are loaded as override sheets because they 
need to have the highest priority in our currently broken cascade. How are we 
going to deal with this when we fix the cascade? Users may want to change the 
content but not break the Editor, so should we load these stylesheets as Backstop 
sheets like the rest of the UA stylesheets and then declare all the Editor rules 
as !important rules? It seems unavoidable because we certainly don't want the 
content to override the Editor rules. But then what if the Editor is relying on 
some of these rules to be !important in some stylesheets and not !important in 
other stylesheets?

No doubt we'll make it work once more, but I think it's now worth looking at ways 
to better insulate our internals (chrome, editor included) from the content... 
with a separate set of stylesheets for the chrome for instance.

Comment 77

[fantasai]

The concept of the UA stylesheet is merely that of a collection of style rules
the UA ascribes to a document, be they written in CSS, C++, DSSSL, Fortran or
any other language. The CSS2 model describes all the UA rules that are set as
defaults and can be overridden by document authors and users. It does not
describe those UA rules which override everything else, and that is what Ian's
definition of UA !important rules describes. "Internal Resource" basically
renames the concept. The only thing you're complaining about is the use of
!important syntax.

> 2) We don't use native widgets,
> When the spec was designed... correct execution of their program.

This is irrelevant.

>The chrome would be rendered using a set of 2 stylesheets (User and UA)

IIRC, the chrome is an XML document. The style system should treat it as such.

bug 62838> Marking dependency.
> But then what if the Editor is relying on some of these rules to be !important
> in some stylesheets and not !important in other stylesheets?

The cascade within the UA level need not conform to CSS rules as long as it
behaves like a CSS cascading level in its interaction with author and user style 
rules. IMO, an implementation that requires two or three (conceptual) sub-levels 
within the UA level is not in conflict with the CSS model.

> the chrome uses xul.css and almost nothing else, so why not separate this 
> stylesheet from the other UA sheets...
> with a separate set of stylesheets for the chrome for instance.

This bug is on cascading order, not style set scope. You should file a separate
bug.

BTW, you forgot to answer Braden's question.

Comment 78

[Hixie (not reading bugmail)]

> Marc mentioned the question of interoperability of user stylesheets across 
> browsers: the more we store vital declarations in the UA stylesheets, the 
> more difficult it will be for users to import a generic user stylesheet and 
> still get what they need.

As far as user stylesheets are concerned there is no difference between storing 
unoverridable style rules in the UA stylesheet using !important or storing them
in the C++ code or in a separate level of the CSS cascade.

This is therefore not an argument against using !important to make our life 
simpler.


> About removing html.css, my point is that the chrome uses xul.css and almost 
> nothing else, so why not separate this stylesheet from the other UA sheets if 
> it makes things simpler and the code more efficient?

What has the chrome got to do with this? I'm referring to the content area here.
The content area uses xul.css (to render XUL documents) as well as mathml.css 
(to render MathML documents) and html.css (to render HTML and XLink documents)
and quirk.css (to render HTML documents in a backwards-compatible manner).

This has nothing to do with using !important or not as far as I can tell. The
html.css UA stylesheet would benefit from using an unoverridable !important for
things like :table and :viewport, and quirk.css would benefit from it for some
of its quirks.

Comment 79

[David Hyatt]

xul.css is not a chrome stylesheet.  It is simply a stylesheet that defines only
the most basic and minimal rules to make XUL widgets function.  It does not
attempt to define appearance properties.  The skins do that. 

The chrome registry does bring in a second agent sheet that defines the
appearance of scrollbars.  I view this stylesheet as evolving into a full-blown
forms stylesheet which defines the appearance of form controls + scrollbars
according to the user's current skin.  For XUL <browser> widgets, you can also
specify an entire set of agent sheets that you would like to be used for your
content area.  Thus chrome is completely configurable with regards to the agent
sheets being loaded in its content areas.

For XUL skins we use normal author sheets (and scoped author sheets from XBL). 
Basically nearly all of the chrome rules are specified in author sheets, with
only stuff that is also needed for the content area specified in agent sheets.
Are you mostly concerned with agent sheets here, or would chrome author sheets
be affected by this bug?

Comment 80

[Pierre Saslawsky]

To answer Braden's question (2001-07-23 22:41): "But suppose, for a moment, that 
UA-important rules *could* be overridden in the user stylesheet. What would then 
be the use of a UA-important rule?"...

The use of UA-important rules would be to override any normal rules. As I wrote 
before, the cascade should be
  1) Non-important rules:  author > user   > UA
  2) Important rules:      user   > author > UA
It is also what Braden and Marc agreed on after reading 6.4.1: "For '!important' 
declarations, user style sheets override author style sheets which override the 
default style sheet."

Instead, the proposed implementation makes one of the levels a bit pointless. If 
we have the following cascade, there is no difference between author and author-
!important:
  1) Non-important rules:  author > user   > UA
  2) Important rules:      UA     > user   > author

I think it is closer to the spirit of the CSS Founding Fathers to let the users 
have the last word with !important rules.  But I also agree that:
1) this may be a question of interpretation,
2) the notion of "last word" should be understood as "last word before C++ and 
other things that should never be changed".

These "things that should never be changed" don't have their place, IMO, in the 
cascade as currently defined by the spec.  They only exist because of Mozilla's 
architecture and they should be stored/processed somewhere else - I suggested at 
a higher level in the cascade.  Ideally, we would have these 3 features:
- No change whatsoever in the UA stylesheet should affect the chrome, even less 
cause a breakage of the application.
- Everything in the UA stylesheet should be in text form (nothing in zip 
archives).
- Everything in the UA stylesheet should be overridable by the user stylesheet.

Independently from these considerations,  we will also probably want to let the 
users change the chrome to fit their needs but, IMO again, this should be a 
clearly separate problem (ie. some files can be in zip archives, things can break 
etc...).  It is the conclusion that we reached last year when hyatt implemented 
the 2 user stylesheets: userChrome and userContent.

---

To answer hyatt's question: I am mostly concerned with agent sheets, specifically 
their maintenability (by us) and overridability (by users).  You are in a better 
position to estimate how chrome author sheets would be affected by a change of 
priority in the cascade with the current proposal, or by overrides from the user 
stylesheet if we decide to comply with 6.4.1.  And then remains the question of 
the override sheets in the Editor...

---

Hixie and Glazman will be at the WG meeting next week.  If you have some thoughts 
or recommendations, I'm sure they will report them in all fairness.

Comment 81

[Hixie (not reading bugmail)]

For the record: At CSSWG meetings I represent myself and only myself. Not
Mozilla, not Netscape, and not any other contributors to this bug. Since I very
firmly believe that UA stylesheet "!important" rules should be non-overridable,
that is the opinion I shall be arguing and putting forward should this issue 
come up next week.

Comment 82

[Pierre Saslawsky]

Ian: you indeed got a seat on the condition that you would be representing only 
yourself.  When I invited other people to share their opinions, it was merely a 
suggestion for you to explain during the meeting "I firmly believe that... but 
other people objected that...". The multiplication of ideas often result in 
better solutions.

The debate is not just the clarification of 6.4.2 versus 6.4.1 step 2.  My main 
concern is that the UA stylesheet becomes relatively complex for users to 
understand (with some parts in zip archives) and to override (with !important 
rules here and there, plus maybe some potential for breaking the chrome).

In the end, if 6.4.2 prevails next week we'll check in the patch as-is and the 
issues I raised may become the subject of an enhancement.  If we go with 6.4.1 
step 2, it will force us to solve the issues sooner, or at least find a temporary 
solution based on small changes to the proposed patch. Either way we all win to 
have a debate on that topic.

Anyhow, thanks to fantasai for his patience (it will be more than 2 months 
between the submission of the patch and, hopefully!, a clarification from the 
WG).

Comment 83

[fantasai]

> The use of UA-important rules would be to override any normal rules.

What is the point of overriding normal author rules if you can't override
!important ones? If something's important enough to override normal author
rules, then it's probably important enough to override everything else as well.
Certainly it should be more important than anything else the author writes--
!important rules included.

> My main concern is that the UA stylesheet becomes relatively complex for users
> to understand and to override.

As I said before, this is a separate issue. It has little to do with the
cascade and everything to do with how you organize and handle the rules in the
UA stylesheet files.


*fantasai ponders the shortcomings of English pronouns... *

Comment 84

[Pierre Saslawsky]

> then it's probably important enough to override everything else as well

If something is *that* important, it should be moved somewhere else.

> It has little to do with the cascade

The 2 issues are of course related: if we had not interpreted the spec as we did, 
we would have had to find a better solution much sooner.

Comment 85

[Pierre Saslawsky]

...the issues are related but, I agree, they can be dealt with separately.  
That's what I wrote in my previous comments.

Comment 86

[fantasai]

Either you need to override all the author rules, or you don't need to override 
them at all. Give me one example where it is useful for the UA to override 
normal author rules but not !important ones.

Comment 87

[Pierre Saslawsky]

(CCd Daniel in case it is debated at the f2f)

Your question would be better addressed to the persons who originally designed 
the spec, and their response would probably be as valid as our justification as 
of why in the current proposal (2001-04-30 14:56) Author-normal and Author-
!important are on two consecutive levels of priorities, or why Override-normal is 
above Author-!important (which seems to be in contradiction with the spec).

Maybe they overlooked the description of UA-!important and did not make 6.4.2 as 
clear as 6.4.1 step 2 because, like me, they were less disturbed by the existence 
of a relatively useless level of priority as a by-product of a square design, 
than by the abuse of the alternative.  Maybe, like me, they thought that 
everything in the UA stylesheet should be overridable by the user, and if the UA 
had something important it should not simply not put it there.  

I don't know the details and I would have certainly prefered less ambiguity in 
the specification.  But the thing that is very clear to me is that the whole idea 
was to give Authors a certain level of control over the browser, and Users an 
even greater level of control over the pages.  It was designed to give 
flexibility to authors and users, not as a convenient way for the programmers to 
store their internal mechanisms.

I think that the issues brought by the evolution of our UA stylesheet in the past 
12 months or so deserve to be mentioned to the WG when they debate over the 
clarification of the two articles above.  Let's present the thing in all 
fairness.  They are wise people.  We'll see.

Comment 88

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

I am going to raise this issue, if it is possible, during our next week's face
to face meeting.
My personal opinion : I clearly remember the discussions we had for that section
of the CSS 2 spec, quite a long time ago. The absolute precedence of the
important User stylesheet rules is a major characteristic of CSS. It is
absolutely necessary
that users can override everything in a user agent, including its UA stylesheet.
I remind the readers of this comment that W3C works hand in hand with governmental
agencies about accessibility. I am quite sure that the Keepers of the Temple,
Bert Bos and Håkon Lie (inventors of CSS) on one hand, the W3C staff on the other
will strictly refuse any modification or clarification of the spec trying to
give a User stylesheet a lower precedence than the UA stylesheet for !important
rules.

From my perspective, the UA stylesheet should not be a convenient dumping zone
for things that are not in the CSS specification. It seems to me that all the
problem described in this bug is a problem of scoping, not a problem of
cascade or interpretation of what the spec says about the cascade. My 0.02euros
only.

Comment 89

[Hixie (not reading bugmail)]

Pierre, Daniel: Do you believe that there exists rules that must not be 
overriden by user stylesheets, for example our rules for the ":viewport" or
":table" psuedo-elements, or do you believe the user should be able to override
everything, including things that will cause crashes and non-standard behaviour?

Comment 90

[Marc Attinasi]

If everything in html.css, forms.css, and quirk.css is to be overridable, then
we will have to _seriously_ analyze which rules have to be moved to code (where
they cannot be overriden by the user). This analysis will take some time, and we
probably need to do it anyway since our current use of !important in the ua
stylesheets has been totally reactionary (eg. we only put those things in as a
response to bugs, afaik). We definitely have some rules that cannot be
overridden given the current state of our rendering engine: there are already 6
!important rules in the ua stylesheets, and probably many more that need to be
invariant and are just bugs waiting to be found and reported.

Comment 91

[Pierre Saslawsky]

The role of the UA stylesheet is to expose things that users and authors can 
freely override, and what I said is that if a declaration is so vital that 
overriding it can cause a crash or affect the chrome, then this declaration 
should be moved somewhere else - not necessarily in code.  I don't deny the 
"!importance" of these declarations, I would like it to be conferred on them 
through another mechanism.

Comment 92

[Hixie (not reading bugmail)]

Marc: agreed. And I posit that "moving to code" is a most decidely poorer 
solution that leaving them in a stylesheet, marked specially. CSS is easier to
debug, easier to change, and easier to understand, plus it doesn't run the risk
of memory leaks and so on.

Pierre: You say that "the role of the UA stylesheet is to expose things that 
users and authors can freely override" -- where did you read this? I don't
believe this interpretation at all.

And where would you suggest moving these important rules, if you think that the
UA stylesheet is the wrong place and you agree that the code is a poor place?

Comment 93

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

I believe that all rules must be overridable by user. If we want one rule to
be applied whatever the user does, let's put it in another scope.

Comment 94

[Myk Melez [:myk] [@mykmelez]]

Fixing database corruption caused by bug 95743.  Pay no attention to the man
behind the curtain.

Comment 95

[Pierre Saslawsky]

*** Bug 45852 has been marked as a duplicate of this bug. ***

Comment 96

[Hixie (not reading bugmail)]

Pierre, Daniel: Ok, make a proposal for what your "other scope" could be. Note
that a requirement is that it not take more disk, code, memory or time footprint
than the proposed ua!important solution.

Please also explain why this scope is better than a file linked from in ua.css
with rules marked as !important and suitably commented as being non-overridable,
possibly with reasons why.

Comment 97

[Daniel Glazman (:glazou) (NOT MONITORING BMO ANY MORE)]

"not take more disk, code, memory or time footprint"... That's all ? This
request is, from my personal perspective, partly irrelevant. I can agree on
some portions of it, but never ask me to do less code if it makes us move from
an ugly proprietary behavior to something more acceptable, more reliable,
more maintainable.

I do think that the modification of the !important precedence rules must not be
done.

Comment 98

[Pierre Saslawsky]

Ian: we are all striving to make the product better by every aspects and if some 
compromises are necessary, rest assured that they will be (as usual :-) the 
result of a collegial decision.

Maybe I did not make two points clear enough:
1) I understand that in any application that supports CSS stylesheets, some 
things cannot be overridden because they are defined in C++.
2) I also understand that some of the CSS declarations we have in Mozilla -
because of our architecture- are so critical to a correct execution of the 
application that they should be considered as immutable as C++ code.

The one thing I really want to avoid is an abuse of the current interpretation of 
UA!important.  See for instance the proposed patch to bug 58755:
  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=46444
Text colors and background colors and declared UA!important. In the current tree, 
some background images are !important already. It breaks my heart to see things 
like this.

Then there is the problem of legibility of the current UA.css, a file that is 
supposed to be read, understood and (in my interpretation) entirely overridable 
by the user.  I don't think it is a coincidence if the rules that are currently 
marked "magic" (or seem magic) and declared !important (or that will sooner or 
later become !important because they cannot be overridden but we haven't 
identified them as such yet) are also the ones that users need the less to know 
about.  We should not expose the most arcane rules in UA.css anymore than we 
would copy in the file big comments containing critical parts of the C++ code, 
just to let the users know how cool it is the way it works inside Mozilla.  The 
message we currently give to users is: "Here is the whole thing, now please only 
override what you understand and don't touch the rest even though, just in case, 
we already put some of it out of reach (it's easy to tell, it's marked 
!important) and some other things, you can override but you cannot see them 
because they are in jar files."  This is an excellent message for programmers, I 
wish all aplications were as open as Mozilla, but I'm afraid we are somewhat 
missing the point for everybody else.

Overall what I'm saying is that the interpretation and especially the use we 
currently make of UA!important are not a good response to the problems above, and 
what I proposed earlier is to move these declarations to a resource stylesheet 
that would have the highest precedence in the cascade.

In the meanwhile the Working Group decided to not decide, which means that we are 
left on our own to get things moving <grin!>. Now about compromises and collegial 
decisions, I wouldn't be too disturbed if we kept until "LATER" the current 
interpretation of UA!important provided we banish its use everywhere except from 
within a particular linked stylesheet ("resource.css") that could also contain 
the most arcane moz declarations. During the cleanup, it would be nice to make 
sure that the !important declarations deserve to be, and that all the moz 
declarations really can be overridden.  We could also have two separate sheets: 
"resource.css" and "html-moz.css".

Isn't it a nice compromise?
And (hum...) would you volunteer?

Comment 99

[Hixie (not reading bugmail)]

Daniel: It's all very well saying you don't like my solution, but unless you 
have an alternative proposal, it's not a very tenable solution...

Pierre: So if I understand your proposed compromise correctly, you are saying 
that you support the use of UA!important to mean "immutable", provided that we
move all such rules into one UA stylesheet, containing very strong comments, and
put similar comments in the other UA stylesheets explaining the danger of having
!important at that level of the cascade.

I fully support this compromise, and volunteer to do the necessary cleanup work.
(As soon as I have a tree, in about 2 or 3 weeks.) If we agree to go down this
route, a bug should be filed regarding this work (or we can reuse 56362). This
work can be done either before or after the code to make UA!important immutable
is checked in.

Comment 100

[Pierre Saslawsky]

Correct: the WG did not take a position, we can split the issues the way we want. 
The important point is to make it easy for users to override what they need.  

First, it means that the attached patch can go in after reviews.  I'll check for 
instance that it does not implement the cascade you proposed on [2001-04-30 
14:56] where override-normal has a higher precedence than !important rules from 
other stylesheets.

Second, the problem is wider than just moving 6 UA!important declarations to an 
imported stylesheet.  LXR reports approximately 400 instances of "!important" in 
50 files or so:
    http://lxr.mozilla.org/seamonkey/search?string=%21important
Most of these instances are from the chrome but the problem is the same.  We have 
to check which sheets are loaded as UA sheets and which ones are Override or 
Author sheets.

The things to do would be:
- Find all the sheets loaded as UA sheets and make sure they really need to be UA 
instead of Override or Author.
- Make sure the UA!important declarations need to be "!important", then move them 
to "resource.css" .
- General cleanup, like separate the Mozilla-specific declarations (put them at 
the bottom of the file, for instance, or in a separate sheet), put some comments 
about these declarations, write in comments the URLs on LXR of these UA sheets 
that are imported from jar archives.
- In the future, restrict the use of UA!important rules anywhere else than in 
"resource.css".  Having them all at the same place makes it easier to spot abuses 
and it leaves us some flexibility in case we need to change the way we store or 
handle these declarations.  It would be interesting to know if we need exceptions 
to that rule (otherwise maybe we can enforce it by code? :-)  Basically from a 
programmer standpoint, declaring a color as UA!important should be seen as bad as 
hard-coding it in C++.

Comment 101

[fantasai]

> Pierre: So if I understand...

I agree completely.

Pierre: Let's please adjourn the discussion on Mozilla's stylesheets'
organization to some other bug, as it doesn't belong here. You can mark a depend 
if you wish.

Comment 102

[Hixie (not reading bugmail)]

Pierre wrote:
> First, it means that the attached patch can go in after reviews.  I'll check 
> for instance that it does not implement the cascade you proposed on [2001-04-
> 30 14:56] where override-normal has a higher precedence than !important rules 
> from other stylesheets.

Er. Override rules ase _supposed_ to be more important than any other normal
rules, that's per the CSS OM spec.


> Second, the problem is wider than just moving 6 UA!important declarations to 
> an imported stylesheet.  LXR reports approximately 400 instances of "!
> important" in 50 files or so:
>    http://lxr.mozilla.org/seamonkey/search?string=%21important
> Most of these instances are from the chrome but the problem is the same.  

No it's not. They are author-level stylesheets. There is nothing wrong with 
author stylesheet having !important rules. Especially in chrome, where they have
very little to cascade with.


Most of your other points seem reasonable, although I agree with fantasai that
we should move them to other bugs. Could you open the bugs for the issues you
want resolved, since you are the one who knows what they are? Thanks!

Comment 103

[Pierre Saslawsky]

>Er. Override rules ase _supposed_ to be more important than any other normal
>rules, that's per the CSS OM spec.

The spec says:
  The override style sheet takes precedence over author style sheets. An
  "!important" declaration still takes precedence over a normal declaration.
  Override, author, and user style sheets all may contain "!important"
  declarations.
but in your proposal, you wrote that Override-normal should have a higher 
precendence that Author-!important and User-!important, which is not what the 
spec says.  An !important declaration (be it Author or User) should always have a 
higher precedence than a normal declaration (be it Override).


>--
>No it's not. They are author-level stylesheets. There is nothing wrong with 
>author stylesheet having !important rules. Especially in chrome, where they have
>very little to cascade with.

Not all of these. For instance, forms.css is loaded as Agent sheet (userContent 
and userChrome too, I just noticed).  I also know for sure that some of these 
sheets are loaded as Override sheets.  Maybe other ones have been inadvertently 
loaded as Agent sheet.


>--
>Could you open the bugs for the issues you want resolved, since you are the one
>who knows what they are? Thanks!

We had 2 kinds of issues:
- Precedence of UA!important (this bug): The WG did not decide on that one, which 
means that there is no "legal" reason not to continue what we have been doing.
- Overridability of UA.css in Mozilla (some new bug): I described the issues in 
my previous comments but you sound like these are non-issues.  Is it the case?

Comment 104

[Hixie (not reading bugmail)]

*** Bug 89832 has been marked as a duplicate of this bug. ***

Comment 105

[Hixie (not reading bugmail)]

> [spec]

Hmm. That's not how I read it, but I can understand your interpretation. (I read
it meaning that all override sheets were more important than author sheets, and
that within each level, the !important rules overrode the normal rules.)

So, are we all agreed that the following order is the one we should implement?:

   [least important]
    1. UA normal declarations
    2. User normal declarations
    3. (HTML mapped attributes)
    4. Author normal declarations
    5. (style attribute declarations)
    6. Override normal declarations
    7. Author !important declarations
    8. (style attribute !important declarations)
    9. Override !important declarations
   10. User !important declarations
   11. UA !important declarations
   [most important]


> For instance, forms.css is loaded as Agent sheet 

But forms.css is not a chrome stylesheet. It's a UA stylesheet. In fact, it used
to be part of html.css -- forms.css doesn't affect the chrome at all.


> (userContent and userChrome too, I just noticed).

That's exactly what this bug is trying to fix! :-)


> - Overridability of UA.css in Mozilla (some new bug): I described the issues 
> in my previous comments but you sound like [you think] these are non-issues.  
> Is it the case?

I personally have never seen a problem here.


fantasai: How much work would it be to change "backstop stylesheet" to "ua
stylesheet" in the code? If it's easy, then you might want to consider doing
that at the same time, in order to make the code easier to read. No biggie.

Comment 106

[Pierre Saslawsky]

Ian: what I meant is that in the list returned by LXR, the instances of 
!important are not all in chrome author sheets.  Some are in chrome, some in 
content.  Some are in override sheets, some in UA sheets. We need to sort it out.  
It will be the object of a separate bug as we said.

About the cascade, we are on an agreement.  Note that:
- After I check in my fix for bug 62150 and in strict mode only, the style 
attribute declarations will be part of the cascade with a weight of (1,0,0), 
which is equivalent to the weight of a selector containing a simple class.
- According to the current code, the HTML mapped attributes have a higher 
precedence that Author and style attribute declarations instead of being just 
above the UA declarations.  It needs to be checked and maybe reported.

Comment 107

[David Baron :dbaron:]

> - According to the current code, the HTML mapped attributes have a higher 
> precedence that Author and style attribute declarations instead of being just 
> above the UA declarations.  It needs to be checked and maybe reported.

Works fine for me: http://www.people.fas.harvard.edu/~dbaron/css/test/noncss1

Comment 108

[fantasai]

Ok, so I will update the patch and the testcase to put author !important above
override normal, and change "Backstop" to "UA" or "Agent". (Anyone have a
preference? I'm leaning towards the latter.)

>a simple class

a simple ID

Comment 109

[fantasai]

Attachment: Revised Testcase

Comment 110

[fantasai]

Attachment: patch v2

Comment 111

[fantasai]

re-requesting review

Comment 112

[Pierre Saslawsky]

Some preliminary notes...
- Why do we recursively add the parent's important rules in 
StyleSetImpl::AddImportantRules()?
- PresShell::CloneStyleSet() does not clone the user sheets.
- Open a bug to remove PREFS_USE_OVERRIDE and do what is described in the 
comments (text and bg colors in agent sheet - link colors in override sheet)

Comment 113

[fantasai]

- Why do we recursively add the parent's important rules in 
StyleSetImpl::AddImportantRules()?

We have to add them in forwards order; otherwise the cascade goes in reverse for 
!important rules. One can't walk down the tree, so the rule nodes need to be 
stacked as we go up and then read down from highest (first) to lowest (last).
I'm not sure if that answers your question. You really should ask Hyatt, though, 
since he wrote the function and can probably explain it better than any 
speculation of mine.

Comment 114

[fantasai]

Attachment: patch v3

Comment 115

[Pierre Saslawsky]

Comment on attachment 52420 [details] [diff] [review]
patch v3

r=pierre

Comment 116

[Pierre Saslawsky]

I opened 2 related bugs:
  bug 103594: Clean up colors prefs code in nsPresShell
  bug 103596: [CASCADE] !important declarations don't inherit correctly

Comment 117

[Pierre Saslawsky]

*** Bug 45850 has been marked as a duplicate of this bug. ***

Comment 118

[Pierre Saslawsky]

waterson/hyatt/attinasi: please s/r fantasai's patch (the first one to volunteer 
might want to notify the other folks when starting the review).

Comment 119

[fantasai]

On Mon, 08 Oct 2001 11:21:19 -0700, Marc Attinasi wrote:

The changes look good. I am curious about the testing done on this 
patch. Clearly there is a large potential for regressions since so much 
is changed. Particularly, what tests have you run, have others tested it 
too, and do you have a landing / damage-control strategy? Please post 
this information to the bug so we have a lasting document there, and 
I'll be glad to sr.

- marc

So, as of right now, I have to go dig through regression data...
Anyone have a special request for testing?

Comment 120

[David Hyatt]

I would like to review this patch.  I'll get you comments tomorrow.

Comment 121

[fantasai]

Attachment: Regression Test Results - Analysis

Comment 122

[David Hyatt]

Can you convert all of the following to nsCOMPtr? That will let you eliminate
the initializations to nsnull in the constructor as well as the releases that
happen in the destructor.

   nsISupportsArray* mOverrideSheets;  // most significant first
   nsISupportsArray* mDocSheets;       // " "
-  nsISupportsArray* mBackstopSheets;  // " "
+  nsISupportsArray* mUserSheets;      // " "
+  nsISupportsArray* mAgentSheets;  // " "
 
-  nsISupportsArray* mBackstopRuleProcessors;  // least significant first
-  nsISupportsArray* mDocRuleProcessors; // " "
-  nsISupportsArray* mOverrideRuleProcessors; // " "
+  nsISupportsArray* mAgentRuleProcessors;  // least significant first
+  nsISupportsArray* mUserRuleProcessors;      // " "
+  nsISupportsArray* mDocRuleProcessors;       // " "
+  nsISupportsArray* mOverrideRuleProcessors;  // " "

Comment 123

[fantasai]

> what tests have you run

I ran the Revised Testcase, which is, IMHO, a pretty comprehensive test of  
cascading--at least, the aspect of it that's dealt with in nsStyleSet.

I've also run the block regression tests. A large number of tests failed under 
verification, but I did not get report of style differences for most of them. 
The ones that failed the verification and also had style differences are noted 
in the attachment. If you want the raw output from the scripts, I can provide 
that for you.

Classic works fine, Modern's menus and dialogs seem ok--but I'm getting the 
blank content area problem again. Unfortunately, I forgot to test Modern before 
compiling the changes...

> have others tested it too

not that I know of -- although, IIRC, you tested a very similar patch

> do you have a landing / damage-control strategy?

I've not a clew as to what you expect from me here...

> Can you convert all of the following to nsCOMPtr?

Yes.

Comment 124

[David Hyatt]

The logic in FileRules seems sound to me.  You're even handling !important
correctly for scoped stylesheets (which introduce nested document cascade levels
for each scope).

Comment 125

[Marc Attinasi]

>> do you have a landing / damage-control strategy?

>I've not a clew as to what you expect from me here...

This landing will have a reasonably high potential for regressions. As such a
reasonable plan for monitoring the fallout and fixing the bugs or bakcing out is
all I'm expecting. Landing and then taking a 4 week vacation to some unconnected
island resort immediatly afterwards would suck (for the rest of us anyway).

Comment 126

[Marc Attinasi]

Comment on attachment 52420 [details] [diff] [review]
patch v3

sr=attinasi (if hyatt continues to be happy with it)

Comment 127

[fantasai]

Attachment: patch v4

Comment 128

[fantasai]

New patch using nsCOMPtr - only nsStyleSet.cpp's diff has changed. (I ran cvs 
diff for nsStyleSet.cpp and copied the new version into the v3 patch.)

Testing: compiled, ran Revised Testcase, opened up Prefs Dialog and changed a 
pref

> a reasonable plan for monitoring the fallout and fixing the bugs or backing
> out

Plan: Once this is checked in, I'll post a notice to n.p.m.layout about the
      changes. (The UA !important level needs to be explained in any case.)
      Anyone can CC me on bugs that seem even remotely related to this, and
      I'll look into it. Changes will be backed out at the discretion of you
      and your peers, as I have nothing to offer on this type of decision.
      I will try to fix any problems that result from this checkin, but other
      than that, there's not much I can do..

Does that suffice?

> Landing and then taking a 4 week vacation

      I'm not planning on any four-week vacations just now--the longest I'll be
      gone is about three days. ;) If you want, I can write out my schedule and
      post it somewhere.

Comment 129

[Marc Attinasi]

Would you be willing to submit to a polygraph test and possibly provide
fingerprints or DNA samples? ;)  Seriously, your plan sounds fine.

Comment 130

[Pierre Saslawsky]

Comment on attachment 53428 [details] [diff] [review]
patch v4

r=pierre (just comparing with the previous diffs - let me know if you would like a more in-depth review)

Comment 131

[Pierre Saslawsky]

Why did you mark the latest attachment obsolete?  Is it just a question of bit-
rot or did you see a problem?

Comment 132

[Pierre Saslawsky]

Patch v3 was r/sr and I reviewed the small changes between v3 and v4.  I thought 
it was ready for checkin.

Comment 133

[fantasai]

> Why did you mark the latest attachment obsolete?  Is it just a question of
> bitrot or did you see a problem?

Just bitrot. Hafta switch from nsIRuleNode to nsRuleNode.

Comment 134

[fantasai]

Attachment: patch v5

Comment 135

[fantasai]

Changes from v4 (apart from switching to nsRuleNode) - 
 - cleaned up some indentation
 - removed initialization for mStyleRuleSupplier
     It's an nsCOMPtr--IIRC, that initializes to null anyway
 - changed AddImportantRule's recursion somewhat: I removed the 'parent'
     variable and related check (which means it goes one call deeper for Agent
     rules).

So, bitrot was a good thing, in this case.

I'm hoping Pierre can check the fix in sometime this week, so

  re-requesting review (again ;)


I'm actually more worried about doing something stupid with all this 
pointer/COMPtr stuff than about reversing the cascade or anything logical like 
that. So please check over those?

Comment 136

[Pierre Saslawsky]

Don't worry, I'll review within the next 2 days and take care of any bit-rot 
before checking in.  You are already my Friend of the Tree this week :-P

attinasi/hyatt: s/r please.

Comment 137

[Pierre Saslawsky]

r=pierre.  hyatt, please s/r

fantasai: Did you try an override stylesheet with the Revised Testcase? I didn't.

Comment 138

[fantasai]

> Did you try an override stylesheet with the Revised Testcase?

Yes. What I didn't test was whether or not userChrome.css is loaded at the user 
level.

BTW, did you account for dbaron's last checkin to nsStyleSet.cpp? The changes in 
HasStateDependentStyle conflict.

http://bonsai.mozilla.org/cvsview2.cgi?root=/cvsroot&subdir=mozilla/content/base
/src&command=DIFF&file=nsStyleSet.cpp&rev1=3.104&rev2=3.105

Comment 139

[Pierre Saslawsky]

The patch in nsDocumentViewer.cpp shows that userChrome and userContent are 
correctly loaded as user sheets (see also nsChromeRegistry::GetUserSheets())
    -          // XXX For now, append as backstop until we figure out something
    -          // better to do.
    -          (*aStyleSet)->AppendBackstopStyleSheet(sheet);
    +          (*aStyleSet)->AppendUserStyleSheet(sheet);

I'll fix the conflicts before checking in.

Comment 140

[David Hyatt]

sr=hyatt

Comment 141

[Pierre Saslawsky]

The patch was checked in after fixing 2 merge problems in 
ResolveStyleForNonElement and HasStateDependentStyle.  Thanks fantasai.

Comment 142

[Chris Waterson]

It looks like these may have caused a 20 to 30ms slowdown on the page-loader on
btek. We're pretty close to the resolution of that test, but...

Comment 143

[Pierre Saslawsky]

The follow-up on bloat/perf problems is at bug 108660.  Hyatt wrote "dbaron is on 
this.  Basically we waste time constructing extra SelectorMatchesData even when 
there are no rules in a particular level of the cascade."

Comment 144

[Boris Zbarsky [:bzbarsky]]

*** Bug 106404 has been marked as a duplicate of this bug. ***

Comment 145

[Madhur Bhatia]

verified