[webkit-reviews] review canceled: [Bug 79128] Fieldset unexpectedly stretches to minimum intrinsic width. : [Attachment 132768] Updated patch with reftests

Tue Mar 20 10:15:21 PDT 2012

Julien Chaffraix <jchaffraix at webkit.org> has canceled 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 132768: Updated patch with reftests
https://bugs.webkit.org/attachment.cgi?id=132768&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.ht
ml:17
> +		  
1111111111111111111111111111111111111111111111111111111111111111111111111111111
11111111111111111111111111111111111111111111111111111111111111111111111111

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.