bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

position:fixed element is missing on the first page in Print Preview when styled with downloadable font

RESOLVED FIXED in mozilla37

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Depends on: 1 bug, {testcase})

Trunk
mozilla37
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

1.42 KB, text/html
Details
10.67 KB, text/html
Details
13.48 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.05 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.56 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.62 KB, text/html
Details
3.20 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.12 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.64 KB, patch
Details | Diff | Splinter Review
Created attachment 8532697 [details]
Testcase #1

Follow-up from bug 423192 comment 10.

(I'm filing this in Layout rather than Print Preview b/c I see that it's
the frame tree that is wrong.)

STR
1. save the attached testcase to a local file
2. load the file
3. Print Preview

ACTUAL RESULT
"This is a header!" is missing on the first page (it's present on subsequent pages).

EXPECTED RESULT
"This is a header!" on all pages.

NOTE
The bug only occurs when the preference gfx.downloadable_fonts.enabled
is true (default).
(Assignee)

Comment 1

4 years ago
Created attachment 8532706 [details]
frame tree

The out-of-flow frame (red) that should've been on the first page has been
placed on the FixedList of the second page for some reason.
(Assignee)

Comment 2

4 years ago
FTR, there are also some assertions - when entering Print Preview:

###!!! ASSERTION: style context has old rule node: 'mRoots[i]->RuleNode()->RuleTree() == mRuleTree', layout/style/nsStyleSet.cpp, line 254
###!!! ASSERTION: old rule tree still referenced: 'Not Reached', layout/style/nsStyleSet.cpp, line 2081

and when closing Print Preview:
###!!! ASSERTION: destroying style context from old rule tree too late: 'styleSet->GetRuleTree() == mRuleNode->RuleTree() || styleSet->IsInRuleTreeReconstruct()', layout/style/nsStyleContext.cpp, line 92
###!!! ASSERTION: destroying style context from old rule tree too late: 'styleSet->GetRuleTree() == mRuleNode->RuleTree() || styleSet->IsInRuleTreeReconstruct()', layout/style/nsStyleContext.cpp, line 92
###!!! ASSERTION: destroying style context from old rule tree too late: 'styleSet->GetRuleTree() == mRuleNode->RuleTree() || styleSet->IsInRuleTreeReconstruct()', layout/style/nsStyleContext.cpp, line 92

The assertions may be a separate issue though.
Keywords: testcase
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
OS: Linux → All
QA Contact: mats
Hardware: x86_64 → All
(Assignee)

Updated

4 years ago
Assignee: nobody → mats
QA Contact: mats
(Assignee)

Comment 3

4 years ago
Created attachment 8533767 [details] [diff] [review]
part 1 - Remove mFixedContainingBlock.  Make GetAbsoluteContainingBlock() check for an ancestor with the right frame type instead.

There are multiple underlying bugs here, I'll describe them one by one.

What happens in Print Preview on the testcase is that the (fixed pos)
element (see the frame dump above) using the remote font gets
a restyle request, which leads to a RecreateFramesForContent call.

When inserting the new frame, we create a nsFrameConstructorState
and in particular call GetAbsoluteContainingBlock which in this case
returns mFixedContainingBlock as the fixed pos. container:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=1ff07cb9adaf#5996
mFixedContainingBlock is assigned when creating a ViewportFrame
or PageContentFrame:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=1ff07cb9adaf#2676
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=1ff07cb9adaf#2953
Thus, when printing, it's the PageContentFrame in the last printed page.
So that's why RecreateFramesForContent inserts the new fixed pos
frame into the last page.

The proposed solution is to remove mFixedContainingBlock as
a state and instead make GetAbsoluteContainingBlock return the
appropriate ancestor frame.
Attachment #8533767 - Flags: review?(roc)
(Assignee)

Comment 4

4 years ago
Created attachment 8533768 [details] [diff] [review]
part 2 - Move incremental reflow hack from nsSimplePageSequenceFrame::Reflow to nsTableOuterFrame. This is to avoid breaking pages that don't even use tables.

So, with the above fix the frame tree is now correct but it
still doesn't render the fixed pos content on the first page.
That's because we have intentionally disabled incremental reflows
in Print Mode ("until we've taught tables how to do it right in
paginated mode"):
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?rev=11fa2110679d#172

I suggest that we move that hack to nsTableOuterFrame::Reflow
instead so that pages that doesn't use tables isn't crippled by it.
Attachment #8533768 - Flags: review?(roc)
(Assignee)

Comment 5

4 years ago
Created attachment 8533770 [details] [diff] [review]
part 3 - Deal with the placeholder being on a different page than the out-of-flow frame.

So, with those two fixes we now get a crash in
layout/base/crashtests/675246-1.xhtml
This testcase creates a frame tree where the placeholder and its
out-of-flow are on different pages (even before the earlier patches),
but we have never reflowed such a frame tree though (because of the
wallpaper removed in part 1).  It's a null-pointer crash in
CalculateHypotheticalBox because we never find the 'cbrs->frame' in
the ancestors on page 2, b/c it's the PageContentFrame on page 1.
I think we can just subtract the offset from the root to 'cbrs->frame'
at this point to get the correct offset.
Attachment #8533770 - Flags: review?(roc)
(Assignee)

Comment 6

4 years ago
Created attachment 8533772 [details]
float-clear-000-print.html frame dumps

And there's another crash in ReflowPushedFloats on
layout/reftests/pagination/float-clear-000-print.html
(Again, a frame tree we haven't reflowed due to the wallpaper removed in
part 1).  Here we have two consecutive (sibling) pushed floats in the
frame list we're iterating, we save the current frame's next-sibling
so we can continue iterating from there, but in this case the
next-sibling is also the next-in-flow and it gets pushed.
Here are some frame dumps to illustrate what happens.
(Assignee)

Comment 7

4 years ago
Created attachment 8533773 [details] [diff] [review]
part 4 - Don't use the current next-sibling as a reference point where to continue the loop, since that frame may be pushed if it's also the next-in-flow.  Instead, use the frame from the last iterati

The fix for that is to keep track of the last iterated frame instead
and use that as the reference point where to continue the iteration.
Attachment #8533773 - Flags: review?(roc)
(Assignee)

Comment 8

4 years ago
Created attachment 8533775 [details] [diff] [review]
part 5 - Don't report a reflow as NS_FRAME_NOT_COMPLETE when it's actually complete.  Because it will give the frame the wrong BSize.

And lastly, there is now a layout error for
layout/reftests/pagination/float-clear-000-print.html
https://tbpl.mozilla.org/?tree=Try&rev=7ac702469208
This is because our reflow of pushed floats isn't stable (i.e. reflowing
it twice yeilds different results).  The problem here is that we change
the reflow status from COMPLETE to NOT_COMPLETE when our next-in-flow
has pushed floats.  This is bad since it affects the height of the
current frame (we end up in the last 'else' block in ComputeFinalSize).
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=1289066eccbe#1548

As the note says, this code is a wallpaper for a columns crash:
http://hg.mozilla.org/mozilla-central/rev/2ed966e4fa58
I'm not sure if it's still needed, but relaxing the condition somewhat
(making the reflow status OVERFLOW_INCOMPLETE when our next-in-flow
is an overflow container) seems safe for now and it fixes the layout
bug.

https://tbpl.mozilla.org/?tree=Try&rev=0a09d78c8196
Attachment #8533775 - Flags: review?(roc)
Comment on attachment 8533768 [details] [diff] [review]
part 2 - Move incremental reflow hack from nsSimplePageSequenceFrame::Reflow to nsTableOuterFrame. This is to avoid breaking pages that don't even use tables.

Review of attachment 8533768 [details] [diff] [review]:
-----------------------------------------------------------------

This is rather scary since it enables incremental reflows in paginated documents for most content other than tables. A lot of those code paths will have been tested by multicol, but some won't, or not well. This is still the right thing to do but we probably should alert our fuzzers.
Attachment #8533768 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> This is rather scary since it enables incremental reflows in paginated
> documents for most content other than tables. A lot of those code paths will
> have been tested by multicol, but some won't, or not well. This is still the
> right thing to do but we probably should alert our fuzzers.

For a while we disabled breaking floats in multicol containers because of incremental reflow bugs. I don't remember if we ever re-enabled that. If we did not, then this change exposes us to those bugs again. Please check.
(Assignee)

Comment 12

4 years ago
It's still disabled (bug 447660) - I responded there.  I think the risk is lower
for printing since incremental reflows are rare in this context.

I can add back the removed chunk in nsSimplePageSequenceFrame behind a pref
if you want -- then we have the option to disable it with a hotfix.  If that
option isn't needed then we can land it as is I think -- it's trivial to add
that chunk back during Beta if we find bugs.
Flags: needinfo?(roc)
(Assignee)

Updated

4 years ago
Blocks: 447660
OK but please stay alert :-)
Flags: needinfo?(roc)

Updated

3 years ago
Depends on: 1152354

Updated

3 years ago
Depends on: 1155230
(Assignee)

Comment 16

3 years ago
Bug 1152354 added back the incremental reflow hack in nsSimplePageSequenceFrame
so I suspect this bug still occurs.  I filed bug 1157012.
Depends on: 1157012

Comment 17

2 years ago
(In reply to Mats Palmgren (:mats) from comment #16)
> Bug 1152354 added back the incremental reflow hack in
> nsSimplePageSequenceFrame
> so I suspect this bug still occurs.  I filed bug 1157012.


Yes, this bug still occurs:  zencellose.zenglobalsolutions.com/datasheets/test3.html you can try to print and see that first page is missing.  Sometimes in preview it is there, sometimes not, but header and footer never makes it to paper on first page.
You need to log in before you can comment on or make changes to this bug.