[webkit-reviews] review granted: [Bug 79128] Fieldset unexpectedly stretches to minimum intrinsic width. : [Attachment 133190] Patch as dicussed.

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted SravanKumar S
<ssandela at innominds.com>'s request for review:
Bug 79128: Fieldset unexpectedly stretches to minimum intrinsic width.
https://bugs.webkit.org/show_bug.cgi?id=79128

Attachment 133190: Patch as dicussed.
https://bugs.webkit.org/attachment.cgi?id=133190&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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
> +		  
1111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111

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.