680747 - progress element in table flows out of column

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[François Kooman]

Attachment: Test case showing progress element flowing out of its table cell

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110811165603

Steps to reproduce:

Place a <progress> element inside a table.


Actual results:

The progress element flows outside its cell.


Expected results:

The progress element should stay inside the cell (i.e.: the cell should be expanded to fit the progress element).

The issue does not occur when you specify a width with the CSS width property.

Works fine on: Google Chrome Dev channel and Webkit nightly (Mac OS X)
Fails on Firefox 6 (Mac OS X), Firefox 64 bit Nightly on Linux...

Comment 1

[Mounir Lamouri (:mounir)]

David, the progress frame has an intrinsic width/height sets here nsProgressFrame::ComputeAutoSize, is that not enough or the table reflow code doesn't takes this into account? IOW, is that a bug in the progress side or the table side?

Comment 2

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

The progress element needs appropriate implementations of nsIFrame::GetMinWidth and nsIFrame::GetPrefWidth.  (They'd likely return the same thing assuming the model is that the progress element determines its width solely from its own width property, and otherwise takes a default width.)

Comment 3

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

It's also possible (perhaps even likely) that you'd want the progress element to shrink when it has 'auto' width and its containing block is narrower than its default auto width.  But you may still want the GetMinWidth/GetPrefWidth implementations to be as though that weren't the case.

Comment 4

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

Actually, though GetMinWidth and GetPrefWidth shouldn't consider the element's own 'width' property, so basically they should return the same value that GetAutoSize *now* does.

Then you have the additional question of whether you should also change ComputeAutoSize to consider (aCBSize.width - aMargin.width - aBorder.width - aPadding.width) -- which is a change that you wouldn't make to GetMinWidth or GetPrefWidth.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This is fixing the testcase. I didn't update ComputeAutoSize() on purpose. I believe this is another issue so I'm going to open a bug for it.

Comment 6

[Mounir Lamouri (:mounir)]

David, I've been able to write the fix with your explanations but I have a few questions:
- Why there is no GetMinHeight and GetPrefHeight? Is that related to the box model that never try to shrink the height?
- Why GetMinWidth and GetPrefWidth do not have to take into account 'width' being set? Is it never called if width is set? IOW, if width is set, min-width will be set to width automatically?
- Why is this (aCBSize.width - aMargin.width - aBorder.width - aPadding.width) not needed for GetMinWidth and GetPrefWidth? The methods do not have the needed arguments to do the computation, does that mean this is always done when GetMinWidth is used?
- If I understand it correct (aCBSize.width - aMargin.width - aBorder.width - aPadding.width) would be needed to shrink the frame inside it's container when the container is narrower than the auto size. IOW, the following code should show a progress bar of 9em instead of 10em?
data:text/html,<div style="width:9em; height: 10em; background-color: red;">foo<br><progress value=0.5></progress></div>
Opera and Chrome doesn't seem to do that. Is there a reason why it seems needed to you?

Comment 7

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

Comment on attachment 554806 [details] [diff] [review]
Patch v1

r=dbaron

Comment 8

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

(In reply to Mounir Lamouri (:volkmar) from comment #6)
> David, I've been able to write the fix with your explanations but I have a
> few questions:
> - Why there is no GetMinHeight and GetPrefHeight? Is that related to the box
> model that never try to shrink the height?

Basically, because most of the CSS box model is built around a layout algorithm where widths are input and heights are output.  However, the widths that are input can, when sizing around the content is required, be a function of the "intrinsic" widths of the content, which are computed in another pass over the content prior to the layout algorithm (done in GetMinWidth / GetPrefWidth).

> - Why GetMinWidth and GetPrefWidth do not have to take into account 'width'
> being set? Is it never called if width is set? IOW, if width is set,
> min-width will be set to width automatically?

They don't because the rule is that in these functions, the *parent* of an element considers its 'width', 'min-width', and 'max-width', rather than the element itself.  This is important for implementation of things like width: min-content, width: max-content, and width: fit-content (which we currently implement with -moz- prefixes).  It wouldn't be possible to implement these if the definition of intrinsic widths on an element depended on its own 'width'.

> - Why is this (aCBSize.width - aMargin.width - aBorder.width -
> aPadding.width) not needed for GetMinWidth and GetPrefWidth? The methods do
> not have the needed arguments to do the computation, does that mean this is
> always done when GetMinWidth is used?

GetMinWidth and GetPrefWidth are returning intrinsic sizes of the content (basically, for a paragraph with text and no styles, GetMinWidth should give the width of the longest word and GetPrefWidth should return the width of all the text laid out on one line, and then everything more complicated fits in to those concepts).  Since they're something that happens prior to the layout algorithm rather than during it, the containing block size doesn't (and can't) come in to play.

> - If I understand it correct (aCBSize.width - aMargin.width - aBorder.width
> - aPadding.width) would be needed to shrink the frame inside it's container
> when the container is narrower than the auto size. IOW, the following code
> should show a progress bar of 9em instead of 10em?
> data:text/html,<div style="width:9em; height: 10em; background-color:
> red;">foo<br><progress value=0.5></progress></div>
> Opera and Chrome doesn't seem to do that. Is there a reason why it seems
> needed to you?

I don't remember what "IOW" means, but:  I thought that might be a nice behavior since the element is essentially flexible, but I don't feel strongly about it.  It's probably fine as it is.

Comment 9

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/cb78a4209560

Comment 10

[Mounir Lamouri (:mounir)]

Thanks for the explanations :)

I think I'm going to have <progress> not shrinking when it's inside a narrower container because other browsers seem to do that and other elements too (like <input>). I do not have enough experience with the CSS box model to evaluate the idea itself but I believe consistency is always good :)

Oh, and IOW means "In Other Words".

Comment 11

[Mounir Lamouri (:mounir)]

By the way, F. Kooman, thank you for filling those two bugs :)

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 554806 [details] [diff] [review]
Patch v1

