Closed Bug 930218 Opened 9 years ago Closed 7 years ago

Parent shrink-wrap width too wide when child has intrinsic ratio and percent height and ancestor has border-box box-sizing


(Core :: Layout, defect)

Windows 7
Not set



Tracking Status
firefox45 --- fixed


(Reporter: ufukbay, Assigned: bzbarsky, Mentored)



(Whiteboard: [lang=c++])


(3 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

Put a canvas(max-height:100%) inside a floating div(with fixed height and box-sizing:border-box)

Actual results:

A gap appears between right sides of the elements (2 x wrapper's border-size)

Expected results:

Wrapper should wrap the canvas without any gap.
Wrapper does not have to be a floating div. An inline-block element would also behave similarly.
Component: General → Layout
Ever confirmed: true
Version: unspecified → Trunk
Blocks: 243412
Seems likely to be something wrong with the code in nsLayoutUtils::IntrinsicForContainer that handles elements with an intrinsic ratio -- probably either GetPercentHeight or some of its callers in IntrinsicForContainer need to handle box-sizing.  (All this code is in layout/base/nsLayoutUtils.cpp.)

At first glance, it seems likely that the problem is that GetPercentHeight needs to consider the box-sizing value on the containing block, and use the containing block's content-box height.  Then again, I'm assuming that percentage heights should always be relative to the content-box height, no matter what box-sizing is.  I think that's probably correct, but also likely unspecified.  (Note, however, that GetPercentHeight should *not* consider the box-sizing on aFrame; the callers appear to do that, though it's worth checking that they all do it correctly.)

So fixing this bug requires:
 (a) understanding the GetPercentHeight callers and checking whether the above is correct
 (b) fixing as appropriate
 (c) writing a reftest

Looking at the code, I'm also very suspicious of the code in IntrinsicForContainer that uses GetPercentHeight on padding -- that seems wrong, since percentage padding is relative to widths.
Whiteboard: [mentor=dbaron]
Blocks: 364066
David, does this need to block unprefixing box-sizing?
Flags: needinfo?(dbaron)
I don't have a strong opinion either way, but I'm inclined to think it does not.
Flags: needinfo?(dbaron)
Whiteboard: [mentor=dbaron] → [mentor=dbaron][lang=c++]
Dear David,

I'm a new comer to the Mozilla code base.
Could I work on this bug as a first bug ?
This is probably a bit too much for a *first* bug; you're better off starting with something else, I think.
Thank you for the advice, I'll find something easier to start with.

Good luck squashing this one,
Hi, I am interested in working with the bug. So please can you assign it to me?


One other note about this bug:  it's likely to involve fixes for a few separate problems.  It's best to keep fixes for separate problems in separate patches so that they're easier to review
Assignee: nobody → allamsetty.anup
Mentor: dbaron
Whiteboard: [mentor=dbaron][lang=c++] → [lang=c++]
Assignee: allamsetty.anup → nobody
Duplicate of this bug: 1081883
Duplicate of this bug: 1081883
dbaron, I tried the nightly version and still can reproduce. I am interested in this bug. May I take this bug?
Flags: needinfo?(dbaron)
Yes, absolutely.

(Sorry for the delay responding; was on vacation.)
Flags: needinfo?(dbaron)
Duplicate of this bug: 1187172
Summary: Canvas not wrapped by parent that has border-box sizing → Parent shrink-wrap width too wide when child has intrinsic ratio and percent height and ancestor has border-box box-sizing
Duplicate of this bug: 1211627
Duplicate of this bug: 1230658
Assignee: nobody → bzbarsky
Comment on attachment 8696088 [details] [diff] [review]
part 1.  Factor out the computation of block-size taken up by box-sizing into a separate function so we can reuse it

>+      // XXXbz Calling GetPercentBSize on padding values looks bogus, since
>+      // percent padding is always a percentage of the inline-size of the
>+      // containing block.  We should perhaps just treat non-absolute paddings
>+      // here as 0 instead.

Maybe file a bug on this?
Attachment #8696088 - Flags: review?(dbaron) → review+
Attachment #8696089 - Flags: review?(dbaron) → review+
> Maybe file a bug on this?

Bug 1231059 filed.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Regressions: 1677353
You need to log in before you can comment on or make changes to this bug.