Closed Bug 1321803 Opened 3 years ago Closed 3 years ago

Content truncated when printing a page with a fixed-height body with overflow:scroll propagated to viewport

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr52 --- fixed
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

Testcase:

  <!DOCTYPE html>
  <body style="overflow: scroll; height: 100px">
    Something
    <div style="height: 2000px"></div>
    Something else
  </body>

In normal layout, the overflow style is propagated to viewport in this case, so the fact that the body has a fixed height doesn't really affect the user-perceived behavior much.  What we should probably do is not clip this case during printing.  The clipping was added in bug 129941.

Real-world site triggering this problem (via "overflow-x: hidden" on the <body>): http://semantic-ui.com/
Depends on: 129941
The basic problem, of course, is that in a paginated context we don't actually propagate scroll to viewport, because there is no viewport...  But it still seems odd to me to apply the overflow style there for the case when we _would_ have propagated to viewport in a non-print context.
We could factor out the "is this the element that would propagate overflow" bit out of GetPropagatedScrollbarStylesForViewport.  The problem is that we don't want to keep calling it in ConstructNonScrollableBlock or in ApplyPaginatedOverflowClipping.

For the former, we could probably solve the problem by using a flag on the fcdata.  For the latter... maybe we could use a flag on the frame?  Flag bit 60 seems to be free for blockframes, but it's the _only_ free bit.
Flags: needinfo?(dbaron)
Note that this bug was filed in response to bug 129941 comment 153.
I don't see why you need a new block frame bit for this; it looks like
it ought to be sufficient to *not* set NS_BLOCK_CLIP_PAGINATED_OVERFLOW
in this case.  (And not set FCDATA_FORCED_NON_SCROLLABLE_BLOCK to make
that happen, I think.)  We want to have a regular block (not a
scrollframe), but we don't want that regular block to do any special
clipping.  So I don't see why you'd need extra calls in
ApplyPaginatedOverflowClipping.

(For my own notes, this in turn makes the code in
nsFrame::ShouldApplyOverflowClipping, which is where the code added to
ApplyPaginatedOverflowClipping in bug 129941 now lives, not clip for the
block.)

Or are you talking about using a new block frame bit to make the check
for whether this is the frame that propagates scrollbar styles to the
viewport faster?

Is that check really expensive enough that it needs something special?
I'd think that if the first test is whether the frame's content node has
no parent (to be followed later by a slower check that it's the document
root) or is an HTML body element ought to be fast enough.
Flags: needinfo?(dbaron) → needinfo?(bzbarsky)
Priority: -- → P3
Oh, that's what I get for reading the code from bug 129941 as it landed, not the code as it is right now.  ;)

You're absolutely right that we just need to leave off NS_BLOCK_CLIP_PAGINATED_OVERFLOW from the framestate.

Given that, we only need to do checking in frame construction, not painting, and we can restrict it to the paginated case.  We can also restrict to <html:body> elements; the root element case is handled differently anyway and we don't apply this clipping behavior in that case.  I'll write a patch.
Flags: needinfo?(bzbarsky)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8818334 [details] [diff] [review]
part 1.  Add an API on nsPresContext to test whether the given element would have its scrollbar styles propagated to the viewport in non-paginated mode

>+bool
>+nsPresContext::ElementWouldPropagateScrollbarStyles(Element* aElement)
>+{
>+  MOZ_ASSERT(IsPaginated(), "Should only be called on paginated contexts");
>+  if (aElement->GetParent() && !aElement->IsHTMLElement(nsGkAtoms::body)) {
>+    // We certaintly won't be propagating from this element.

s/certaintly/certainly/

>+    return false;
>+  }
>+
>+  // Go ahead and just call UpdateViewportScrollbarStylesOverride, but update a

s/UpdateViewportScrollbarStylesOverride/GetPropagatedScrollbarStylesForViewport/


r=dbaron with that
Attachment #8818334 - Flags: review?(dbaron) → review+
Attachment #8818335 - Flags: review?(dbaron) → review+
Comment on attachment 8818336 [details] [diff] [review]
part 3.  Don't claim that we forced a non-scrollable block for <body> elements in a print presentation that would have propagated their scrollbars to the viewport.  We do still want to create a non-scrollable block for them, though

>+      // We don't actually want to do the scrollframe suppression behavior
>+      // (which involves clipping at paint time) for cases when we would have
>+      // propagated to the viewport.

This comment could be a bit clearer, I think.  Perhaps more like:

  If the scrollable frame would have propagated its scrolling to the viewport, we still want to construct a regular block rather than a scrollframe so that it paginates correctly, but we don't want to set the bit on the block that tells it to clip at paint time.

r=dbaron with that

If you didn't already, it might be good to test that the tests fail as expected with both this patch and the patch to bug 129941 disabled.
Attachment #8818336 - Flags: review?(dbaron) → review+
>s/certaintly/certainly/

Fixed.

>s/UpdateViewportScrollbarStylesOverride/GetPropagatedScrollbarStylesForViewport/

Er, yes.  Fixed.

> Perhaps more like:

Done.

> it might be good to test that the tests fail as expected with both this patch and
> the patch to bug 129941 disabled.

Good point.  They do.  Specifically, some of the tests for this bug, some of the tests for bug 129941, and some of the tests for bug 626395 fail.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d052baf061
part 1.  Add an API on nsPresContext to test whether the given element would have its scrollbar styles propagated to the viewport in non-paginated mode.  r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/231bcfd177f8
part 2.  Change existing tests for paginated overflow clipping to use a <div> instead of the <body> as the scrollable thing being tested.  r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/93f9057d4ca0
part 3.  Don't claim that we forced a non-scrollable block for <body> elements in a print presentation that would have propagated their scrollbars to the viewport.  We do still want to create a non-scrollable block for them, though.  r=dbaron
Duplicate of this bug: 1380453
Comment on attachment 8818334 [details] [diff] [review]
part 1.  Add an API on nsPresContext to test whether the given element would have its scrollbar styles propagated to the viewport in non-paginated mode

Requesting consideration for ESR52 approval of these 3 patches coming from duped bug 1380453. Mike Kaply has gotten reports of this bug from enterprise users (a large installation at that), so I think this warrants serious consideration as something affecting our target audience.

I've already confirmed that the patches graft cleanly and bz confirms that they're safe and haven't caused any known issues since they shipped with Fx53.

(In reply to Boris Zbarsky [:bz] from bug 1380453 comment #4)
> Should be fairly safe, actually.  It added a new API and one single caller
> of that API, plus various test bits.  And the API impl should backport
> cleanly to 52 from 53.  Plus we have no known regressions from it.
Attachment #8818334 - Flags: approval-mozilla-esr52?
Comment on attachment 8818334 [details] [diff] [review]
part 1.  Add an API on nsPresContext to test whether the given element would have its scrollbar styles propagated to the viewport in non-paginated mode

Given that this affected a lot of enterprise users. Let's uplift this to ESR52.3.
Attachment #8818334 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.