Closed Bug 244055 Opened 20 years ago Closed 18 years ago

Page Layout for editor

Categories

(Core :: Layout, enhancement)

x86
Windows 2000
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: atremon, Assigned: atremon)

References

Details

Attachments

(2 files, 12 obsolete files)

1.95 KB, application/x-xpinstall
Details
47.23 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040519
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040519

I would like a page layout for the editor.

Reproducible: Always
Steps to Reproduce:
I don't know what you mean, or why it's a layout bug.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → INVALID
Attached patch Some work I did for page layout (obsolete) — Splinter Review
Some people would like a page layout while editing because some html pages are
bound to be printed.
This is just get some advice: am I working in the right direction?
I 'll just add a comment next to explain what I did.
Attached file Addon for composer
Adds a button to composer to set page mode while editing
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attachment id=148857

  To get page layout while editing we have to create a new context besides the
existing ones: nsGalleyContext, nsPrintContext and nsPrintPreviewContext that
inherit nsPresContext.
So nsPageLayout sets mPaginated member (defined in nsPresContext) to true, and
implements nsIPageContext, which is defined like nsIPrintPreviewContext.

  When a new nsIDocumentViewer is created, InitInternal is called on the
nsDocumentViewerImpl object which creates a nsGalleyContext. I added a
mIsPageMode to create a nsPageContext instead if it is set to true. That member
can be set by the SetPageMode method defined in nsIContentViewer.
SetPageMode does the same as ReturnToGalleyPresentation() (kill old presentation
shell and context, and call InitInternal() followed by Show() ).

  As the nsPageContext tells it is paginated,
nsCSSFrameContructor::ContructRootFrame will create a nsSimplePageSequence and
not a CanvasFrame (which is created when holding a GalleyContext). This method
is called by nsPresShell::InitialReflow(), which is called by
DocumentViewerImpl::InitPresentationStuff(), which is called by InitInternal().

  Now the reflow process has to be modified: nsSimplePageSequence, which usually
accepts only initial reflows, must accept reflows other than resize, when the
context is a page context.
The modification for nsBoxFrame::IsInitialReflowForPrintPreview() is to use the
same trick used when doing a print preview for nsGfxScrollFrameInner::LayoutBox().
In nsPageFrame, we want to paint the background like when we are print
previewing, but I didn't get the print settings yet.
For nsPageContentFrame, it must accept incremental reflows when holding a page
layout.
You'll probably need to fix pagination too.  I believe at the moment the
assumption is that once something doesn't fit on a page we never have to worry
about it fitting (there are no provisions for getting rid of the continuation
frames).  In a dynamic environment, that assumption will obviously be broken.
Assignee: nobody → atremon
Status: REOPENED → NEW
Yeah. When a block has laid out all its lines, and they all fit into the
available height, it needs to try pulling lines from its next-in-flow (if any).

Alexandre, can you explain why nsPageContext needs to be different from
nsPrintPreviewContext?
Another problem for pagination in a dynamic environment is that I think
currently we have no reflow type to handle the situation where the
availableHeight of a frame has changed. (As would happen when a block gets moved
up or down the page.) Probably Resize-reflow should be overloaded to handle that
case.

The good news is that all of these problems have to be solved to make columns
work well, anyway :-).
> Alexandre, can you explain why nsPageContext needs to be different from
> nsPrintPreviewContext?

The first reason is that I anticipated there would be differences between the
two contexts.

The 2nd reason (stupid reason but added to the first solved any of my
hesitations) is that when we change layout (through a call to the
DocumentViewer::InitInternal method), we lose editor mode (I didn't try to find
out why), and when in print preview context, we can't go back to editor mode (I
didn't try to find out why either).

The difference between page context and print preview context then proved to be
when in print preview context, we refuse all incremental reflows.

Ok, so I get on on this !
I was wrong about blocks not pulling lines from their next-in-flows. There is
code to do that. But I can't think of a situation where this code would actually
be executed, currently...

