Closed
Bug 131397
Opened 22 years ago
Closed 22 years ago
[FIX]removeProperty on a CSS declaration from a style rule does not update document
Categories
(Core :: DOM: CSS Object Model, defect, P1)
Core
DOM: CSS Object Model
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: sebastianf, Assigned: bzbarsky)
Details
Attachments
(2 files, 2 obsolete files)
2.94 KB,
text/html
|
Details | |
1.75 KB,
patch
|
dbaron
:
review+
jst
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
Updated•22 years ago
|
Attachment #74507 -
Attachment mime type: text/xhtml → application/xhtml+xml
Comment 2•22 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.
Assignee | ||
Comment 3•22 years ago
|
||
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
Assignee | ||
Comment 4•22 years ago
|
||
Make RemoveProperty send update notifications
Assignee | ||
Comment 5•22 years ago
|
||
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•22 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•22 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?
Assignee | ||
Comment 9•22 years ago
|
||
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 10•22 years ago
|
||
Comment on attachment 74625 [details] [diff] [review] Proposed patch sr=jst
Attachment #74625 -
Flags: superreview+
Assignee | ||
Comment 11•22 years ago
|
||
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 13•22 years ago
|
||
Comment on attachment 74764 [details] [diff] [review] Better patch that avoids perpetuating bug 131775 sr=jst
Attachment #74764 -
Flags: superreview+
Updated•22 years ago
|
Attachment #74764 -
Flags: approval+
Comment 14•22 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
Assignee | ||
Comment 15•22 years ago
|
||
fix checked in on trunk
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.
Description
•