Bug 79128 – Fieldset unexpectedly stretches to minimum intrinsic width.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mat Marquis]

Hey guys,

This originally came in as a jQuery Mobile bug ( https://github.com/jquery/jquery-mobile/issues/3150 ), but after some digging it seems to be an issue inherent to fieldsets in WebKit. I would expect a largely unmodified fieldset to behave in much the same way as a div would, but it seems that percentage-based widths are ignored in favor of the inherent width of the elements they contain—even when a specific `overflow` property is set.

My best guess is that it’s related to the “stretchesToMinIntrinsicWidth()” boolean added here: 
http://trac.webkit.org/changeset/18788/trunk/WebCore/rendering/RenderFieldset.h

Thanks!
Mat Marquis

[Mustafizur Rahaman( :rahaman)]

Can you please provide a reduced test case to reproduce the issue so that it can be easily debugged?

[SravanKumar S(:sravan)]

Created attachment 132454 [details]
patch

The reference to this should be IE9 and not FF, as the issue is present is FireFox also.
fixed the issue by returning false if width is exclusively specified in source(html/css).

[Gustavo Noronha (kov)]

Comment on attachment 132454 [details]
patch

Attachment 132454 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11967786

[Gyuyoung Kim]

Comment on attachment 132454 [details]
patch

Attachment 132454 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11975061

[Build Bot]

Comment on attachment 132454 [details]
patch

Attachment 132454 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11974392

[WebKit Review Bot]

Comment on attachment 132454 [details]
patch

Attachment 132454 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11963824

[SravanKumar S(:sravan)]

Created attachment 132455 [details]
Patch after fixing the typo

Accidentally((Shift+~)) RenderFieldset got changed to RenderFieldSet,  my bad, sorry for the noise.
Uploading the patch again, after fixing the issue.

[SravanKumar S(:sravan)]

Hi,

Can some one please review the patch.

[Julien Chaffraix]

Comment on attachment 132455 [details]
Patch after fixing the typo

View in context: https://bugs.webkit.org/attachment.cgi?id=132455&action=review

> Source/WebCore/ChangeLog:9
> +        in css before stretching to minimum intrinsic width.

Your explanation about us matching IE and not FF sounds reasonable to be put here in your ChangeLog (note also the FF acknowledged the broken behavior).

> Source/WebCore/rendering/RenderFieldset.cpp:201
> +    if (style()->width().isSpecified())

After this change, there is no guarantee a web author cannot manually shrink a fieldset below its intrinsic size. There is some warnings on the Mozilla bug about the potential compatibility risk about that: https://bugzilla.mozilla.org/show_bug.cgi?id=504622

I don't know fieldsets enough to know how big of a risk this is.

> Source/WebCore/rendering/RenderFieldset.h:45
> +    virtual bool stretchesToMinIntrinsicLogicalWidth() const; 

Extra-space at the end of the line.

> LayoutTests/fast/forms/fieldset-width-nostretch-ifspecified.html:5
> +                width: 50%;

Length::isSpecified means 3 values:
* percent length
* fixed length
* calculated length

I would like to see a test for at least the 2 first.

> LayoutTests/fast/forms/fieldset-width-nostretch-ifspecified.html:19
> +            <div class="constrain">
> +                <h1>div</h1>
> +                <p>This is a div with width set to 50%</p>
> +            </div>
> +            <fieldset class="constrain">
> +                <h1>fieldset</h1>
> +                <p>This is a fieldset with width set to 50%</p>
> +            </fieldset>

This looks like a good candidate for a ref tests as the <div> and the <fieldset> should behave the same.

[SravanKumar S(:sravan)]

"This looks like a good candidate for a ref tests as the <div> and the <fieldset> should behave the same."

Well, the actual bug is related to fieldsets, and bug raiser has just used div to reference, so am thinking to remove divs from the test case, and just add a plain .html and upload its -expected.png and -expected.txt.

One more problem i see if we add ref-test with div is am seeing that width of fieldset is little more than the div, so am not sure if ref-test will pass, also i have never written a reftest, so please clarify if we really need a ref test here.

[Julien Chaffraix]

(In reply to comment #10)
> "This looks like a good candidate for a ref tests as the <div> and the <fieldset> should behave the same."
> 
> Well, the actual bug is related to fieldsets, and bug raiser has just used div to reference,

Yes, which is why I said it would be a good candidate for a ref tests. The div and fieldset have similar behavior here.

> One more problem i see if we add ref-test with div is am seeing that width of fieldset is little more than the div, so am not sure if ref-test will pass,

You may have to tweak the div and fieldset's paddings, margins and borders so that their size matches.

> also i have never written a reftest, so please clarify if we really need a ref test here.

Yes, it would be the best. Having not tried it, I don't know which issues you will see. Your test as it is should be made cross-platform anyway and I think ref testing is the best way to go.

[SravanKumar S(:sravan)]

Created attachment 132768 [details]
Updated patch with reftests

This issues is present only when css width is specified interms of Percentage and not with Fixed width(px,cs etc) case. So, updated the code and added ref tests accordingly.

[Julien Chaffraix]

Comment on attachment 132768 [details]
Updated patch with reftests

View in context: https://bugs.webkit.org/attachment.cgi?id=132768&action=review

> Source/WebCore/rendering/RenderFieldset.cpp:201
> +    // If width is exclusively specified then Fieldsets should not stretch
> +    if (style()->width().isPercent())

It's undesirable to have different behavior between specified widths. If fixed positions don't work here, I think our logic needs to be changed elsewhere so that they work the same.

Also the fixed position case *should* be tested.

> Source/WebCore/rendering/RenderFieldset.h:45
> +    virtual bool stretchesToMinIntrinsicLogicalWidth() const; 

Still extra-space.

> LayoutTests/fast/forms/fieldset-percent-width-nostretch-ifspecified-expected.html:17
> +                111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111

First a test should include:
* the bug title
* the bug number
* condition to pass (our container should be 50% here)

> LayoutTests/fast/forms/fieldset-percent-width-nostretch-ifspecified.html:1
> +<html>

Please add a doctype (those comments are also for the -expected.html):
<!DOCTYPE html>

> LayoutTests/fast/forms/fieldset-percent-width-nostretch-ifspecified.html:3
> +        <style type="text/css" media="screen">

No need for the attribute.

> LayoutTests/fast/forms/fieldset-percent-width-nostretch-ifspecified.html:15
> +        <div class="wrap">

the wrap class is unused. I would add it back and put a fixed size on this wrapper to limit the potential for cross-browser differences.

[SravanKumar S(:sravan)]

Hi Julien,
After little analysis, there are quite a few points we need to consider here.

1.FF/Chrome/Safari all have same behaviour w.r.t Percentage/Calculated widths, but when Fixed width is specified Chrome/Safari obeys/follows the width given, but FF does'nt(FF has the bug in all 3 cases)Now, if what ever chrome is doing for Fixed width is intentional, then it should do the same for Percentage/Calculated cases.

Note: This deviation is already creating web-compatibility issue.

2. The reason for this deviation is because below logic is setting minPreferredLogicalWidth()(as it is higher than logicalWidth() for our test case) as logical width for Fieldset.

    // Fieldsets are currently the only objects that stretch to their minimum width.
    if (stretchesToMinIntrinsicLogicalWidth()) {
        setLogicalWidth(max(logicalWidth(), minPreferredLogicalWidth()));
        logicalWidthLength = Length(logicalWidth(), Fixed);
    }

And, m_minPreferredLogicalWidth is calculated differently for Fixed case and Calc/Percent case in void RenderBlock::computePreferredLogicalWidths()

Following is the code responsible for this behaviour.

if (!isTableCell() && styleToUse->logicalWidth().isFixed() && styleToUse->logicalWidth().value() > 0 && style()->marqueeBehavior() != MALTERNATE)
        m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth = computeContentBoxLogicalWidth(styleToUse->logicalWidth().value());
    else {
        m_maxPreferredLogicalWidth = 0;

    if (childrenInline())
            computeInlinePreferredLogicalWidths();


3. With above analysis, if we have to fix it, then i see fix in either of the following two places, 

In RenderFieldset.cpp:
bool RenderFieldset::stretchesToMinIntrinsicLogicalWidth() const
{
    if(style()->width().isSpecified())
        return false;
    return true; 
}

Note: This change means completely specific to Fieldsets
or 

In void RenderBlock::computePreferredLogicalWidths()

by changing if block to use styleToUse->logicalWidth().isSpecified() for m_minPreferredLogicalWidth instead of styleToUse->logicalWidth().isFixed()

Note: As this is a general logic, i am thinking this might hurt other cases which follows the same path.


I think i lack big picture or broader view to decide as which is the right fix, or atleast if any of them is right fix.
Please let me know your comments.

[SravanKumar S(:sravan)]

One disclaimer: Comment# 14 behaviour is without <!DOCTYPE html>.
With that DTD(HTML5), i see that behaviour is uniform in all 3 cases(Fixed/Percent/Calc), and it follows this bug. 

So, from your review comments if test case needs to have <!DOCTYPE html>, then i think then my first patch of

In RenderFieldset.cpp:
bool RenderFieldset::stretchesToMinIntrinsicLogicalWidth() const
{
    if(style()->width().isSpecified())
        return false;
    return true; 
}

will hold good, Please confirm.

[Julien Chaffraix]

(In reply to comment #15)
> One disclaimer: Comment# 14 behaviour is without <!DOCTYPE html>.
> With that DTD(HTML5), i see that behaviour is uniform in all 3 cases(Fixed/Percent/Calc), and it follows this bug. 
> 
> So, from your review comments if test case needs to have <!DOCTYPE html>, then i think then my first patch of
> 
> In RenderFieldset.cpp:
> bool RenderFieldset::stretchesToMinIntrinsicLogicalWidth() const
> {
>     if(style()->width().isSpecified())
>         return false;
>     return true; 
> }
> 
> will hold good, Please confirm.

Yes, consistency is important here so I think this is the way to go. We should abide by the author's sizing if he specified one. This matches IE9.

[SravanKumar S(:sravan)]

Created attachment 133190 [details]
Patch as dicussed.

[Julien Chaffraix]

Comment on attachment 133190 [details]
Patch as dicussed.

View in context: https://bugs.webkit.org/attachment.cgi?id=133190&action=review

> Source/WebCore/ChangeLog:8
> +        Fieldset element width will now check if css width is specified exclusively

exclusively? I think the adverb is "explicitly" (note: I am not a native English speaker)

> Source/WebCore/rendering/RenderFieldset.cpp:200
> +    // If width is exclusively specified then Fieldsets should not stretch

Ditto here.

> Source/WebCore/rendering/RenderFieldset.h:45
> +    virtual bool stretchesToMinIntrinsicLogicalWidth() const;

Forgot to mention that while we are touching this virtual call, we should annotate it with OVERRIDE.

> LayoutTests/fast/forms/fieldset-width-nostretch-ifspecified-expected.html:11
> +                overflow: hidden;
> +                white-space: nowrap;

Nit: AFAICT the overflow: hidden shouldn't be needed, not sure about the white-space: nowrap.

> LayoutTests/fast/forms/fieldset-width-nostretch-ifspecified-expected.html:22
> +                width: 200px; width: calc(350px + 50px);

Nit: It should be 2 lines for consistency.

> LayoutTests/fast/forms/fieldset-width-nostretch-ifspecified-expected.html:36
> +    <p> For this test to pass, you should see our first container to be 50% of its parent, second 300px fixed and thrid calc width of 400px(if not supported fallback to 200px fixed) </p>

thrid -> third

> LayoutTests/fast/forms/fieldset-width-nostretch-ifspecified-expected.html:39
> +                11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111

Nit: Usually random text is http://en.wikipedia.org/wiki/Lorem_ipsum

> LayoutTests/fast/forms/fieldset-width-nostretch-ifspecified.html:22
> +                width: 200px; width: calc(350px + 50px);

Ditto.

[SravanKumar S(:sravan)]

Created attachment 133386 [details]
Patch with review comments

Thanks for all the review comments and info shared.

[WebKit Review Bot]

Comment on attachment 133386 [details]
Patch with review comments

Clearing flags on attachment: 133386

Committed r111818: <http://trac.webkit.org/changeset/111818>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.