Closed Bug 88681 Opened 23 years ago Closed 23 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: 23 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: