If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in mozilla1.1beta

Status

()

Core
Layout
P1
normal
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: jst, Assigned: bz)

Tracking

({regression})

Trunk
mozilla1.1beta
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 RTM] [ETA 07/26], URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Created attachment 91003 [details]
CSS file for testcase
(Reporter)

Comment 2

15 years ago
Created attachment 91004 [details]
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.
(Reporter)

Updated

15 years ago
Keywords: regression
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?
The patch in question is attachment 71687 [details] [diff] [review].
(Reporter)

Comment 5

15 years ago
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...

Updated

15 years ago
QA Contact: petersen → amar

Comment 6

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

Comment 7

15 years ago
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.

Updated

15 years ago
Priority: -- → P2
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → mozilla1.1beta
Blocks: 157611
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

15 years ago
*** Bug 157611 has been marked as a duplicate of this bug. ***
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.
Created attachment 91620 [details] [diff] [review]
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?
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

15 years ago
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.
Created attachment 91690 [details] [diff] [review]
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....
Attachment #91620 - Attachment is obsolete: true
Comment on attachment 91690 [details] [diff] [review]
Bang on DidCauseReflow instead

r=dbaron, although I admit not understanding the logic around
gAsyncReflowDuringDocLoad, etc.
Attachment #91690 - Flags: review+

Comment 16

15 years ago
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.)
Attachment #91690 - Flags: superreview+
(Reporter)

Comment 17

15 years ago
The dummy request is still needed for plugins, and maybe for images too.
(Reporter)

Comment 18

15 years ago
Oh, and for <frame> elements too, they still load their data from the frame code...
Does this patch fix bug 157487?

Comment 20

15 years ago
Yes.
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!
Blocks: 81546, 157487
Whiteboard: approval requested
May as well take this.  ;)
Assignee: dbaron → bzbarsky
Status: ASSIGNED → NEW
Summary: XML document's not properly reflown if scrollbars required → [FIX]XML document's not properly reflown if scrollbars required

Comment 23

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

a=scc for checkin to the mozilla trunk
Attachment #91690 - Flags: approval+
Checked in on the trunk.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Blocks: 156522

Comment 25

15 years ago
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.
Keywords: nsbeta1

Updated

15 years ago
Keywords: mozilla1.0.1

Comment 26

15 years ago
a=chofmann for 1.0.1  add fixed1.0.1 keyword after checking into the branch
Keywords: mozilla1.0.1 → mozilla1.0.1+

Comment 27

15 years ago
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!
Keywords: nsbeta1 → nsbeta1+

Updated

15 years ago
Keywords: adt1.0.1

Comment 28

15 years ago
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!
Keywords: adt1.0.1 → adt1.0.1+
Whiteboard: approval requested → [adt2 RTM] [ETA 07/22]
checked in attachment 91690 [details] [diff] [review] to the 1.0 branch.
Keywords: adt1.0.1+, mozilla1.0.1+ → adt1.0.1, fixed1.0.1
*** Bug 158541 has been marked as a duplicate of this bug. ***

Comment 31

15 years ago
 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.
Status: RESOLVED → VERIFIED

Comment 32

15 years ago
Replacing "fixed1.0.1" with "verfied1.0.1" per Comment #31 From Amarendra
Hanumanula.
Keywords: fixed1.0.1 → verified1.0.1

Updated

15 years ago
Keywords: adt1.0.1 → adt1.0.1+
Whiteboard: [adt2 RTM] [ETA 07/22] → [adt2 RTM] [ETA 07/26]
You need to log in before you can comment on or make changes to this bug.