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

RESOLVED FIXED in Firefox 45

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Ufku, Assigned: bz, Mentored)

Tracking

Trunk
mozilla45
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 821279 [details]
ff-block-canvas-inside-border-box-sizing.html

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.

Updated

4 years ago
Status: UNCONFIRMED → NEW
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++]

Comment 4

3 years ago
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.

Comment 6

3 years ago
Thank you for the advice, I'll find something easier to start with.

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

Regards,
Anup
Sure.

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@dbaron.org
Whiteboard: [mentor=dbaron][lang=c++] → [lang=c++]

Updated

3 years ago
Assignee: allamsetty.anup → nobody
Duplicate of this bug: 1081883

Updated

3 years ago
Duplicate of this bug: 1081883

Comment 11

2 years ago
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
Created 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
Attachment #8696088 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8696089 [details] [diff] [review]
part 2.  Account for the parent's box-sizing whe figuring out the percentage height of a kid with an intrinsic ratio for purposes of determining the parent's shrink-wrap width
Attachment #8696089 - Flags: review?(dbaron)
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.

Comment 20

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6196e9f9d139
https://hg.mozilla.org/integration/mozilla-inbound/rev/38fbb8418ffc

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6196e9f9d139
https://hg.mozilla.org/mozilla-central/rev/38fbb8418ffc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.