241694 - [FIXr]Incorrect sizing of canvas when root element has borders

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Simon Gilligan]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a) Gecko/20040425 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a) Gecko/20040425

Please refer to test case at http://level11.com.au/mozbug/htmlwidth.htm.

The test case contains a single page, with yellow border on the HTML element and
a red border on the BODY element. HTML and BODY elements are set to 100% height
and width, with -moz-box-sizing:border-box.

Reproducible: Always
Steps to Reproduce:
Navigate to the test page.
Shorten your browser so the document overflows the viewport to give a vertical
scrollbar.

Actual Results:  
The yellow border is positioned one border width too wide, which creates a gap
between the red and yellow borders on right hand side, which inturn creates a
horizontal scrollbar to accomodate the extra document width. 

Note this behaviour can be observed in Moz 1.6 and 1.4.1.

Expected Results:  
Would have expected the yellow and red borders to remain next to each other (ie
no gap) and positioned flush up against the vertical scrollbar. No horizontal
scrollbar should be created.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase: no need for border-box

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problem seems to be in the code in CanvasFrame::Reflow that does:

    // First check the combined area
    if (NS_FRAME_OUTSIDE_CHILDREN & kidFrame->GetStateBits()) {
      // The background covers the content area and padding area, so check
      // for children sticking outside the child frame's padding edge
      nscoord paddingEdgeX = kidDesiredSize.width - border.right;
      nscoord paddingEdgeY = kidDesiredSize.height - border.bottom;

      if (kidDesiredSize.mOverflowArea.XMost() > paddingEdgeX) {
        kidDesiredSize.width = kidDesiredSize.mOverflowArea.XMost() +
                               border.right;
      }
      if (kidDesiredSize.mOverflowArea.YMost() > paddingEdgeY) {
        kidDesiredSize.height = kidDesiredSize.mOverflowArea.YMost() +
                                border.bottom;
      }
    }

At least when I comment this block out the bug goes away.

I suspect the only issue is that we should be using the overflow area without
adding in the bottom or right border (and possibly without comparing to this
"padding edge" business -- I see no reason for the canvas frame to ever be any
size other than the overflow area of the root frame).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

Not adding in the borders is no good -- that makes us end up with text
overlapping the bottom border in that testcase...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This incidentally fixes bug 57906 and bug 15405....

On branch, I'd say we don't want to fix either of those at this late stage? (We
can do it; just involves removing a tad less code, basically.)	But on trunk,
why not?

Comment 6

[Boris Zbarsky [:bzbarsky]]

David, Robert, what do you think of that approach?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that would be OK for branch, I think

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks reasonable to me. I don't think we need this on the branch, though.

Comment 9

[Hixie (not reading bugmail)]

> This incidentally fixes bug 57906 and bug 15405....

Yay!

Comment 10

[David Baron :dbaron:]

Comment on attachment 147028 [details] [diff] [review]
Patch that would be OK for branch, I think

This patch doesn't make much sense to me -- it introduces a discontinuity over
small variations of the overflow area or the desired size...

Also, if you want to land a patch on the branch, land it on the trunk first,
since the branch doesn't get much testing, and then land the other patch later.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> it introduces a discontinuity

Indeed, it does.

The problem, I guess, is that I don't see a good way to stretch the way we're
sorta expecting to without having this bug.  The basic problem is that we want
to stretch if there is content overflowing the content area.  But if there's no
overflow in the X direction, the overflow XMost() will just be the right border
edge.  That's exactly what it will be if there's content overflowing into the
right border but not past it.  But if there's no overflow we shouldn't stretch,
and if there's overflow we should, if we want to keep the "root stretches"
behavior, which I think we do want to keep on the branch.  This bug is about the
fact that we stretch in both cases, so we end up with a too-wide root when
there's vertical overflow but not horizontal overflow.

So possible approaches to the problem:

1)  Check in the trunk patch and don't touch the branch.
2)  Check in a patch that only fixes bug 57906 on trunk, test there, check it in
    on branch, then fix bug 15405 on trunk.
3)  Check in the patch that fixes both bugs on trunk, test, check in on branch.

Frankly, I prefer #1 -- it's far safer.  The number of places that use borders
on the root (which is the only way to trigger this bug) is pretty small, while
the number that gets the clientWidth or clientHeight of the root in JS and
manipulates it is rather nontrivial...

So unless someone has objections, I'm going to check the reviewed patch in on
the trunk tomorrow or the day after and leave branch well enough alone.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fix checked in on trunk.