156985 - [FIX]XML document's not properly reflown if scrollbars required

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Johnny Stenback (:jst)]

Loading an XML file that is large enough that scrollbars are needed to show it
will cause the XML file to not show up at all until the window size is changed
to trigger a reflow. Dbaron suggested backing out his fix for bug 156522 and
that fixes the problem.

Comment 1

[Johnny Stenback (:jst)]

Attachment: CSS file for testcase

Comment 2

[Johnny Stenback (:jst)]

Attachment: testcasae

Load this in a window small enough that it needs scrollbars and this won't
show, load in a window large enough that you don't need the scrollbars and
it'll work just fine.

Comment 3

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

This is fixed by heikki's patch in nsXMLDocument.cpp, which was checked in only
to the branch but not the trunk:

----------------------------
revision 1.149.2.4
date: 2002/05/18 00:03:39;  author: heikki%netscape.com;  state: Exp;  lines: +1 -0
branches:  1.149.2.4.2;
Bug 81546, workaround to make XHTML documents with forms to display. r=harishd,
sr=jst, a=ADT,chofmann,brendan
----------------------------

Should we consider checking this patch into the trunk pending further investigation?

Comment 4

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

The patch in question is attachment 71687 [details] [diff] [review].

Comment 5

[Johnny Stenback (:jst)]

I'd argue that this is not "fixed" by Heikki's change, the problem might be
worked around by that change, but there's still a problem...

Comment 6

[rbs]

*** Bug 157409 has been marked as a duplicate of this bug. ***

Comment 7

[José Jeria]

Go to:
http://212.181.21.47/xml/records.xml
and you will be able to reproduce this very easily. The content is shown after 
a resize. 

And reloading the page makes the scrollbars not to work, only resizing again 
makes them work.

Comment 8

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

A few more details about why this happens:

In the old world (before bug 156522), the AttributeChanged on the scrollbar
would trigger XUL layout logic that would handle the damage without posting a
new reflow command.  (I'm not quite sure how it does that, but box has its
mysterious ways sometimes.  Perhaps it was synchronous.)

In any case, after bug 156522, moving the slider causes a reflow command to be
posted during the initial reflow, which is not the typical case, although I
suspect it does seem to happen in other cases (witness bug 81546).

In other words, my change means that, if a scrolllbar is present, we're now
posting a reflow command in the middle of the initial reflow, where we weren't
doing so before.

If there's a reflow command posted, then PresShell::UnsuppressPainting won't
actually unsupress -- instead it will set mShouldUnsuppressPainting, which
indicates that we *should* unsuppress after the next call to
ProcessReflowCommands.  However, ProcessReflowCommands is never called to
process any reflows that were queued during the initial reflow.

Comment 9

[Axel Hecht]

*** Bug 157611 has been marked as a duplicate of this bug. ***

Comment 10

[Boris Zbarsky [:bzbarsky]]

So it sounds like the real problem is that reflow commands that were queued up
during the initial reflow are never processed....

ProcessReflowCommands is called from the following places:

1)  FlushPendingNotifications()
2)  ReflowEvent::HandleEvent(), but only if the shell is not batching reflows
    (that is, always except in editor, pretty much).

That's it.

It seems that the "right fix" is that at the end of InitialReflow we want to
post a reflow event if the reflow queue is not empty.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Fixes this bug and bug 81546 (tested on Johnny's testcase, on the url in bug
157611, using about:blank as default homepage, URL in comment 7, the testcases
in bug 81546).	This code is basically the code that ProcessReflowCommands
executes if it gets interrupted while the reflow command queue is not empty.

Thoughts?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Also, see bug 81546 comment 63.  The reason this is not a problem for HTML is
that InitialReflow for HTML is called before all the content is in place.  So
more reflows happen in HTML documents as content is inserted into the content
model and reflow commands are placed in the queue (AppendReflowCommand posts a
reflow event).

Comment 13

[rbs]

That's the left-over that PresShell::DidCauseReflow() is supposed to cleanup in 
its call to |FlushPendingNotifications| before returning, but the |if| there is 
preventing it from doing it sometimes. Seems that it is DidCauseReflow() that 
might need some care.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Bang on DidCauseReflow instead

Yup.  You're right.  So it seems to me that the correct behavior here is to
_always_ post a reflow event in DidCauseReflow (unless we're doing a
synchronous flush due to gAsyncReflowDuringDocLoad being false). 
PostReflowEvent() already hadles things like an event already being posted, the
queue being empty, etc, so we don't need to worry about that.

I can think of no reason to restrict the posting to the cases when
mDocumentLoading is true, though I cannot offhand think of a way to set up a
situation in which one of the other callers of DidCauseReflow() will leave
stuff in the queue without posting a reflow event....

Comment 15

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

Comment on attachment 91690 [details] [diff] [review]
Bang on DidCauseReflow instead

r=dbaron, although I admit not understanding the logic around
gAsyncReflowDuringDocLoad, etc.

Comment 16

[rbs]

Comment on attachment 91690 [details] [diff] [review]
Bang on DidCauseReflow instead

sr=rbs

(On a different note, now that <iframe> & friends are being handled in the
content, I wonder if the dummy request is of any use. Might be worth
investigating at some stage if it can be removed since it may be adding undue
complications to the code now.)

Comment 17

[Johnny Stenback (:jst)]

The dummy request is still needed for plugins, and maybe for images too.

Comment 18

[Johnny Stenback (:jst)]

Oh, and for <frame> elements too, they still load their data from the frame code...

Comment 19

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

Does this patch fix bug 157487?

Comment 20

[rbs]

Yes.

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Does this happen on the branch too? If so, we should get this in there and
remove the hack in nsXMLDocument::EndLoad().

PS. Thanks for getting this sucker fixed!

Comment 22

[Boris Zbarsky [:bzbarsky]]

May as well take this.  ;)

Comment 23

[Scott Collins]

Comment on attachment 91690 [details] [diff] [review]
Bang on DidCauseReflow instead

a=scc for checkin to the mozilla trunk

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in on the trunk.

Comment 25

[Jaime Rodriguez, Jr.]

amar: when verifying this as fixed, pls also check to see if boris's checkin
resolved bug 157487. if it does, pls mark bug 157487 as wfm, with a comment that
this bug 156985 resolved it. then, pls add a comment to bug 156522, that bug
157487 no longer blocks bug 156522.

Comment 26

[chris hofmann]

a=chofmann for 1.0.1  add fixed1.0.1 keyword after checking into the branch

Comment 27

[Jaime Rodriguez, Jr.]

chofmann: from my conversation with Kevin, i understood that the patch for bug
156522 will include the needed attachments from the various dependent bugs. do
we need this patch applied to the branch? 

kevin: if we need this one on the branch too (to support bug 156522), pls
nominate it by adding adt1.0.1 to the status whiteboard. thanks!

Comment 28

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in to the 1.0 branch, then replace
"mozilla1.0.1+" with "fixed1.0.1". thanks!

Comment 29

[Kevin McCluskey (gone)]

checked in attachment 91690 [details] [diff] [review] to the 1.0 branch.

Comment 30

[Boris Zbarsky [:bzbarsky]]

*** Bug 158541 has been marked as a duplicate of this bug. ***

Comment 31

[Amarendra Hanumanula]

 Works fine with todays branch and trunk builds: 2002-07-22-08-1.0 and 
2002-07-08-trunk. The given XML testcase shows up first time when I load it in a 
small window.

Comment 32

[Jaime Rodriguez, Jr.]

Replacing "fixed1.0.1" with "verfied1.0.1" per Comment #31 From Amarendra
Hanumanula.