Closed Bug 112854 Opened 23 years ago Closed 22 years ago

Empty styles not included in cssRules[]

Categories

(Core :: DOM: CSS Object Model, defect)

Product:
Core
Component:
DOM: CSS Object Model
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.0

People

(Reporter: robert, Assigned: bzbarsky)

References

Details

(Whiteboard: [DIGBug][eapp])

Attachments

(1 file)

Patch. Tested this for a bit and everything seems to be working correctly...
674 bytes, patch
dbaron
: review+
attinasi
: superreview+
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.5) Gecko/20011011
BuildID:    2001101117

The cssRules collection will not does not contain CSS rules that don't have any
properties specified.  See the attached test case for an example of this - It
illustrates how the empty '.style2' rule is not included.

Reproducible: Always
Steps to Reproduce:
1. See attached test case
*sigh*  The attachment page in bugzilla isn't working for me, so here's the html
source of the test:
================ SNIP ===============
<html>
<head>
<style type="text/css">
<!--
.style1 {
	color:red;
}
.style2 {
}
.style3 {
	color:blue;
}
-->
</style>

</head>

<body bgcolor="#FFFFFF">
<div class=style1>
	Some text written in style 1 <span style="text-decoration:underline"
onclick="boldify('.style1')">(make this class bold!)<span>
</div>
<div class=style2>
	Some text written in style 2 <span style="text-decoration:underline"
onclick="boldify('.style2')">(make this class bold!)<span>
</div>
<div class=style3>
	Some text written in style 3 <span style="text-decoration:underline"
onclick="boldify('.style3')">(make this class bold!)<span>
</div>

<script>
// Do a quick pass of the style sheets to see what styles we find
for (var i=0; i < document.styleSheets.length; i++) {
	var myRules = document.styleSheets[i].cssRules;
	for (var j=0; j < myRules.length; j++) {
		var myRule = myRules[j];
		document.write('Found rule with selector, "' + myRules[j].selectorText + '"<br>');
	}
}
</script>

<script>
/**
 * Find the selector for a style and make it bold
 */
function boldify(aSelector) {
	var myStyle = findStyle(aSelector);
	if (myStyle) {
	} else {
		alert('Hey, can\'t find style for "' + aSelector + '"');
	}
	myStyle.fontWeight = 'bold';
}

/**
 * General method for finding a style by selector in a document
 */
function findStyle(aSelector, aDocument) {
	aDocument = aDocument ? aDocument : document;
	for (var i=0; i < aDocument.styleSheets.length; i++) {
		var myRules = aDocument.styleSheets[i].cssRules;
		for (var j=0; j < myRules.length; j++) {
			var myRule = myRules[j];
			if (myRule.selectorText == aSelector) return myRule.style;
		}
	}

	return null;
}
</script>

</body>
</html>
================ SNIP ===============
That's because we don't bother creating css rule objects for rules that will
have no effect....

Those rules are just no present in our internal stylesheet representation.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Unfortunately, this means I can't assign an element a CSS class and then assign
style properties to it dynamically. I *have* to assign stuff to it in the CSS
sheet, which I really don't want to do... especially if I want it to inherit all
of it's CSS styles by default.
You can workaround this by finding some CSS property that isn't used and setting
that in the style sheet, but what "isn't used" changes from one application to
another.  You end up having to figure this out each time based on what the style
is applied to and what properties you'll want to set.  Yuck!

Anyhow, that's my $.02 on this.
For performance, you're better off swapping classes in and out on the element
and leaving the rules in the stylesheet static.  Probably we could fix this if
we start to make our DOM CSS object more wrappers than direct access to our
internal structures, which we'll have to do when we restructure things.
I think you're overlooking the fact that the whole point of having a CSS rule is
that can apply it to more than 1 element.  Otherwise you just set the style for
the element itself.  Trying to find all the elements you need to change the
class name on seems much less efficient then modifying the stylesheet rule.

For example, imagine a form with optional and required fields where the user can
click a button to hide the optional fields.  The easiest way to do this would be
to assign optional fields the "optionalFieldStyle" and then set the 'display'
property to 'none' on this rule in the style sheet.

The real-world case that I'm trying to solve is even more extreme.  I have a log
of messages (100's of them) that my script is generating.  Each message is
displayed in a span with the class set according to the severity of the message
(info/warning/error).  I want to conditionally display/hide messages based on
their severity.
True, true.  The CSSOM is just such a mess and a pain to implement that I try to
advocate avoiding it.
Future.
Target Milestone: --- → Future
jst: how big a pain is this to fix?

kieffer: for the example you quote, can you work around the issue by giving the
rule some property which will make the rule appear in cssRules[]? e.g. .msg {
display: inline; } ?
Whiteboard: [DIGBug][eapp]
bz: Do you have an answer for Bob's question above? I can't really tell w/o
digging into the CSS internals...
> how big a pain is this to fix?

I _think_ this may be fixable by just changing the following code in
ParseDeclarationBlock in the CSS Parser:

    if (dropDeclaration ||
        (0 == count)) { // didn't get any XXX is this ok with the DOM?
      if (nsnull != declaration) {
        declaration->RuleAbort();
        declaration = nsnull;
      }
    }

The thing to do would be to remove the |0 == count| part (and of course the
comment).

David?  What do you think?
It seems like it should just work, although we might want to double-check that
the index / bits stuff still works.
Major corporations depend on eapp bugs, and need them to be fixed before they
can recommend Mozilla-based products to their customers. Adding nsbeta1+ keyword
and making sure the bugs get re-evaluated if they are targeted beyond 1.0.
Keywords: nsbeta1+
Target Milestone: Future → ---
eapp was incorrectly used to change this to nsbeta1+. Resetting to nsbeta1 to
nominate. This is an important issue and deserves to be nsbeta1+ by the ADT.
Keywords: nsbeta1+ → nsbeta1
I've been playing around with this some more and found a better workaround.  A
style object can be created programatically using the sheet.insertRule() method.
 So, as long as you have a <STYLE> block in your document, you can add blank
rules to it.  E.g:

    window.mySheet = document.getElementsByTagName('style')[0].sheet;
    mySheet.insertRule('.foobar {background-color:red}', 0);

Given that this is not too painful a workaround, the priority of this bug might
be lowered.
Comment on attachment 69894 [details] [diff] [review]
Patch.  Tested this for a bit and everything seems to be working correctly...

r=dbaron
Attachment #69894 - Flags: review+
Comment on attachment 69894 [details] [diff] [review]
Patch.  Tested this for a bit and everything seems to be working correctly...

sr=attinasi
Attachment #69894 - Flags: superreview+
Since Boris made the fix he can have the glory. Reassigning and setting
milestone to 1.0 (shouldn't be a problem I hope, since there is a fix). Boris,
if you haven't done so already could you ask for drivers' approval?
Assignee: jst → bzbarsky
Keywords: mozilla1.0
Target Milestone: --- → mozilla1.0
Oops.  I checked this in a month ago... :)

nsCSSParser.cpp
revision 3.174
date: 2002/02/19 02:27:38;  author: bzbarsky%mit.edu;  state: Exp;  lines: +1 -2
Leave rules with empty decl blocks in the sheet so they're picked up by
the DOM.  Bug 112854, r=dbaron, sr=attinasi
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: