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

RESOLVED FIXED in mozilla1.8alpha1

Status

()

Core
Layout: Misc Code
P1
major
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Simon Gilligan, Assigned: bz)

Tracking

({testcase})

Trunk
mozilla1.8alpha1
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Reporter)

Description

13 years ago
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.
Created attachment 147020 [details]
Testcase: no need for border-box
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).
Status: UNCONFIRMED → NEW
Component: Layout → Layout: Misc Code
Ever confirmed: true
QA Contact: core.layout → core.layout.misc-code
Summary: HTML width problem in border-box box model → Incorrect sizing of canvas when root element has borders
Created attachment 147022 [details]
Another testcase
Not adding in the borders is no good -- that makes us end up with text
overlapping the bottom border in that testcase...
Created attachment 147024 [details] [diff] [review]
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?
David, Robert, what do you think of that approach?
Blocks: 15405, 57906
Created attachment 147028 [details] [diff] [review]
Patch that would be OK for branch, I think
Attachment #147028 - Flags: superreview?(dbaron)
Attachment #147028 - Flags: review?(dbaron)
Attachment #147024 - Flags: superreview?(dbaron)
Attachment #147024 - Flags: review?(dbaron)
Looks reasonable to me. I don't think we need this on the branch, though.
> This incidentally fixes bug 57906 and bug 15405....

Yay!

Updated

13 years ago
Keywords: testcase
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.
Attachment #147024 - Flags: superreview?(dbaron)
Attachment #147024 - Flags: superreview+
Attachment #147024 - Flags: review?(dbaron)
Attachment #147024 - Flags: review+
Attachment #147028 - Flags: superreview?(dbaron)
Attachment #147028 - Flags: superreview-
Attachment #147028 - Flags: review?(dbaron)
Attachment #147028 - Flags: review-
> 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.
Assignee: nobody → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha
Summary: Incorrect sizing of canvas when root element has borders → [FIXr]Incorrect sizing of canvas when root element has borders
Fix checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.