[FIX]removeProperty on a CSS declaration from a style rule does not update document

VERIFIED FIXED in mozilla1.0

Status

()

Core
DOM: CSS Object Model
P1
major
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Sebastian Ferreyra, Assigned: bz)

Tracking

Trunk
mozilla1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
After calling someCssRule.removeAttribute( someAttribute ); the attribute is
effectively removed but apparently the engine does not recompute the style of
affected elements correctly, perhaps not at all.

For example someCssRule.setAttribute("display", none) hides an otherwise visible
element. But someCssRule.removeAttribute("display") does not restore it to its
original and default value. It's hard to think of a work around other than to
manually search for all elements that are affected by this rule and set the
properties by hand to their default values, of course it's also complicated to
calculate the default values for each element from Javascript.
(Reporter)

Comment 1

16 years ago
Created attachment 74507 [details]
A test case demonstrating the bug
Attachment #74507 - Attachment mime type: text/xhtml → application/xhtml+xml

Comment 2

16 years ago
CC'ing bz.
I think the description of this bug is wrong because CSS style rules don't have
a "setAttribute" method. However the testcase seems to be correct.
Created attachment 74624 [details]
Better testcase

The testcase has one "problem" (I don't think it's a bug, but it's an
annoyance).  The .selectorText of a rule like "[lang=es] { ... }" is
"*[lang=es]", not "[lang=es]".	That is, the implied "all elements" from the
sheet is explicitly present for the style rule's .selectorText.  Revised
testcase passes "*[lang=es]" as the selector so that we actually find the right
rule.  The problem described in the summary (not updating after
.removeProperty) is still present.
Attachment #74507 - Attachment is obsolete: true
Created attachment 74625 [details] [diff] [review]
Proposed patch

Make RemoveProperty send update notifications
Taking bug.

Sebastian, does the "revised testcase" pass in IE?  I assume that your original
testcase does...

If it does not, could you please file a separate bug on the .selectorText issue
and cc me on it?  The problem there is that the spec doesn't really say what the
selectorText should be... :(
Assignee: jst → bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: review
OS: Windows 2000 → All
Priority: -- → P1
Hardware: PC → All
Summary: After removing attributes through a CssRule, engine does not recompute affected element styles. → [FIX]removeProperty on a CSS declaration from a style rule does not update document
Target Milestone: --- → mozilla1.0

Comment 6

16 years ago
No question, Boris is the man!
Comment on attachment 74625 [details] [diff] [review]
Proposed patch

r=dbaron
Attachment #74625 - Flags: review+
(Reporter)

Comment 8

16 years ago
Ok, firstly, I'm sorry about the bad initial description, it was 5AM or so when
I filed the bug. I wrote .setAttribute instead of .setProperty because that's
the name of my JS function and also the non-w3c-standard function in IE
rule.style, no comments on that :)

Boris, when I first found the bug I was using class selectors, (example <h3
class="en"> ), somewhere in the middle I changed it to the more adequate
lang="en" and then changed the selectors too. I didn't test on IE after changing
this.

I tested it now. IE selectorText returns string 'UNKNOWN' if the selector in the
style definition is '[lang=es]' or '*[lang=es]' while it works for say: 'h3' or
'.es'. In other words, it doesnt work well in IE.

In Mozilla, at worst, it's annoying. If I had my choice I would go with the
explicit selectorText (leave it as it is) because it is better defined. That or
selectorText should be exactly what's on the css text.

w3 dom interface definition for interface CSSStyleRule:

selectorText of type DOMString 
The textual representation of the selector for the rule set. The implementation
may have stripped out insignificant whitespace while parsing the selector.

According to the definition above I understand that although it may strip
whitespace the text remains as is. Should this be filed as a bug?
I just talked to Ian (aka "Friendly CSS Working Group Member").  His opinion is
that given a selector of '[lang=es]' in the original sheet the DOM .selectorText
option should be either '*|*[lang=es]' or '*|*[lang="es"]'.

Changing from "*[lang=es]" to either "[lang=es]" or "*[lang=es]" depending on
what  was in the original sheet would be a good bit of work, since we don't keep
that information around right now.  Since the CSS working group is considering
recalling the entire DOM Style spec to Working Draft stage due to its ambiguity
and since no one else seems to implement .selectorText for these sorts of rules,
I doubt that anything will get changed about our handling of it until the spec
is vastly clarified.

This is not to say you shouldn't file a bug, just warning you that it may be a
while before anything happens with it... :)
Comment on attachment 74625 [details] [diff] [review]
Proposed patch

sr=jst
Attachment #74625 - Flags: superreview+
Created attachment 74764 [details] [diff] [review]
Better patch that avoids perpetuating bug 131775
Attachment #74625 - Attachment is obsolete: true
Comment on attachment 74764 [details] [diff] [review]
Better patch that avoids perpetuating bug 131775

r=dbaron
Attachment #74764 - Flags: review+
Comment on attachment 74764 [details] [diff] [review]
Better patch that avoids perpetuating bug 131775

sr=jst
Attachment #74764 - Flags: superreview+

Updated

16 years ago
Attachment #74764 - Flags: approval+

Comment 14

16 years ago
Comment on attachment 74764 [details] [diff] [review]
Better patch that avoids perpetuating bug 131775

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
fix checked in on trunk
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.