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)
Core
CSS Parsing and Computation
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
scc
:
approval+
|
Details | Diff | Splinter Review |
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
Comment 1•24 years ago
|
||
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).
Comment 4•24 years ago
|
||
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
Comment 5•24 years ago
|
||
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
Comment 6•24 years ago
|
||
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
Comment 7•24 years ago
|
||
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
Comment 8•24 years ago
|
||
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/
Comment 9•24 years ago
|
||
Ian, you are right. Hacks suck. Thanks.
Updated•23 years ago
|
Target Milestone: mozilla1.0 → mozilla1.2
Comment 10•23 years ago
|
||
leaving in 1.2
Severity: normal → minor
Priority: P3 → P4
Whiteboard: [Hixie-P4] → [Hixie-P4][bae:20011129]
Comment 11•23 years ago
|
||
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
Comment 12•23 years ago
|
||
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
Assignee | ||
Comment 13•23 years ago
|
||
*** Bug 80607 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•23 years ago
|
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
Comment 14•23 years ago
|
||
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
Comment 15•23 years ago
|
||
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
Assignee | ||
Comment 16•23 years ago
|
||
*** Bug 118560 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 17•23 years ago
|
||
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.
Comment 18•23 years ago
|
||
*** Bug 126400 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Whiteboard: [Hixie-P4][bae:20011129] → [Hixie-P4][bae:20011129][CSS1-5.3.2]
Assignee | ||
Comment 19•23 years ago
|
||
Taking and targetting for mozilla1.0.
Assignee: attinasi → dbaron
Status: ASSIGNED → NEW
Target Milestone: mozilla1.2 → mozilla1.0
Assignee | ||
Comment 20•23 years ago
|
||
Bug 99649 would probably be fixed by a fix to this bug (and the workaround could
be removed).
Assignee | ||
Comment 21•23 years ago
|
||
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 22•23 years ago
|
||
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+
Assignee | ||
Comment 23•23 years ago
|
||
> 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
Comment 24•23 years ago
|
||
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?
Assignee | ||
Comment 25•23 years ago
|
||
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 26•23 years ago
|
||
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+
Assignee | ||
Comment 27•23 years ago
|
||
This patch removes the extra scoping in a block and adds the debug variable
that bzbarsky suggested.
Attachment #74583 -
Attachment is obsolete: true
Assignee | ||
Comment 28•23 years ago
|
||
Comment on attachment 75254 [details] [diff] [review]
patch addressing review comments
Transferring r/sr.
Attachment #75254 -
Flags: superreview+
Attachment #75254 -
Flags: review+
Comment 29•23 years ago
|
||
Comment on attachment 75254 [details] [diff] [review]
patch addressing review comments
a=scc
Attachment #75254 -
Flags: approval+
Assignee | ||
Comment 30•23 years ago
|
||
Fix checked in 2002-03-21 15:51 PST.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 31•23 years ago
|
||
*** Bug 69400 has been marked as a duplicate of this bug. ***
Comment 32•23 years ago
|
||
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
Assignee | ||
Comment 33•23 years ago
|
||
*** Bug 127594 has been marked as a duplicate of this bug. ***
Comment 34•23 years ago
|
||
*** Bug 99649 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 35•23 years ago
|
||
*** Bug 135113 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 36•23 years ago
|
||
*** Bug 135464 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 37•23 years ago
|
||
*** Bug 138201 has been marked as a duplicate of this bug. ***
Comment 38•23 years ago
|
||
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...?
Assignee | ||
Comment 39•23 years ago
|
||
... or comment on bug 15608, which I just did.
Updated•23 years ago
|
Status: RESOLVED → VERIFIED
Comment 40•23 years ago
|
||
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.
Description
•