Asking for approval to push this fix to Aurora given that it's low risk and would fix a web facing bug.

Comment 13

[François Kooman]

(In reply to Mounir Lamouri (:volkmar) from comment #11)
> By the way, F. Kooman, thank you for filling those two bugs :)

You are welcome, thanks for the quick response and fixes! Awesome :)

Comment 14

[Johnathan Nightingale [:johnath]]

(In reply to Mounir Lamouri (:volkmar) from comment #12)
> Comment on attachment 554806 [details] [diff] [review]
> Patch v1
> 
> Asking for approval to push this fix to Aurora given that it's low risk and
> would fix a web facing bug.

The test for Aurora is really around whether this represents a recent regression (I don't think it does) or represents a critical crasher or security fix. We try to avoid taking "opportunistic" wins on the stabilization branches, since RR means things get out pretty quickly anyhow. Is there a compelling reason to break the rules for this one?

Comment 15

[Johnathan Nightingale [:johnath]]

Comment on attachment 554806 [details] [diff] [review]
Patch v1

(Minusing pursuant to last comment, feel free to re-nom if appropriate, though)

Comment 16

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

Comment on attachment 554806 [details] [diff] [review]
Patch v1

I'm going to renom:

The basic reasoning here is that we really shouldn't have shipped <progress> (new in Fx 6) with this bug to begin with... except that it wasn't discovered until after we shipped.  If we'd found it when 6 was in Aurora I'd have pushed hard for either (a) land this or (b) disable <progress> until we can get it landed.

In other words, it's a bug in a new feature, except one we made the mistake of shipping.

Comment 17

[Mounir Lamouri (:mounir)]

Pushed to Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f00ceef85a8

The fix is going to appear in Firefox 8.
François, thank you again for reporting those bugs.

Comment 18

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a2) Gecko/20111003 Firefox/9.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111002 Firefox/10.0a1

Verified in Firefox 8, 9, 10 on Ubuntu 11.04, Mac OS 10.6, Windows 7 and Windows XP using the test case in the attachment.
The progress element no longer overlaps the text.