Closed Bug 377119 Opened 15 years ago Closed 14 years ago

[FIX]printing reftest only shows first page on Mac

Categories

(Core :: Layout, defect, P1)

PowerPC
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: dbaron, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Bug 370439 caused a Mac-only regression with the printing reftest (layout/reftests/printing/272830-1.html).  It only shows the first page when showing the test, but it shows both pages for the reference.
Here's roughly what's happening:

1) During the initial reflow, gfxScrollFrameInner posts a reflow callback to update the scrollbar attributes.
2) After we've finished processing the reflow callback, presshell flushes any new layout notifications.
3) There is a pending restyle of a scrollbar button which didn't have frames but now will. I believe this is because the scrollbar XBL constructor has run which calls initScrollbars
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/scrollbar.xml#40
which changes the visible buttons from the standard Windows layout to the Mac layout. I'm not sure when the XBL constructor ran or why this restyle wasn't flushed before the initial reflow (Boris?).
4) The scrollbar's nsBoxFrame::InsertFrames asks for the scrollbar frame to be reflowed again:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBoxFrame.cpp#1038
5) The scrollbar is a reflow root, but its parent isn't, so we have to reflow the document itself to handle this.
http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3409
6) We reflow the document again. But tables don't know how to do incremental reflow in the context of pagination, so the table continuation on the second page is basically just deleted. Oops.

I think steps 4+5 are wrong --- we shouldn't need to reflow the whole document given that the scrollbar is a reflow root and we're just messing with its children. But I'm not sure which of step 4 or 5 is wrong in this case. David?

I think step 3 is also a problem. Constructing a frame for the up-top scrollbar button and then later deleting the frame and constructing a new frame for the bottom-up button is stupid extra overhead on Mac. I guess we don't want to run the XBL constructor before we construct frames for the default children because that would mean running script during frame construction. Is there a way to have the initial hidden attributes be set differently for Mac, short of cloning scrollbar.xml? If not, maybe we should just do that.
We could probably also fix this bug by ensuring that after frame construction but before reflow, all XBL constructors have run and all their pending restyles have been flushed.
We could also fix this by fixing table incremental reflow in paginated contexts, of course, but that's not something I want anyone to do for 1.9.
Wouldn't this test also fail on Linux in a case like bug 263444? The commented out throw in scrollbar.xml is a bit misleading.
(In reply to comment #2)
> We could probably also fix this bug by ensuring that after frame construction
> but before reflow, all XBL constructors have run and all their pending restyles
> have been flushed.

So, adding something like 

mDocument->BeginUpdate(UPDATE_ALL);
mDocument->EndUpdate(UPDATE_ALL);

to nsPresShell::InitialReflow()?  That could work.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsGfxScrollFrame.cpp&rev=3.301&mark=434-435#434 might have something to do with this problem; could someone try removing that bit of code and seeing if it fixes the problem?  (It's a shot in the dark, but it could work... the justification is that the page sequence returns early from reflow if it isn't dirty.)

(I can't do any coding at the moment because my hard drive failed.  I should have that resolved by this weekend.)
> I'm not sure when the XBL constructor ran or why this restyle wasn't flushed
> before the initial reflow (Boris?).

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&rev=3.984&mark=2627,2628#2627

It do think it might make sense to move that to right after we construct the frame tree, so make the logic in InitialReflow something like:

1)  Construct the frame tree
2)  Process XBL constructors
4)  Make sure we're not dead
5)  Flush pending restyles
6)  Probably make sure we're not dead again, since step 5 can run XBL ctors too
7)  Do the actual reflow

sicking, what do you think?

> 5) The scrollbar is a reflow root, but its parent isn't

I'd think that for a eTreeChange targeted at a reflow root we could just reflow the reflow root, since eTreeChange means something _under_ the root changed, right?  dbaron would know for sure.
Attachment #261285 - Flags: superreview?(jonas)
Attachment #261285 - Flags: review?(jonas)
Assignee: cbarrett → nobody
Component: Widget: Cocoa → Layout
Flags: blocking1.9+
QA Contact: cocoa → layout
Filed bug 378784 on steps 4 and 5 from comment 1.
Assignee: nobody → bzbarsky
Priority: -- → P1
Summary: printing reftest only shows first page on Mac → [FIX]printing reftest only shows first page on Mac
Target Milestone: --- → mozilla1.9alpha4
Target Milestone: mozilla1.9alpha4 → mozilla1.9alpha5
Comment on attachment 261285 [details] [diff] [review]
The frame constructor changes per comment 6

r/sr=me. Smaugs patch is going to fix the shell iteration, right?
Attachment #261285 - Flags: superreview?(jonas)
Attachment #261285 - Flags: superreview+
Attachment #261285 - Flags: review?(jonas)
Attachment #261285 - Flags: review+
Comment on attachment 261285 [details] [diff] [review]
The frame constructor changes per comment 6

>+    // Now reget the root frame, since all that script might have
>+    // affected it somehow.

That's impossible; the viewport frame gets created once, in InitialReflow, and doesn't get destroyed until the presshell get destroyed.

>+  // XXXbz do we need the extra grip and the NS_ENSURE_STATE?  Or is
>+  // aPO->mPresShell guaranteed to not go away?

aPO->mPresShell is guaranteed not to go away; all the print objects are owned by the print engine, and the print engine can't get destroyed while the presentation is being created.
> That's impossible; the viewport frame gets created once,

I'd really rather not start relying on that....  I'll document that more clearly.

> aPO->mPresShell is guaranteed not to go away

Great!
Fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
This seems to have  broken the Tab Sidebar extension (sidebar remains empty) on Windows (don't know about Mac or Linux).

Seems to work for me on Linux, just fine.  I would expect it to work everywhere, unless the extension is doing something _really_ screwy.

Peter, do you build?  If so, can you do some experiments for me?
(In reply to comment #15)
> Peter, do you build?  If so, can you do some experiments for me?
No I don't build, unfortunately. 

Do you need any particular settings for the extension?  Or does it show up with the vanilla install settings?
It does show up with vanilla settings Boris.
Dave Townsend (Mossop) is going to have a closer look at it later, if he can find the time. 
The issue with Tab Sidebar was an extension problem which is fixed now.
Flags: in-testsuite+
Depends on: 452185
You need to log in before you can comment on or make changes to this bug.