207477 - wrong rendering of websites (after Ctrl+Shift+R)

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Adam Hauner]

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]

Attachment: multimoz.org (wrong rendering)

All screenshots are from 2003052805/1.4branch/W2K

Comment 2

[Adam Hauner]

Attachment: multimoz.org (correct rendering)

Comment 3

[Adam Hauner]

Attachment: slunecnice.cz (wrong rendering)

Comment 4

[Adam Hauner]

Attachment: slunecnice.cz (correct rendering)

Comment 5

[Jeff D. Hanson]

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)]

WFM on WinXP 2003052808, but I have seen it at other sites.

Comment 7

[Adam Hauner]

Confirming according comment #5.

Comment 8

[Adam Hauner]

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-4, no longer working on Mozilla)]

Might this have the same cause as bug 194638?

Comment 10

[Adam Hauner]

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-4, no longer working on Mozilla)]

Attachment: simple testcase

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

While poking around, I noticed that nsBlockReflowContext::mBlockShouldInvalidate
is always PR_FALSE and should be removed.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it. I think it's the right thing.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

Attachment: 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-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

fixed

Comment 24

[Adam Hauner]

Works fine with 2003071508/trunk/W2K on both sites and testcase. Thanx!
v=aha

Comment 25

[Asa Dotzler [:asa]]

mozilla1.5a released. unsetting request.