Last Comment Bug 680747 - progress element in table flows out of column
: progress element in table flows out of column
Status: VERIFIED FIXED
[qa!]
: testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 686886
  Show dependency treegraph
 
Reported: 2011-08-21 03:40 PDT by François Kooman
Modified: 2011-10-05 05:18 PDT (History)
5 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
fixed


Attachments
Test case showing progress element flowing out of its table cell (523 bytes, text/html)
2011-08-21 03:40 PDT, François Kooman
no flags Details
Patch v1 (2.86 KB, patch)
2011-08-22 02:22 PDT, Mounir Lamouri (:mounir)
dbaron: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description François Kooman 2011-08-21 03:40:55 PDT
Created attachment 554707 [details]
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) 2011-08-21 03:52:16 PDT
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-10 2011-08-21 08:08:38 PDT
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-10 2011-08-21 08:12:02 PDT
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-10 2011-08-21 08:16:10 PDT
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) 2011-08-22 02:22:36 PDT
Created attachment 554806 [details] [diff] [review]
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) 2011-08-22 02:34:03 PDT
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-10 2011-08-22 07:20:28 PDT
Comment on attachment 554806 [details] [diff] [review]
Patch v1

r=dbaron
Comment 8 David Baron :dbaron: ⌚️UTC-10 2011-08-22 07:29:54 PDT
(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) 2011-08-23 01:38:34 PDT
http://hg.mozilla.org/mozilla-central/rev/cb78a4209560
Comment 10 Mounir Lamouri (:mounir) 2011-08-23 01:59:46 PDT
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) 2011-08-23 02:00:23 PDT
By the way, F. Kooman, thank you for filling those two bugs :)
Comment 12 Mounir Lamouri (:mounir) 2011-08-23 02:01:13 PDT
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 2011-08-23 02:08:06 PDT
(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] 2011-08-23 15:05:28 PDT
(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] 2011-08-23 15:05:58 PDT
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-10 2011-08-23 19:42:54 PDT
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) 2011-08-26 04:04:30 PDT
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] 2011-10-05 00:26:55 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.