(In reply to comment #8)
> > Alexandre, can you explain why nsPageContext needs to be different from
> > nsPrintPreviewContext?
> 
> The first reason is that I anticipated there would be differences between the
> two contexts.
> 
> The 2nd reason (stupid reason but added to the first solved any of my
> hesitations) is that when we change layout (through a call to the
> DocumentViewer::InitInternal method), we lose editor mode (I didn't try to
> find out why), and when in print preview context, we can't go back to editor
> mode (I didn't try to find out why either).
> 
> The difference between page context and print preview context then proved to
> be when in print preview context, we refuse all incremental reflows.

OK.

> Ok, so I get on on this !

Sounds good.

A few more comments:

Don't use std::string in code that you eventually want to check in.

It's quite bad how layout code keeps checking for nsIPrintPreviewContext,
nsIPrintContext, and now nsIPageContext. It's also bad when we have code like this:
// If we are printing (constrained height)
since with your work, and soon with columns, we'll have constrained heights
without printing. Layout code should just be calling methods in nsIPresContext
to get exactly the information it wants. For example, we should add to
nsIPresContext something like this:

  // Returns PR_TRUE if this is a dynamic document (scripts run, editing and
  // DOM changes are allowed)
  PRBool IsDynamic();

which would be true for page and galley contexts but not print or print preview
contexts. Instead of adding extra checks for nsIPageContext maybe you could
introduce such new methods on nsIPresContext instead.
(In reply to comment #9)
> I was wrong about blocks not pulling lines from their next-in-flows. There is
> code to do that. But I can't think of a situation where this code would actually
> be executed, currently...

That's exactly what I was wondering: would be useful for page layout, but why
does it already exist ?!

> Don't use std::string in code that you eventually want to check in.

That's just stupid code for myself that I didn't take out of that patch, but it
isn't supposed to be checked in ;-)

> 
> It's quite bad how layout code keeps checking for nsIPrintPreviewContext,
> nsIPrintContext, and now nsIPageContext. It's also bad when we have code like
this:
> // If we are printing (constrained height)

I didn't like that 'if I am a print previewing then ...' either!
I definitely agree ...
> why does it already exist ?!

I don't know. Looks like it's been there since near the beginning of time. It
might possibly be called during incremental reflows that balance tables, which
happen even during printing.
Attached patch Some more work (obsolete) — Splinter Review
I worked on the reflow process to add features missing for a editable page
layout.
So I didn't yet change the PresContext to add a 'IsDynamic' method, I still use
QueryInterface to know if I am in page layout context.

1. When adding lines to a page, the lines reaching the bottom wouldn't jump to
next page.
(nsBlockFrame.cpp, l. 2145) When sliding lines, we have to check whether we
reached the bottom of the block, and if so, push the line. We should check if
the context is paginated (or if the block has constrained height).

2. When typing text in page 1, everything would disappear on page 2 if I left
the line pulling from next-in-flow.
The reason for this is that every time a page (actually, a block) is reflowed,
it tries pulling the 1st line from its next-in-flow. If it can't, the line is
marked as overflow, and when the next page is reflowed, it adds the overflowed
lines in front of its lines. Then all next lines will have to slide by deltaY
with deltaY = line->mBounds.YMost() - oldYMost; (line 2140 of
nsBlockFrame.cpp), line being the line added in front. As oldYMost is the YMost
of the line on the previous page, deltaY gets a meaningless value.

To solve this, when pushing a line, I mark the first line of next-in-flow as
dirty. (nsBlockFrame.cpp, l.4253) This is also useful when pushing a line
because of a new line added (see 1).

3. We have to mark the AreaFrame in the next page as dirty if lines have been
pushed. (nsBlockFrame.cpp, l.2114).

4. When typing text on another page than the 1st one, the page doesn't get
reflowed, because we have no reflowCommand in the reflowPath.
Added PrepareResizeReflow (nsBlockFrame.cpp, l.762).

We also don't see the cursor blinking, because of
nsCaret::GetViewForRendering() computing a wrong clipRect (or viewOffset ?)

5. We don't want to create a new page if we are in an incremental reflow or
dirty (nsPageFrame l.154)

6. Assertions modified in nsSimplePageSequence (l.452) & nsPageContentFrame
(l.126)
Attachment #148857 - Attachment is obsolete: true
Attached patch Wrong file ... (obsolete) — Splinter Review
Attachment #150949 - Attachment is obsolete: true
For point 2, it's somewhat of an artifact. Maybe it would be better to add an
attribute 'mOldBock', to see if we can use the oldYMost or not.

> To solve this, when pushing a line, I mark the first line of next-in-flow as
> dirty. (nsBlockFrame.cpp, l.4253) This is also useful when pushing a line
> because of a new line added (see 1).

Here it is not the AreaFrame that we mark dirty, but rather the last line of
AreaFrame:

> 3. We have to mark the AreaFrame in the next page as dirty if lines have been
> pushed. (nsBlockFrame.cpp, l.2114).

(4) It's not really this: the block inside the AreaFrame on page 2 is pulled by
page 1, so it's the DrainOverflowLines that's important, not the
PrepareResizeReflow.
There's also something else, because the reflow is wrong when typing on page > 2

> 4. When typing text on another page than the 1st one, the page doesn't get
> reflowed, because we have no reflowCommand in the reflowPath.
> Added PrepareResizeReflow (nsBlockFrame.cpp, l.762).
(In reply to comment #14)
> This sounds fantastic!

Hope it will prove to be also ! ;-)
Thanks!
How is this going? I'm working on columns and I need to fix some of these bugs
for that too...
(In reply to comment #17)
> How is this going? I'm working on columns and I need to fix some of these bugs
> for that too...

I was working on having the cursor displayed when clicking on page >2, but I
didn't do anything actually, so go ahead, and I'll merge my work against it.
Attached patch updated patch (obsolete) — Splinter Review
Updated patch following Robert's work on the columns: we do not need the
changes in nsBlockFrame.
Modified the computing of the clip in nsCaret::GetViewForRendering so as to
have the caret blinking on the pages following page 1.

To do: get the print settings, add a pseudo-rule like "moz-viewport" to set the
background color in ua.css, because the @media is screen for page layout.
Attachment #150950 - Attachment is obsolete: true
Attachment #157602 - Attachment is obsolete: true
It's looking nicer now that the nsPresContextType is there.
Attachment #157621 - Attachment is obsolete: true
Comment on attachment 157789 [details] [diff] [review]
added print settings / print device context page size

Hi Robert,

I thought maybe I could try to get this one checked in even if it is not (yet,
I hope) complete, so that I can continue with new clean patches.
Attachment #157789 - Flags: review?(roc)
Attachment #157789 - Flags: review+ → review?(roc)
I'd like a new method on nsPresContext to cover some cases where you're testing
the presentation type
-- IsInteractive() (does this presentation support dynamic content changes,
scripting, and user input)

Shouldn't page layout mode get the print style medium?

In future it would help if you can make patches using -utp6 so we get more
context and the name of the enclosing C function in each hunk. Thanks!
Comment on attachment 157789 [details] [diff] [review]
added print settings / print device context page size

Minusing pending comments
Attachment #157789 - Flags: review?(roc) → review-
> Shouldn't page layout mode get the print style medium?

I set the mode to screen because otherwise I don't get the IBeam cursor when
hovering over text content.
In ua.css, for @media print, "cursor: default !important;" is defined. I can't
find a way to force an IBeam cursor when in page layout, overriding this rule.
Perhaps we ought to have another way to have rules conditional on interactivity,
perhaps a separate UA sheet that gets applied to interactive documents. dbaron,
what do you think?
Alexandre, could you post a query to www-style asking what media type we should
use for interactive paged documents? See
http://www.w3.org/TR/REC-CSS2/media.html
I think none of the media in the list are suitable.
The idea is to use the reflow path we get when incremental reflowing, and
detecting in nsAreaFrame, if the parent is a pageContentFrame, whether lines
overflow or not.
if lines overflow, then we need to reflow the next page, otherwise not.
Unfortunately, I have to have a look at this because this criteria does not
seem ok.
My columns patch solves this problem. Hopefully it will be checked in soon; once
it is checked in, you can copy the approach that nsColumnSetFrame uses.
Attached patch Updated patch for 1.8.b1 (obsolete) — Splinter Review
The september 2004 patch from robert for columns solves the problem of
reflowing next pages (actually, I don't remember exactly what the problem was I
tried to solve with patch 160514)

The problem is when typing 2 consecutive spaces or more on a line, it
dissapears and reappears when moving the cursor around.

When typing long lines without space or CR, the line is clipped (like in
fixed-width tables).

Still the problem of the media (screen or print?) => to have a Ibeam cursor
instead of the arrow.
Attachment #157789 - Attachment is obsolete: true
Attachment #160514 - Attachment is obsolete: true
When a line has 2 consecutive spaces MeasureText puts the TEXT_WAS_TRANSFORMED
flag because nsTextTransformer::GetNextWord returns wasTransformed=true. This
is because a word starts with a space as there are 2 consecutive spaces.
Because of the TEXT_WAS_TRANSFORMED PaintUnicodeText is called instead of
PaintAsciiText.
Attachment #183368 - Flags: review?(roc)
Alexandre, I'm going to save this until 1.9 opens, OK?
OK, Robert!

1.9's open, in case you didn't know.
Yeah, we should probably think about landing this.
One thing I am looking at: I don't get the dark grey background behind the pages.
nsPageFrame:DrawBackground calls nsCSSRendering::PaintBackground which calls FindBackground that returns true because it finds, for example, a nsTextControlFrame, with its white background.
Attachment #183368 - Attachment is obsolete: true
Attachment #183478 - Attachment is obsolete: true
Attachment #183368 - Flags: review?(roc)
Comment on attachment 206927 [details] [diff] [review]
updated for version corresponfing to firefox 1.5

Robert, do you think this would be ok for checking in?
Attachment #206927 - Flags: review?(roc)
> One thing I am looking at: I don't get the dark grey background behind the
> pages.

Seems that in firefox 1.5 print preview has a white background...

Attachment #206927 - Flags: review?(roc)
Attachment #206927 - Flags: review?(roc)
Attachment #206927 - Flags: review?(roc)
Attachment #206927 - Flags: review?(roc)
First of all, sorry about being slack about reviewing this.

+  void SetPageMode(in PRBool aPageMode, in nsIPrintSettings aPrintSettings);

should start lowercase. How about calling it setPaginated? How about dropping aPageMode and just passing nsnull to aPrintSettings to turn off pagination? This could use a better comment.

-      bounds += drawViewOffset;   // offset to coords of returned view
+      //bounds += drawViewOffset;   // offset to coords of returned view
+      nsIView* firstChildView = scrollableView->View()->GetFirstChild();
+      bounds += firstChildView->GetPosition();

What bug does this fix?

+  PRBool mIsPageMode;

might as well make it PRPackedBool

+        if(mIsPageMode) {
+        }

Put a comment explaining where you will (did) create it

If you do SetPageMode(PR_FALSE, ...), then it seems to do new nsPresContext(nsPresContext::eContext_PageLayout) and then that gets overwritten in InitInternal. That doesn't seem right, should we be exiting SetPageMode early when !aPageMode?

+#include "nsIDeviceContextSpecFactory.h"
+#include "nsIDeviceContextSpec.h"
+#include "nsGfxCIID.h"
+static NS_DEFINE_IID(kDeviceContextSpecFactoryCID, NS_DEVICE_CONTEXT_SPEC_FACTORY_CID);

These should go up top.

This creates a mode where the dimensions are fixed to the paper dimensions. Suppose I wanted a mode where the window represented a page, how would I extend this to do that? (Not your problem, I'm just interested)

+    if (!IsDynamic()) {
+      mImageAnimationMode = imgIContainer::kDontAnimMode;
+      mNeverAnimate = PR_TRUE;
+    }

I think you should have the animation mode setting controlled entirely by IsDynamic(), outside of the prescontext type test.

+  if (mMedium == nsLayoutAtoms::print && !IsDynamic())

Here too, you should just test IsDynamic().

I think we should just remove HasPaginatedScrolling and SetPaginatedScrolling. Instead of calling it from nsCSSFrameConstructor, we should just not create the root scrollbar when the the prescontext type is genuine printing.

     // If frame is complete and has a next-in-flow, we need to delete
     // them now. Do not do this when a break-before is signaled because
     // the frame is going to get reflowed again (and may end up wanting
     // a next-in-flow where it ends up), unless it is an out of flow frame.
-    if (NS_FRAME_IS_COMPLETE(aFrameReflowStatus)) {
+    if (NS_FRAME_IS_COMPLETE(aFrameReflowStatus) && !mPresContext->IsDynamic()) {

I don't think this is correct. Why do you need it? This would break columns, I think.

   // If the reflow was successful and the child frame is complete, delete any
   // next-in-flows
-  if (NS_SUCCEEDED(result) && NS_FRAME_IS_COMPLETE(aStatus)) {
+  if (NS_SUCCEEDED(result) && NS_FRAME_IS_COMPLETE(aStatus) && aPresContext->Type() != nsPresContext::eContext_PageLayout) {

Ditto.

-  if (eReflowReason_Incremental != aReflowState.reason) {
+  PRBool isDynamic = aPresContext->IsDynamic();
+  if (eReflowReason_Incremental != aReflowState.reason || isDynamic) {

Why don't we just do it always? Does it break real printing somehow?

+  if (eReflowReason_Incremental != aReflowState.reason ||
+    aPresContext->IsDynamic()) {

Ditto

+  if (eReflowReason_Incremental != aReflowState.reason ||
+    aPresContext->IsDynamic()) {

Ditto. Why don't we just allow incremental reflow for actual printouts? What would be broken that isn't broken for Page_Layout mode? It would be great to keep things consistent, and removing this conditional would actually make printing more consistent with normal reflow too.

+    if (contentPage && mPrevInFlow && eReflowReason_Incremental != aReflowState.reason
+      && eReflowReason_Dirty != aReflowState.reason) {

So we don't create new pages during incremental or dirty reflows... That seems wrong. Doesn't that mean that if you edit the last page and run out of room you won't get a new page added? I suspect page frames need to be overhauled to use an overflow list and work the way blocks and other frames with continuations do.

+    if (aPresContext->Type() == nsPresContext::eContext_PrintPreview ||
+      aPresContext->IsDynamic()) {
       // fill page with White

How about rewriting this to just check that the medium is print? I think that better reflects the situation here: we don't want to fill with white if our destination is paper.

I think that we should never really need to check aPresContext->Type(). Everything you might want to know is one of IsPaginated(), IsDynamic(), and whether the medium is 'print'. Maybe we should have a helper function for the latter, say "IsScreen()". Maybe we need an extra helper function "HasPageBorders()" to express that we want to draw the special print preview/page layout page borders --- I can imagine paginated screen layouts where you don't want them.

-      if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {
+      if (aPresContext->Type() == nsPresContext::eContext_PrintPreview
+        || aPresContext->IsDynamic()) {
         mPD->mPrintSettings = aPresContext->GetPrintSettings();

as before, why do we need to do this differently for printing?

+    //NS_ASSERTION(mPD->mPrintSettings, "Must have a good PrintSettings here!");

What does it mean to not have a printsettings?

   // absolutely ignore all other types of reflows
   // we only want to have done the Initial Reflow
-  if (eReflowReason_Resize == aReflowState.reason ||
+  PRBool isDynamic = aPresContext->IsDynamic();
+  if (eReflowReason_Resize == aReflowState.reason || (!isDynamic && (

As before, if we're going to support these frames in dynamic paginated mode, doing non-initial reflows, then we may as well do it for printing as well.

+  if (isPrintPreview || isDynamic) {

You could just use my proposed "HasPageBorders()" here.

   if (pageSize != adjSize &&
-      (adjSize.x != 0 || adjSize.y != 0 || adjSize.width != 0 || adjSize.height != 0)) {
+      (adjSize.x != 0 || adjSize.y != 0 || adjSize.width != 0 || adjSize.height != 0)
+      && !isDynamic) {
     suppressLeftMargin   = pageSize.x != adjSize.x || (pageSize.x == adjSize.x && !adjSize.x);
     suppressTopMargin    = pageSize.y != adjSize.y || (pageSize.y == adjSize.y && !adjSize.y);

What is this doing that you don't want to do in dynamic mode?

+  if (!isDynamic && eReflowReason_Incremental == aReflowState.reason) {

As before, I don't think we should skip incremental reflow for printing anymore.

+  if (aIsPaginated && !(aPresContext->Type() == nsPresContext::eContext_PageLayout)) {

Change these to just IsDynamic(), but really we shouldn't be messing with selection this way in nsTextFrame. Selection should be disabled at a higher level; see bug 321433.
Depends on: 321433
> First of all, sorry about being slack about reviewing this.

Actually, I was not sure you received the notifications, because I didn't get them myself. That's why I cancelled review 2 or 3 times. Last time I saw my email address was in a 'skip' (or something) section. How do I change this?

> 
> -      bounds += drawViewOffset;   // offset to coords of returned view
> +      //bounds += drawViewOffset;   // offset to coords of returned view
> +      nsIView* firstChildView = scrollableView->View()->GetFirstChild();
> +      bounds += firstChildView->GetPosition();
> 
> What bug does this fix?

The caret wasn't blinking on pages # > 1, but it introduces a new problem while browsing: the caret doesn't blink in <input> or <textarea>.
 
> If you do SetPageMode(PR_FALSE, ...), then it seems to do new
> nsPresContext(nsPresContext::eContext_PageLayout) and then that gets
> overwritten in InitInternal. That doesn't seem right, should we be exiting
> SetPageMode early when !aPageMode?

I pass PR_FALSE to 'aDoCreation' so I don't think I overwrite 'mPresContext', do I?

> +    if (contentPage && mPrevInFlow && eReflowReason_Incremental !=
> aReflowState.reason
> +      && eReflowReason_Dirty != aReflowState.reason) {
>
> So we don't create new pages during incremental or dirty reflows... That seems
> wrong. Doesn't that mean that if you edit the last page and run out of room you
> won't get a new page added? I suspect page frames need to be overhauled to use
> an overflow list and work the way blocks and other frames with continuations
do.

When the last frame is not complete, then a new page is created in 'if (NS_FRAME_IS_COMPLETE(status)) {....} else if (nsnull == kidNextInFlow) {'
in nsSimplePageSequence. The reflow reason is set to 'reflowReason = eReflowReason_Initial', so in nsPageFrame, the code corresponding to the creation of a new frame is executed. While printing or printpreviewing, the reflow reason is always eReflowReason_Initial.

As for the issue of skipping incremental reflow for printing, I left the tests because they were there. You are right that it might be removed, because while printing or printpreviewing, the reflow reason is never 'incremental' at these points.

I'm going to have a look for the special case I put in nsTextFrame for selection.
Something else is avoid reflowing all the pages if nothing overflows.
(In reply to comment #41)
> Actually, I was not sure you received the notifications, because I didn't get
> them myself. That's why I cancelled review 2 or 3 times. Last time I saw my
> email address was in a 'skip' (or something) section. How do I change this?

Bugzilla prefs? You can always just send me email directly.

> > -      bounds += drawViewOffset;   // offset to coords of returned view
> > +      //bounds += drawViewOffset;   // offset to coords of returned view
> > +      nsIView* firstChildView = scrollableView->View()->GetFirstChild();
> > +      bounds += firstChildView->GetPosition();
> > 
> > What bug does this fix?
> 
> The caret wasn't blinking on pages # > 1, but it introduces a new problem
> while browsing: the caret doesn't blink in <input> or <textarea>.

We're going to redo caret very soon in a way that will hopefully make this much more rational. I suggest you don't worry about that issue now and just leave the caret code alone.

> > If you do SetPageMode(PR_FALSE, ...), then it seems to do new
> > nsPresContext(nsPresContext::eContext_PageLayout) and then that gets
> > overwritten in InitInternal. That doesn't seem right, should we be exiting
> > SetPageMode early when !aPageMode?
> 
> I pass PR_FALSE to 'aDoCreation' so I don't think I overwrite 'mPresContext',
> do I?

I missed that. However, when aPageMode is false we're still using the paper size and so on. I don't think that's what you want, is it?

> As for the issue of skipping incremental reflow for printing, I left the
> tests because they were there. You are right that it might be removed,
> because while printing or printpreviewing, the reflow reason is never
> 'incremental' at these points.

I do think that if we're going to make dynamic pagination work, the right way to go is to have pages handle incremental reflows. However, dbaron's "reflow branch" changes all this so it might be better to wait on fixing up pages to handle incremental reflows until that has landed. In the meantime I can live with these checks for the sake of stopping printing from breaking, but we should put XXX comments by each one saying they should be removed.
Attached patch Following Robert's comments (obsolete) — Splinter Review
My comments on this will follow
Attachment #206927 - Attachment is obsolete: true
Attachment #206927 - Flags: review?(roc)
https://bugzilla.mozilla.org/attachment.cgi?id=211273&action=diff

* mIsPageMode -> PackedBool
* InitInternal: added comment explaining why not creating new PresContext
* Don't do page size stuff if aPageMode is false in SetPageMode
* '#include...' moved to top of file
* 'mImageAnimationMode = imgIContainer::kDontAnimMode; mNeverAnimate = PR_TRUE;' only if not dynamic.
* test 'if (NS_FRAME_IS_COMPLETE(aFrameReflowStatus) && !mPresContext->IsDynamic())' incorrect in nsBlockReflowContext.cpp and nsContainerFrame.cpp
* nsPageContentFrame::Reflow and nsPageFrame::Reflow -> always allow incremental reflows
* Draw white page if presContext tells us (!IsScreen)
* nsSimplePageSequence: only block resize reflows
* SuppressLeftMargin has to be done also in dynamic mode
* removed fiddling in nsCaret
I'm basically OK with landing this as is. I want to check with dbaron that he's at least okay with the concept.
Comment on attachment 211273 [details] [diff] [review]
Following Robert's comments

Alexandre, you really need to be more aggressive about bugging Robert :-)

On a more serious note, it seems like this bug has died again.  Setting review request so that it doesn't get lost again.
Attachment #211273 - Flags: review?(roc)
I think in comment #45 I meant to r+sr this patch.

Eli, would you mind taking a look at it? If you're still OK with it then Alexandre can land it (I believe he has CVS access now?)
Looks fine to me.  Alexandre, go ahead and check it in with r+sr=roc (assuming you don't have any questions).
Attached patch Merged to tip (obsolete) — Splinter Review
Alex, could you double-check that I merged this correctly?  (I'm pretty sure I got everything right, although I'm not quite sure about the nsPageFrame changes.)  Also, I think it's expected that the caret display is wrong and typing a bunch of text doesn't paginate correctly; is that correct?
Attachment #211273 - Attachment is obsolete: true
Attachment #211273 - Flags: review?(roc)
> Alex, could you double-check that I merged this correctly?  (I'm pretty sure I
> got everything right, although I'm not quite sure about the nsPageFrame
> changes.)

Not sure about the SetShell method:
if (mMedium != nsLayoutAtoms::print
would become:
if (IsDynamic() && docURI) {
wouldn't it?

I would delete the comment in nsSimplePageSequenceFrame:
// absolutely ignore all other types of reflows
// we only want to have done the Initial Reflow

In nsPageFrame::SetInitialChildList, as you delete the 'if', shouldn't you outdent the code that was in the if clause ?

As for nsPageFrame, I have the same changes here.

> Also, I think it's expected that the caret display is wrong and
> typing a bunch of text doesn't paginate correctly; is that correct?

The caret display is wrong, but what do you mean by 'doesn't paginate correctly' ?
When typing text, if a line was added, I would have text at the end of page flow to the next, and if it was on the last page, a page would be added.
(In reply to comment #50)
> > Alex, could you double-check that I merged this correctly?  (I'm pretty sure I
> > got everything right, although I'm not quite sure about the nsPageFrame
> > changes.)
> 
> Not sure about the SetShell method:
> if (mMedium != nsLayoutAtoms::print
> would become:
> if (IsDynamic() && docURI) {
> wouldn't it?

Yeah; thanks for spotting that.

> I would delete the comment in nsSimplePageSequenceFrame:
> // absolutely ignore all other types of reflows
> // we only want to have done the Initial Reflow

I did, did you mistype?

> 
> In nsPageFrame::SetInitialChildList, as you delete the 'if', shouldn't you
> outdent the code that was in the if clause ?
 
Yeah, I should.

> As for nsPageFrame, I have the same changes here.
> 
> > Also, I think it's expected that the caret display is wrong and
> > typing a bunch of text doesn't paginate correctly; is that correct?
> 
> The caret display is wrong, but what do you mean by 'doesn't paginate
> correctly' ?
> When typing text, if a line was added, I would have text at the end of page
> flow to the next, and if it was on the last page, a page would be added.
> 
I can't get it to add pages; it seems to think there are more pages from the footer, but it doesn't display and it asserts constantly.  I can try to debug it later.
OK, a bit more info: I only hit the issue when at one point there wasn't a horizontal scrollbar.

The issue is with a page layout document that at one point had or currently has no horizontal scrollbar, when I add enough text to get a new page.  I'm getting the assertion "Computed overflow area must contain frame bounds" in nsIFrame::FinishAndStoreOverflow.  The call is from nsHTMLScrollFrame::PlaceScrollArea.  The computed overflow area is somehow shorter than the actual page length when the number of pages increases.  It could reflect some sort of regression, as that code has changed recently. 

roc, any suggestions?
Could nsSimplePageSequenceFrame or one of the other page-associated frames be calculating its overflow area incorrectly?
(In reply to comment #52)
> no horizontal scrollbar, when I add enough text to get a new page.  I'm getting
> the assertion "Computed overflow area must contain frame bounds" in
> nsIFrame::FinishAndStoreOverflow.  The call is from
> nsHTMLScrollFrame::PlaceScrollArea.  The computed overflow area is somehow

Sounds like you have the first patch (attachment 216341 [details] [diff] [review]) to bug 331458 but not the second (attachment 216695 [details] [diff] [review]).
Er, sorry, I was thinking of another assertion.  I'm actually questioning whether I want that one to be valid in this case...
I came up with a fix that appears to get it working properly. At the end of nsSimplePageSequence::Reflow, I added the following lines:

aDesiredSize.mOverflowArea = nsRect(0, 0, aDesiredSize.width,
                                    aDesiredSize.height);

FinishAndStoreOverflow(&aDesiredSize);

It seems to work.  Is it correct?
Attachment #216701 - Attachment is obsolete: true
(In reply to comment #56)
> I came up with a fix that appears to get it working properly. At the end of
> nsSimplePageSequence::Reflow, I added the following lines:
> 
> aDesiredSize.mOverflowArea = nsRect(0, 0, aDesiredSize.width,
>                                     aDesiredSize.height);
> 
> FinishAndStoreOverflow(&aDesiredSize);
> 
> It seems to work.  Is it correct?

If the intent is that this frame clips all its overflow, yes.  If not, then you'd probably want something more like

aDesiredSize.mOverflowArea.UnionRect(aDesiredSize.mOverflowArea,
                                     nsRect( ... code above ...));
Checked in.

dbaron, we clip pages to their boundaries, so it's impossible for there to be overflow.
Status: NEW → RESOLVED
Closed: 20 years ago18 years ago
Resolution: --- → FIXED
Didn't this need some IID changes?
I noticed that, but I forgot to fix it.  Fix checked in; thanks for noticing.
Doesn't prescontext need a new IID too?
Why does nsPresContext has an IID?  As far as I can tell, it's unused.
Ah, looks like it got left over from the nsIPresContext days.  Wanna nuke it?  rs=bzbarsky.
Blocks: 332701
Depends on: 340564
I backed out the part of this patch that allowed incremental reflow of a page sequence, because tables can't deal with it yet and it was causing test failures... See bug 378480.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: