Closed Bug 63863 Opened 24 years ago Closed 23 years ago

non-attribute change to BODY background doesn't repaint whole window

Categories

(Core :: CSS Parsing and Computation, defect, P4)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: leonard, Assigned: dbaron)

References

()

Details

(Keywords: css1, testcase, Whiteboard: [Hixie-P4][bae:20011129][CSS1-5.3.2])

Attachments

(2 files, 1 obsolete file)

From Bugzilla Helper: User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows NT 5.0) BuildID: 200122705 when called, the background color is only changed in a box around the html elements until the window is refreshed (scrolled, resized, or overlapped), after which the entire body becomes the appropriate color. Reproducible: Always Steps to Reproduce: 1. load page 2. click button (call method) 3. see results Actual Results: incorrect behavior as described (rendered only part of body with different color until refreshing of window) Expected Results: rendered the complete body with a different color
Confirmed Platform: PC OS: Linux 2.2.16 Mozilla Build: 2001010211 M18 Trunk Build Note: If you switch Mozilla windows and then switch back the entire page is covered like it should. My guess is something is not being redrawn properly and is only triggered when Mozilla is forced to redraw. Marking NEW.
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Testcase above: Incomplete change of background-color when dynamically changing 'className' of BODY. The background color of the BODY element should cover the entire canvas (unless HTML element has color other than transparent specified, of course). However, when the background color of the BODY is dynamically changed by changing the BODY's className property, only the background of BODY itself is changed.The attachment contains four BODY style options. Click on them to see that only the BODY background changes. Try moving another window over Mozilla window to see partial repainting. Resize Mozilla window to see total repaint (- though when the browser window is resized within the minimum set by the Menu bar [bug#39738], only partial repaint occurs).
Netscape's standard compliance QA team reorganised itself once again, so taking remaining non-tables style bugs. Sorry about the spam. I tried to get this done directly at the database level, but apparently that is "not easy because of the shadow db", "plus it screws up the audit trail", so no can do...
QA Contact: chrisd → ian
The Body fix-up rule strikes again. It is an invalidation problem: when we propagate the background color, we should invalidate the canvas - not just the body. Reassigned to Marc, like bug 69400.
Assignee: pierre → attinasi
Blocks: 69400
Currently, this works for changes to the bgcolor and background attributes because of a hack in nsCSSFrameConstructor::AttributeChanged that looks for changes to those attributes on the BODY element (I put that in a long time ago). Changing the class (or ID) of the BODY will circumvent this, so we have a couple of choices. 1) change the logic in the AttributeChanged method of the frameConstructor to look for changes to the BODY element's class, id, bgcolor, and background attributes, or 2) try to mange the invalidation where the color is actually propogated, in teh BodyFixupRule. The problem with (2) is that the BodyFixup rule happens at style resolution time, and it is not clear if invalidates at that time are always a good thing. It seems like less of a hack, however, so I'll investigate that approach now. Approach (1) is very simple, simply adding two attributes to a check that is already there (although the logic can be changed to make it slightly more efficient, I think).
Status: NEW → ASSIGNED
Milestone Moz 1.0 - as I indicated before, there is a quick way to fix this, by extending the current hack in AttributeChanged, but I am not convinced that this is the best approach, really.
Priority: -- → P3
Target Milestone: --- → mozilla1.0
Marc: Approach (1) won't work for pathological cases like having a class on the head element which is changed with the following style rules in effect: head.class1 + body { background: red; } head.class2 + body { background: green; } Sorry... :-) See: http://www.hixie.ch/tests/adhoc/css/background/dynamic/
Keywords: css1, testcase
Whiteboard: [Hixie-P4]
Ian, you are right. Hacks suck. Thanks.
Target Milestone: mozilla1.0 → mozilla1.2
leaving in 1.2
Severity: normal → minor
Priority: P3 → P4
Whiteboard: [Hixie-P4] → [Hixie-P4][bae:20011129]
I am noticing very similar behavior in Composer rev. 2001120303 My steps to reproduce: 1. Launch composer and create a new blank page 2. From the toolbar icon, assign the page a background color (e.g. blue) 3. From the menubar select EDIT then UNDO Expected behavior: The page background color should revert to the one previously set. Actual behavior: The background color will remain instead of reverting to the one assigned before the undo. The HTML code is actually undone properly although it won't display the undone background color in the "Normal" view until either the "Show all tags" or "Preview" tabs are clicked first. If the "HTML Source" tab is clicked, the Normal view will still show the pre-undo background color. This doesn't appear to be strictly a redraw issue because if I mimize Composer and then restore, it will still show the pre-undo bg color. Also, if a background color is assigned via the FORMAT menu it will undo correctly. From here, subsequent changes to the bg color will also undo correctly. Tested on win 98
resummarizing to reflect the real bug here. Changing body.style.backgroundColor should actually work now. It's toggling the body style by changing its class that fails.
Summary: body.style.backgroundColor renders incorrectly → Changing class or id of body does not repaint background of whole window
*** Bug 80607 has been marked as a duplicate of this bug. ***
Summary: Changing class or id of body does not repaint background of whole window → non-attribute change to BODY background doesn't repaint whole window
After David Baron's fix in bug 87103, I am seeing repainting issues with the following testcase: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=35929 When switching styles between 'Default Style' and 'none', the background doesn't get repainted properly. The curious thing is that if the focus is somewhere other than the URL bar and the style is switched, clicking in the URL bar forces a proper repaint. Either way, this is working better than before. Before when switching styles to 'none', the background would turn white (default), the the white text wouldn't turn to the default color (black) thereby making most of the text on the page invisible. At least that is fixed. Jake
The same issue I just reported causes problems for the tescase in bug 88681 http://bugzilla.mozilla.org/attachment.cgi?id=40818&action=view The same workaround as I reported in comment 14 applies here also. Also, the testcase mentioned in comment 14 was attached to bug 82446 which was duped to bug 47760 which was supposed to have been fixed by the fix for bug 116161. Seems like bug 88681, 47760, and others that were fixed but regressed somewhat from the patch to 116161 should be re-opened and duped to this or just re- opened and depend on this bug. I'm just trying to show why this bug ought to be looked at well before Mozilla 1.2. Jake
*** Bug 118560 has been marked as a duplicate of this bug. ***
I started to attempt work on a fix for this in nsFrameManager.cpp, and I think this is the right approach. I should probably finish it sometime... @@ -1787,6 +1747,19 @@ // stop the image loading for the frame, the image has changed aPresContext->StopImagesFor(aFrame); } + + const nsStyleBackground *bg; + PRBool isCanvas; + if (aMinChange >= NS_STYLE_HINT_VISUAL && + !nsCSSRendering::FindBackground(aPresContext, aFrame, + &bg, &isCanvas)) { + // This frame's background gets painted on the canvas, so + // invalidate the canvas. + nsIFrame *canvas; + GetCanvasFrame(aPresContext, &canvas); + // XXX Need to call the static function |ApplyRenderingChangeToTree| + // in nsCSSFrameConstructor.cpp + } } else { // XXXdwh figure this out.
*** Bug 126400 has been marked as a duplicate of this bug. ***
Whiteboard: [Hixie-P4][bae:20011129] → [Hixie-P4][bae:20011129][CSS1-5.3.2]
Taking and targetting for mozilla1.0.
Assignee: attinasi → dbaron
Status: ASSIGNED → NEW
Target Milestone: mozilla1.2 → mozilla1.0
Bug 99649 would probably be fixed by a fix to this bug (and the workaround could be removed).
Attached patch proposed fix (obsolete) (deleted) — Splinter Review
Much of this fix is a restructuring of ApplyRenderingChangeToTree to make it do less work when it recurs through a placeholder. I split it into Apply... and DoApply... and added a little bit more work to the outer Apply... (calling FindBackground in a loop). I also had to add a similar loop to the StyleChangedReflow since the reflow doesn't invalidate based on FindBackground.
Comment on attachment 74583 [details] [diff] [review] proposed fix It's not clear to me why you changed the looping construct here: - while (nsnull != aFrame) { + for ( ; aFrame; aFrame->GetNextInFlow(&aFrame)) but they appear equiv. anyway. The assumption that the view manager passed to DoApplyRenderingChangeToTree seems valid to me - but I am not sure. Are you positive that the view manager for any frame's view will be the one passed in? If so, that is a good change. Is there an unnecessary scoping level here? + // If the frame's background is propagated to an ancestor, walk up to + // that ancestor. + { A comment about the 'parent of the ancestor' might be in order here: + while (!nsCSSRendering::FindBackground(aPresContext, ancestor, + &bg, &isCanvas)) { + ancestor->GetParent(&ancestor); + NS_ASSERTION(ancestor, "canvas must paint"); + } I like the removal of my old hack - a lot. sr=attinasi
Attachment #74583 - Flags: superreview+
> It's not clear to me why you changed the looping construct here: > - while (nsnull != aFrame) { > + for ( ; aFrame; aFrame->GetNextInFlow(&aFrame)) > but they appear equiv. anyway. The loop was tall enough that I couldn't see the end from the beginning in one screen, so I couldn't tell what it was iterating over when looking at the top. I just find it easier to read code that way. > The assumption that the view manager passed to DoApplyRenderingChangeToTree > seems valid to me - but I am not sure. Are you positive that the view manager > for any frame's view will be the one passed in? If so, that is a good change. I'm not quite sure what you mean, but I'm pretty sure that the new code is equivalent to the old code with respect to what view manager it will use -- it just removes the null checks since it makes sure to get the view manager in the outer function so that there's no need to check for it on every iteration. > Is there an unnecessary scoping level here? > + // If the frame's background is propagated to an ancestor, walk up to > + // that ancestor. > + { Yes. I wanted to scope those variables since they're "useless" so I didn't want them to leak out. Maybe I'm the only one who likes extra blocks to restrict the scope of variables? :-) > A comment about the 'parent of the ancestor' might be in order here: > + while (!nsCSSRendering::FindBackground(aPresContext, ancestor, > + &bg, &isCanvas)) { > + ancestor->GetParent(&ancestor); > + NS_ASSERTION(ancestor, "canvas must paint"); > + } A comment saying what? Does "Search for the frame that paints this frame's background" seem reasonable?
Status: NEW → ASSIGNED
I'm a little confused by one thing: - ApplyRenderingChangeToTree(aPresContext, outOfFlowFrame, aViewManager); + DoApplyRenderingChangeToTree(aPresContext, outOfFlowFrame, aViewManager); ApplyRenderingChangeToTree used to BeginUpdateViewBatch and EndUpdateViewBatch. DoApplyRenderingChangeToTree does not do this. Does the caller need to handle this itself?
That change is part of the mutual recursion between ApplyRenderingChangeToTree, DoApplyRenderingChangeToTree, UpdateViewsForTree, and SyncAndInvalidateView. Perhaps I should comment that certain functions are only meant to be called within ApplyRenderingChangeToTree so someone doesn't start using them for something else.
Comment on attachment 74583 [details] [diff] [review] proposed fix Ah, ok. Makes sense. Yes, a comment like that would be nice. Even nicer would be a debug var that we set to 1 at the top of ApplyRenderingChangeToTree, set to 0 before returning from it. And then the other functions can assert on it as needed...
Attachment #74583 - Flags: review+
This patch removes the extra scoping in a block and adds the debug variable that bzbarsky suggested.
Attachment #74583 - Attachment is obsolete: true
Comment on attachment 75254 [details] [diff] [review] patch addressing review comments Transferring r/sr.
Attachment #75254 - Flags: superreview+
Attachment #75254 - Flags: review+
Comment on attachment 75254 [details] [diff] [review] patch addressing review comments a=scc
Attachment #75254 - Flags: approval+
Fix checked in 2002-03-21 15:51 PST.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
No longer blocks: 69400
*** Bug 69400 has been marked as a duplicate of this bug. ***
Hmm... I'm seeing a possible glitch here. I checked out the libpr0n site and two of the stylesheets switch quite nicely, but a 3rd doesn't. To reproduce: 1. go to http://www.libpr0n.com/ 2. try switching to the "blue" stylesheet 3. notice the lower half of the screen stays white where it should have truned blue like the upper part of the screen where the content is. 4. To get the whole window painted, just expand the window in any way. The blue will snap to cover the whole content are of the the window. Note that switching between "none" and "green" work just fine. It is strange that this one doesn't work becuase the fix here makes so many other style sheet switches work properly where they didn't before this fix. Jake
*** Bug 127594 has been marked as a duplicate of this bug. ***
*** Bug 99649 has been marked as a duplicate of this bug. ***
*** Bug 135113 has been marked as a duplicate of this bug. ***
*** Bug 135464 has been marked as a duplicate of this bug. ***
*** Bug 138201 has been marked as a duplicate of this bug. ***
http://www.hixie.ch/tests/adhoc/css/background/dynamic/001.html is still broken. Should I reopen or should I file a new bug or...?
... or comment on bug 15608, which I just did.
Status: RESOLVED → VERIFIED
Alrighty then. VERIFIED that the other test cases work as expected.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: