Last Comment Bug 207477 - wrong rendering of websites (after Ctrl+Shift+R)
: wrong rendering of websites (after Ctrl+Shift+R)
Status: VERIFIED FIXED
: testcase
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
: Hixie (not reading bugmail)
Mentors:
http://www.slunecnice.cz/
Depends on: 194638
Blocks: 195598 204289
  Show dependency treegraph
 
Reported: 2003-05-29 05:31 PDT by Adam Hauner
Modified: 2003-07-27 18:56 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
multimoz.org (wrong rendering) (4.12 KB, image/png)
2003-05-29 05:33 PDT, Adam Hauner
no flags Details
multimoz.org (correct rendering) (4.07 KB, image/png)
2003-05-29 05:33 PDT, Adam Hauner
no flags Details
slunecnice.cz (wrong rendering) (25.00 KB, image/png)
2003-05-29 05:34 PDT, Adam Hauner
no flags Details
slunecnice.cz (correct rendering) (24.87 KB, image/png)
2003-05-29 05:34 PDT, Adam Hauner
no flags Details
simple testcase (530 bytes, text/html; charset=UTF-8)
2003-07-09 15:49 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
patch that allows the bug to be seen on Linux (3.42 KB, patch)
2003-07-10 17:55 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
simple testcase (609 bytes, text/html; charset=UTF-8)
2003-07-10 18:27 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
fix (1.17 KB, patch)
2003-07-13 14:47 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
better fix (9.49 KB, patch)
2003-07-13 16:09 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Review

Description Adam Hauner 2003-05-29 05:31:51 PDT
Repro:
1. go to http://www.slunecnice.cz/ or http://musicmoz.org/ for first time
   or reload it with Ctrl+Shift+R

Actual:
Headers of both sides are rendered wrong, on right side are artefacts. They
disappear after fisrt reflow and page looks well, but first impression of
website (or Mozilla rendering) is very bad.

See attached screenshots.
Comment 1 Adam Hauner 2003-05-29 05:33:29 PDT
Created attachment 124434 [details]
multimoz.org (wrong rendering)

All screenshots are from 2003052805/1.4branch/W2K
Comment 2 Adam Hauner 2003-05-29 05:33:54 PDT
Created attachment 124435 [details]
multimoz.org (correct rendering)
Comment 3 Adam Hauner 2003-05-29 05:34:34 PDT
Created attachment 124436 [details]
slunecnice.cz (wrong rendering)
Comment 4 Adam Hauner 2003-05-29 05:34:56 PDT
Created attachment 124437 [details]
slunecnice.cz (correct rendering)
Comment 5 Jeff D. Hanson 2003-05-29 07:27:52 PDT
Confirming:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030527

Saw the defects as shown in the attachments posted.  Also notice problems with
the column widths in the middle of the http://musicmoz.org/ page.  They changed
when refreshed.
Comment 6 Jason Barnabe (np) 2003-05-29 21:04:20 PDT
WFM on WinXP 2003052808, but I have seen it at other sites.
Comment 7 Adam Hauner 2003-06-04 00:27:47 PDT
Confirming according comment #5.
Comment 8 Adam Hauner 2003-06-10 23:47:26 PDT
With today branch build I was unable to reproduce on Slunecnice.cz, so I'm
clearing request to block 1.4.
But rendering is still broken with trunk build.
-> Layout: View Rendering
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-08 21:16:56 PDT
Might this have the same cause as bug 194638?
Comment 10 Adam Hauner 2003-07-08 23:46:36 PDT
David: Probably yes, because rendering gets wrong after scrollbars appear.
Layout is centered and when scrollbars appear, the real center of viewport has
to move several pixels to left.

(So caused also by fixing bug 190193?)
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-09 15:49:28 PDT
Created attachment 127388 [details]
simple testcase
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-10 17:55:38 PDT
Created attachment 127497 [details] [diff] [review]
patch that allows the bug to be seen on Linux

This changes the Linux painting code so that the bug can be debugged on Linux.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-10 18:27:26 PDT
Created attachment 127499 [details]
simple testcase

This one requires clicking, but clicking twice shows something that may be
interesting.

I think the problem may be related to being missing an invalidation codepath
for the relevant case for when a block moves -- but we're getting most of the
invalidation thanks to the size change.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-10 18:29:59 PDT
Note that if I change the 'width: 50%' in attachment 127499 [details] to 'width: 10em'
(i.e., from percentage width to fixed width), there's no invalidation at all,
and I need to move another window over part of the document to see that
something has moved.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-07-13 13:57:52 PDT
While poking around, I noticed that nsBlockReflowContext::mBlockShouldInvalidate
is always PR_FALSE and should be removed.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-07-13 14:47:35 PDT
Created attachment 127675 [details] [diff] [review]
fix

This fixes it. I think it's the right thing.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-07-13 14:49:29 PDT
I assume that the intent of passing PR_FALSE into aDamageDirtyArea in
nsBlockFrame::DirtyLine is to avoid unnecessary invalidations if the entire
block is going to be repainted anyway. This is not the case for a Resize reflow.

By the way, I've noticed (with paint flashing) that when we resize a browser
window, the entire window contents is repainted no matter what.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-13 14:54:52 PDT
How about calling this |forceInvalidate| and getting rid of the other
|forceInvalidate|, and getting rid of |BRS_DAMAGECONSTRAINED| too, since it's
never set?  (It was an old optimization for text inputs before we had reflow roots.)
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-13 14:57:42 PDT
And feel free to get rid of mBlockShouldInvalidateItself too.  (It's obviously
broken if it were used, since the |Invalidate| calls use |mRect|, which is in
the wrong coordinate system.)
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-07-13 16:09:29 PDT
Created attachment 127683 [details] [diff] [review]
better fix

includes the cleanups suggested by dbaron. Also includes a few fixes for
warnings I'm heartily sick of.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-13 16:18:39 PDT
Comment on attachment 127683 [details] [diff] [review]
better fix

>   // Check whether this is an incremental reflow
>-  PRBool  incrementalReflow = aState.mReflowState.reason ==
>-                              eReflowReason_Incremental ||
>-                              aState.mReflowState.reason ==
>-                              eReflowReason_Dirty;
>+  PRBool doInvalidates =
>+    aState.mReflowState.reason == eReflowReason_Incremental
>+    || aState.mReflowState.reason == eReflowReason_Dirty
>+    || aState.mReflowState.reason == eReflowReason_Resize;

Could you put the || at the end of the line to be consistent with the existing
style?

>       rv = ReflowLine(aState, line, &keepGoing, forceInvalidate);

and rename the variable where you use it, too?

>+      rv = ReflowLine(aState, line, &keepGoing, forceInvalidate);

here too?

Perhaps it would be better to use NS_STATIC_CAST(void*, ...) rather than
(void*) to be consistent with the casts already there?

Once you compile, and with those changes, r+sr=dbaron.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-07-13 17:18:50 PDT
oops ... I decided to change the name to doInvalidate at the last minute.
forceInvalidate is a misnomer since the default behavior is with it set to
PR_TRUE; setting it to PR_FALSE is an optimization.

I'll do all you suggest, including using NS_STATIC_CAST. Thanks.
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-07-14 18:01:01 PDT
fixed
Comment 24 Adam Hauner 2003-07-15 14:32:04 PDT
Works fine with 2003071508/trunk/W2K on both sites and testcase. Thanx!
v=aha
Comment 25 Asa Dotzler [:asa] 2003-07-27 18:56:50 PDT
mozilla1.5a released. unsetting request.

Note You need to log in before you can comment on or make changes to this bug.