Closed Bug 88681 Opened 24 years ago Closed 24 years ago

style change to background-color or background-image after disabling a style sheet is not complete

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
Windows 95
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: martin.honnen, Assigned: glazou)

References

Details

(Keywords: regression, Whiteboard: [ruletree], fix in hand)

Attachments

(4 files)

bug demo (button enables/disables second stylesheet which should toggle page background green/lightblue but instead a green stripe on lightblue occurs)
621 bytes, text/html
Details
patch v0.9
8.39 KB, patch
Details | Diff | Splinter Review
patch v1.0
11.20 KB, patch
Details | Diff | Splinter Review
patch v1.1
12.52 KB, patch
Details | Diff | Splinter Review
With the following example the page background starts green as intended then gets blue when you press the button first time but then when you press the button again the page background should get green again but only gets a green stripe the height of the button. I tested with M0.9.2 <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> <html> <head> <title> style disabling </title> <style type="text/css"> body { background-color: green; } </style> <style type="text/css"> body { background-color: lightblue; } </style> <script type="text/javascript"> if (document.styleSheets) document.styleSheets[1].disabled = true; </script> </head> <body> <input type="button" value="toggle second stylesheet" onclick="document.styleSheets[1].disabled = !document.styleSheets[1].disabled;" /> </body> </html>
confirming ; the background of body is not propagated in that case. I discovered that bug during my implementation of :target but, for an unknown reason, did not file it. Looking into it.
After a 3days attack from the dark forces of the planet Caffeïne against the Headache guerilla, a fight full of dignity blood and sense of honour, I am pleased to announce that I have a patch fixing this bug. I need a little rest to understand fully and precisely (a) how my brain produced such a result (b) why it works, but it seems that I could be in the position to shout YAYAYYAAAHHHHHHAA in the coming hours. /nick glazou_break....
patch 0.9 attached ; no code cleanup nor optimization yet.
Daniel, can you please explain what the patch is doing? Also, are you looking for review comments yet?
QA Contact: ian
Summary: style change to background-color after disabling a style sheet is not complete → style change to background-color or background-image after disabling a style sheet is not complete
I've tried glazman's 7/04 patch and it seems to work. It also fixes the problem of not reseting the background image after disabling a style sheet -- this is demonstrated by using Composer, which uses the style sheet "EditorAllTags.css" to show an icon for HTML tags: In any composer page, click on the "Show All Tags" tab at the bottom or from the View menu. Then click back on "Normal" mode. Without this fix, the yellow image with the text "BODY" sideways remains visible. Also, if user's page did have a background image, their image is shifted because of the margin rules in EditorAllTags.css. Marc and Daniel: Should we try to get this fix in for NS 6.1?
Sorry if I did not have to time to answer yestersday, Marc. Ok here are some explanations about what I did in the patch. It works but to be honest, I don't know if it is the best way to solve the problem and if it is not only a work-around. To be honest again, I am a little bit lost in the style engine since hyatt made his good-n-big landing (see bug 87663). That's why I wrote "patch 0.9" and not "1.0". Marc, your expertise is needed ;-) I detected that we do not update the BodyFixupRule or even regenerate it if the document contains two stylesheets, noth applying a background, and if we remove the second one. In the test case above, we apply correctly the green background to body but still apply the BodyFixupRule created when the body had to be lightblue. So my patch deletes the BodyFixupRule when a stylesheet is disabled and obliges the code to recreate a new one if necessary. nsIBodySuper is needed just in order to expose a BodyFixupRule and be able to remove one. Charley: yes, I was pretty sure that it was also fixing that editor bug because the problem is exactly the same. Marc: always destroying the BodyFixupRule when a styelsheet is disabled or removed is probably sub-optimal. It could be removed only if that stylesheet sets any background property.
additional test case which was developed prior to the bug being filed ... http://www.cherny.com/mozilla/css_switch/ here the backgrounds should change color ... thanks for filing martin!
Depends on: 84172
Oh how I detest the BodyFixupRule. I think your approach is sound. I hate that we have to make the PresShell know about the BodyFixupRule though. Can we instead do it in the StyleSet? I'm not too worried about doing this every time a stylesheet is enabled/disabled or added/removed as we pretty much have to tear down the world in those cases anyway. Also, is the doc->GetElementsByTagName approach really the best way to get the body element? It seems like it might be faster to get the root content element then walk it looking for the body (it is generally the first child I think). Just guessing, but if you already checked it out then I trust you ;)
*** Bug 91084 has been marked as a duplicate of this bug. ***
*** Bug 84172 has been marked as a duplicate of this bug. ***
second child, ignoring whitespace text nodes, assuming the DOM is sane. It will definitely be a child of the root <html> element though, otherwise the body fixup rule would not apply.
reassigned to Daniel
Assignee: pierre → glazman
Ian, can you please explain your comment above ? I am probably tired but I did no understand it...
ok, understood... Really tired !
Ok, rewriting the patch according to Marc's comments.
It seems to be a regression from the rule tree landing (bug 78695). Marking dependency on the meta bug 91672, which lists other regressions related to the background and the BodyFixupRule.
Blocks: 91672
Keywords: regression
Whiteboard: [ruletree]
I am not sure it is a regression. I think that this bug was already there a long time ago, before David Hyatt's big change (timeless confirms).
new patch done ; will attach tomorrow from work (modem link right now :-( ) (a) knowledge of BodyFixupRule passed on to StyleSet (b) better research of BODY element just looking at the children of the root element of the document instead of calling GetElementByName tested, works fine
FYI: I think the specific issue of the "body" background image not being removed is a regression.
Patch v1.0 attached in answer to Marc's comments. [s]r= needed please.
Whiteboard: [ruletree] → [ruletree], fix in hand
I think you have some extraneous #includes: +#include "nsIDOMHTMLDocument.h" +#include "nsIDOMNodeList.h" +#include "nsIDOMNode.h" +#include "nsIDOMElement.h" Check those out please, sr=attinasi
- The StyleSetImpl::RemoveBodyFixupRule() method should not be in the StyleSet but for instance in the nsHTMLDocument which already has a quicker way to find the body element (nsHTMLDocument::GetBodyElement). The nsIDocument is passed to PresShell::StyleSheetDisabledStateChanged() already. - The nsBodySuper::RemoveBodyFixupRule() method would fit better in the nsHTMLBodyElement. BodySuper was defined only to override SetDocument. Basically the BodyFixup code would stay where it belongs in the HTMLDocument and the BodyElement, while the StyleSet would be left untouched. Marc, do you agree? Let's not make Daniel write more than 3 patches for this :-(
QA Contact: ian
I disagree about putting BodyFixupRule knowledge in the HTMLDocument. It may be easier, but I think that spreading the knowledge of the BodyFixupRule around is a mistake, and currently the HTMLDocument knows nothing about it. That is why I originally suggested that we move it out of the PresShell. What semantic relationship is there between the Document and the BodyFixupRule? Do we want to create one? I have no opinion on BodySuper over HTMLBodyElement for RemoveBodyFixupRule - I thought that Daniel chose to create the BodySuper interface to avoid changing existing interfaces, but I may be wrong.
Marc : absolutely right. I just chose the easiest and simplest way to give the visibility of a BodyFixupRule outside of a nsHTMLBodyElement. I think that what Pierre proposes is a much more difficult task, for a result which is not less hacky than the rest of BodyFixupRule anyway. I also think that the BodyFixupRule has much more to do with a StyleSet than with a Document.
I see your point for the BodySuper, I had overlooked the class hierarchy. About RemoveBodyFixupRule(), you say that it has much more to do with a StyleSet than with a Document but: - The first thing RemoveBodyFixupRule() does is to receive the Document as a parameter and then nothing in the method uses any member variables nor any methods from the StyleSet. - It implements in the StyleSet a knowledge of the Document that doesn't exist anywhere else except (that's quite understandable) in AddDocStyleSheet(). The StyleSet doesn't need to know anything about the structure of a document, it doesn't even have to know about HTML documents versus other types of documents. - It makes the assumption (certainly valid but that doesn't seem to be hardcoded anywhere else) that the body is a child of the root element and it does the extra processing of iterating through the children to find it. Now I understand it is a hack and we could argue all day about where it doesn't fit, so we might as well leave it where it is but with the following changes: - Implement GetBodyElement(nsIDOMNode**) in nsIHTMLDocument. It's 2 lines of code. - In StyleSetImpl::RemoveBodyFixupRule(), query interface on the document, get the body, query interface on the BodySuper and call RemoveBodyFixupRule(). It's 4 lines of code instead of 40+, and much cleaner.
New patch attached in answer to last Pierre's comments and phone call between us. r=pierre ?
patch v1.1 looks great to me sr=attinasi
Fine with me but: - I asked hyatt to review the 2 lines that should be changed in nsRuleNode.cpp. - The attached patch contains the fix for an unrelated bug in nsRuleNode.cpp (it's about NS_STYLE_CONTENT_TAG_NAME). r=pierre
Remove the background == aSID part of the check. We should always exec the postresolvecallback if one exists, regardless of the type. r/sr=hyatt for that part with the change suggested above.
Hyatt: the place where I added a call to PostResolveCallback was returning |startStruct| without any call to PostResolveCallback... Are you sure you want a call in all cases ? About the NS_STYLE_CONTENT_TAG_NAME thing : yes, of course... sorry, If forgot to remove the corresponding chunk.
Daniel, yes. I'm going to be changing all the postresolve stuff shortly anyway to fix some other bugs, so it won't matter in the long run. :)
Actually I changed my mind. Just leave the patch the way you've coded it. That's the safest thing to do.
Status: NEW → ASSIGNED
it's in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
awesome! My testcase, at least with the background colors, works: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=35929 However, the text color doesn't change. That is bug 47760. Once that bug is fixed, my testcase will work perfectly and developers everywhere will rejoice! :-) Thanks for fixing this one Daniel! jake
BTW, I'm not sure but bug 80607 seems like a dup of this. Is it??? Jake
Ian, can you verify this bug and mark verified-fixed? thanks
Per http://bugzilla.mozilla.org/showattachment.cgi?attach_id=40818, VERIFIED FIXED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: