Closed
Bug 241694
Opened 21 years ago
Closed 21 years ago
[FIXr]Incorrect sizing of canvas when root element has borders
Categories
(Core :: Layout, defect, P1)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha1
People
(Reporter: simon.gilligan, Assigned: bzbarsky)
References
()
Details
(Keywords: testcase)
Attachments
(4 files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review-
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Comment 2•21 years ago
|
||
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
Assignee | ||
Comment 3•21 years ago
|
||
Assignee | ||
Comment 4•21 years ago
|
||
Not adding in the borders is no good -- that makes us end up with text
overlapping the bottom border in that testcase...
Assignee | ||
Comment 5•21 years ago
|
||
Assignee | ||
Comment 6•21 years ago
|
||
David, Robert, what do you think of that approach?
Assignee | ||
Comment 7•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #147028 -
Flags: superreview?(dbaron)
Attachment #147028 -
Flags: review?(dbaron)
Assignee | ||
Updated•21 years ago
|
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.
Comment 9•21 years ago
|
||
Comment 10•21 years ago
|
||
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.
Updated•21 years ago
|
Attachment #147024 -
Flags: superreview?(dbaron)
Attachment #147024 -
Flags: superreview+
Attachment #147024 -
Flags: review?(dbaron)
Attachment #147024 -
Flags: review+
Updated•21 years ago
|
Attachment #147028 -
Flags: superreview?(dbaron)
Attachment #147028 -
Flags: superreview-
Attachment #147028 -
Flags: review?(dbaron)
Attachment #147028 -
Flags: review-
Assignee | ||
Comment 11•21 years ago
|
||
> 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
Assignee | ||
Updated•21 years ago
|
Summary: Incorrect sizing of canvas when root element has borders → [FIXr]Incorrect sizing of canvas when root element has borders
Assignee | ||
Comment 12•21 years ago
|
||
Fix checked in on trunk.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•