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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ufku]

Attachment: 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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

David, does this need to block unprefixing box-sizing?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't have a strong opinion either way, but I'm inclined to think it does not.

Comment 4

[François Freitag]

Dear David,

I'm a new comer to the Mozilla code base.
Could I work on this bug as a first bug ?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is probably a bit too much for a *first* bug; you're better off starting with something else, I think.

Comment 6

[François Freitag]

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

Good luck squashing this one,
François

Comment 7

[Anup Kumar [:reznord]]

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

Regards,
Anup

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 11

[Ethan Lin[:ethlin]]

dbaron, I tried the nightly version and still can reproduce. I am interested in this bug. May I take this bug?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, absolutely.

(Sorry for the delay responding; was on vacation.)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Factor out the computation of block-size taken up by box-sizing into a separate function so we can reuse it

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Maybe file a bug on this?

Bug 1231059 filed.

Comment 20

[Pulsebot]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6196e9f9d139
https://hg.mozilla.org/mozilla-central/rev/38fbb8418ffc