Closed
Bug 43220
Opened 24 years ago
Closed 22 years ago
[CASCADE]author !important rules override user !important rules in user.css
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla0.9.6
People
(Reporter: fantasai.bugs, Assigned: pierre)
References
(Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [need info][nsbeta3-])
Attachments
(10 files, 1 obsolete file)
48 bytes,
text/css
|
Details | |
630 bytes,
text/html
|
Details | |
18.74 KB,
patch
|
Details | Diff | Splinter Review | |
7.08 KB,
text/html
|
Details | |
2.55 KB,
text/plain
|
Details | |
7.14 KB,
text/html
|
Details | |
41.80 KB,
patch
|
Details | Diff | Splinter Review | |
42.07 KB,
patch
|
pierre
:
review+
attinasi
:
superreview+
|
Details | Diff | Splinter Review |
8.55 KB,
text/plain
|
Details | |
47.37 KB,
patch
|
pierre
:
review+
|
Details | Diff | Splinter Review |
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 3•24 years ago
|
||
confirming using 2000062008 nightly build
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows NT → Windows 2000
Comment 4•24 years ago
|
||
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.)
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•24 years ago
|
||
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?
Keywords: css1
Comment 7•24 years ago
|
||
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•24 years ago
|
||
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
Assignee: pierre → hyatt
Comment 9•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 11•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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.
Reporter | ||
Comment 14•24 years ago
|
||
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:3957E177.993BAC60@escape.com fantasai. "Re: user stylesheet". June 26, 2000. n.p.m.style
Updated•24 years ago
|
QA Contact: ckritzer → chrisd
Comment 15•24 years ago
|
||
need info: Marc, why did you reassign this to hyatt? Why do we need to do this in N6, and who should own it?
Whiteboard: [need info]
Comment 16•24 years ago
|
||
I'm the one that implemented user stylesheets. :)
Comment 17•24 years ago
|
||
The bulk of the work to fix this *may* still be at the style system end...
Comment 18•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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.).
Assignee | ||
Comment 21•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 23•24 years ago
|
||
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.
Updated•24 years ago
|
Whiteboard: [need info] → [need info][nsbeta3+]
Target Milestone: --- → M18
Comment 24•24 years ago
|
||
nsbeta3+. Dave will do his part,then hand off to Pierre
Comment 25•24 years ago
|
||
Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so the queries don't get all screwed up.
Keywords: nsbeta3
Reporter | ||
Comment 27•24 years ago
|
||
> 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
Updated•24 years ago
|
QA Contact: chrisd → py8ieh=bugzilla
Reporter | ||
Comment 28•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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!
Assignee: hyatt → pierre
Status: ASSIGNED → NEW
Updated•24 years ago
|
Summary: author !important rules override user !important rules in user.css → author !important rules override user !important rules in user.css [CASCADE]
Comment 31•24 years ago
|
||
Changing to nsbeta3- due to high risk.
Whiteboard: [need info][nsbeta3+] → [need info][nsbeta3-]
Assignee | ||
Comment 32•24 years ago
|
||
Block moved M18 bugs to mozilla0.9.1
Target Milestone: M18 → mozilla0.9.1
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Summary: author !important rules override user !important rules in user.css [CASCADE] → [CASCADE]author !important rules override user !important rules in user.css
Reporter | ||
Comment 33•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
Reporter | ||
Comment 38•23 years ago
|
||
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?
Reporter | ||
Comment 39•23 years ago
|
||
Reporter | ||
Comment 40•23 years ago
|
||
Reporter | ||
Comment 41•23 years ago
|
||
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•23 years ago
|
||
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?
Reporter | ||
Comment 43•23 years ago
|
||
>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•23 years ago
|
||
I thought David put the style data back in already??? I'll check that out. You have been running with the changes though, right?
Reporter | ||
Comment 45•23 years ago
|
||
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.
Reporter | ||
Comment 46•23 years ago
|
||
Comment 47•23 years ago
|
||
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•23 years ago
|
||
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?
Reporter | ||
Comment 49•23 years ago
|
||
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•23 years ago
|
||
Doesn't sound good. I guess I should apply the patch myself and see what I see...
Comment 51•23 years ago
|
||
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.
Assignee | ||
Comment 52•23 years ago
|
||
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.
Assignee | ||
Comment 53•23 years ago
|
||
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•23 years ago
|
||
Why do we have UA-important rules, anyway?
Comment 55•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 59•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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?
Assignee | ||
Comment 62•23 years ago
|
||
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•23 years ago
|
||
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?
Assignee | ||
Comment 64•23 years ago
|
||
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]).
Assignee | ||
Comment 65•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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
Whiteboard: [need info][nsbeta3-] → [need info][nsbeta3-] WG
Comment 68•23 years ago
|
||
... 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.
Whiteboard: [need info][nsbeta3-] WG → [need info][nsbeta3-]
Comment 69•23 years ago
|
||
agreed. This was the basis for my proposal to the UA.
Comment 70•23 years ago
|
||
er. WG.
Assignee | ||
Comment 71•23 years ago
|
||
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•23 years ago
|
||
"... 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•23 years ago
|
||
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.
Assignee | ||
Comment 74•23 years ago
|
||
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•23 years ago
|
||
> 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.
Assignee | ||
Comment 76•23 years ago
|
||
> 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.
Reporter | ||
Comment 77•23 years ago
|
||
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•23 years ago
|
||
> 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•23 years ago
|
||
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?
Assignee | ||
Comment 80•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 82•23 years ago
|
||
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).
Reporter | ||
Comment 83•23 years ago
|
||
> 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... *
Assignee | ||
Comment 84•23 years ago
|
||
> 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.
Assignee | ||
Comment 85•23 years ago
|
||
...the issues are related but, I agree, they can be dealt with separately. That's what I wrote in my previous comments.
Reporter | ||
Comment 86•23 years ago
|
||
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.
Assignee | ||
Comment 87•23 years ago
|
||
(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•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 91•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
assignee_accessible: 1 → 0
CC list accessible: false
qacontact_accessible: 1 → 0
Not accessible to reporter
Updated•23 years ago
|
assignee_accessible: 0 → 1
CC list accessible: true
qacontact_accessible: 0 → 1
Accessible to reporter
Comment 94•23 years ago
|
||
Fixing database corruption caused by bug 95743. Pay no attention to the man behind the curtain.
Assignee | ||
Comment 95•23 years ago
|
||
*** Bug 45852 has been marked as a duplicate of this bug. ***
Comment 96•23 years ago
|
||
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•23 years ago
|
||
"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.
Assignee | ||
Comment 98•23 years ago
|
||
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•23 years ago
|
||
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.
Blocks: 56362
Assignee | ||
Comment 100•23 years ago
|
||
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++.
Reporter | ||
Comment 101•23 years ago
|
||
> 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•23 years ago
|
||
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!
Assignee | ||
Comment 103•23 years ago
|
||
>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•23 years ago
|
||
*** Bug 89832 has been marked as a duplicate of this bug. ***
Comment 105•23 years ago
|
||
> [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.
Assignee | ||
Comment 106•23 years ago
|
||
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•23 years ago
|
||
> - 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
Reporter | ||
Comment 108•23 years ago
|
||
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
Reporter | ||
Comment 109•23 years ago
|
||
Reporter | ||
Comment 110•23 years ago
|
||
Reporter | ||
Comment 111•23 years ago
|
||
re-requesting review
Assignee | ||
Comment 112•23 years ago
|
||
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)
Reporter | ||
Comment 113•23 years ago
|
||
- 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.
Reporter | ||
Comment 114•23 years ago
|
||
Assignee | ||
Comment 115•23 years ago
|
||
Comment on attachment 52420 [details] [diff] [review] patch v3 r=pierre
Attachment #52420 -
Flags: review+
Assignee | ||
Comment 116•23 years ago
|
||
I opened 2 related bugs: bug 103594: Clean up colors prefs code in nsPresShell bug 103596: [CASCADE] !important declarations don't inherit correctly
Assignee | ||
Comment 117•23 years ago
|
||
*** Bug 45850 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 118•23 years ago
|
||
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).
Reporter | ||
Comment 119•23 years ago
|
||
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•23 years ago
|
||
I would like to review this patch. I'll get you comments tomorrow.
Reporter | ||
Comment 121•23 years ago
|
||
Comment 122•23 years ago
|
||
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; // " "
Reporter | ||
Comment 123•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
>> 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•23 years ago
|
||
Comment on attachment 52420 [details] [diff] [review] patch v3 sr=attinasi (if hyatt continues to be happy with it)
Attachment #52420 -
Flags: superreview+
Reporter | ||
Comment 127•23 years ago
|
||
Attachment #53428 -
Attachment description: patch v3 → patch v4
Reporter | ||
Comment 128•23 years ago
|
||
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•23 years ago
|
||
Would you be willing to submit to a polygraph test and possibly provide fingerprints or DNA samples? ;) Seriously, your plan sounds fine.
Assignee | ||
Comment 130•23 years ago
|
||
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)
Attachment #53428 -
Flags: review+
Attachment #53428 -
Attachment is obsolete: true
Assignee | ||
Comment 131•22 years ago
|
||
Why did you mark the latest attachment obsolete? Is it just a question of bit- rot or did you see a problem?
Assignee | ||
Comment 132•22 years ago
|
||
Patch v3 was r/sr and I reviewed the small changes between v3 and v4. I thought it was ready for checkin.
Reporter | ||
Comment 133•22 years ago
|
||
> 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.
Reporter | ||
Comment 134•22 years ago
|
||
Reporter | ||
Comment 135•22 years ago
|
||
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?
Assignee | ||
Comment 136•22 years ago
|
||
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.
Assignee | ||
Comment 137•22 years ago
|
||
r=pierre. hyatt, please s/r fantasai: Did you try an override stylesheet with the Revised Testcase? I didn't.
Assignee | ||
Updated•22 years ago
|
Attachment #55602 -
Flags: review+
Reporter | ||
Comment 138•22 years ago
|
||
> 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
Assignee | ||
Comment 139•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Target Milestone: mozilla1.0 → mozilla0.9.6
Comment 140•22 years ago
|
||
sr=hyatt
Assignee | ||
Comment 141•22 years ago
|
||
The patch was checked in after fixing 2 merge problems in ResolveStyleForNonElement and HasStateDependentStyle. Thanks fantasai.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 142•22 years ago
|
||
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...
Assignee | ||
Comment 143•22 years ago
|
||
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•22 years ago
|
||
*** Bug 106404 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•