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)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha1

People

(Reporter: simon.gilligan, Assigned: bzbarsky)

References

()

Details

(Keywords: testcase)

Attachments

(4 files)

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.
Attached file Testcase: no need for border-box (deleted) —
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
Attached file Another testcase (deleted) —
Not adding in the borders is no good -- that makes us end up with text overlapping the bottom border in that testcase...
Attached patch Proposed patch (deleted) — Splinter Review
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
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!
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
Closed: 21 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
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.

Attachment

General

Created:
Updated:
Size: