Bug 35981

Summary: Account for margin after when laying out <legend> element
Product: WebKit Reporter: Patrick Paul-Hus <hydrozen>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, inferno, jchaffraix, mainerror, marcin, mitz, neildrthomson, ojan, paulirish, tony, webkit.review.bot, weirdan
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Demonstration of the bug
none
Screenshot of margins not being applied in Chromium 13 ubuntu 10.04
none
Proposed fix, take the margin after into account.
none
Patch for landing
none
Patch for landing none

Description Patrick Paul-Hus 2010-03-10 12:46:49 PST
Created attachment 50425 [details]
Demonstration of the bug

I defined a form using semantic markup, but it seems that I can't get a bottom-margin to render under the legend element. Using the web inspector, it does say that there's a bottom-margin applied, but the content that comes after is pushed right underneath and seems to ignore the bottom-margin. I've attached a small html document which shows the issue. It renders fine in IE and FF but there's no bottom margin in Chrome Beta for  and Safari 4.

I am attaching a small HTML document as proof of concept.... for now I fixed the problem by using padding instead of margin.
Comment 1 Pip 2011-09-07 09:01:32 PDT
Created attachment 106589 [details]
Screenshot of margins not being applied in Chromium 13 ubuntu 10.04

Exactly the same code in Firefox 6 makes the second legend float 50px above the fieldset. From http://www.w3schools.com/cssref/tryit.asp?filename=trycss_margin-bottom
Comment 2 WebKit Review Bot 2011-09-07 09:04:36 PDT
Attachment 106589 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1

Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Octavian Damiean 2012-03-05 05:20:11 PST
I can confirm this bug.

Here is a jsfiddle[1] demonstrating the bug. This fiddle[2] shows that the legend element gets rendered with a margin when not setting it inside of a fieldset which of course is an importer use of the legend element.

[1]: http://jsfiddle.net/mainerror/XJYgL/
[2]: http://jsfiddle.net/mainerror/XJYgL/1/
Comment 4 Patrick Paul-Hus 2012-03-05 17:42:20 PST
2 years soon! Happy bday Bug 35981!
Comment 5 Julien Chaffraix 2012-06-06 21:29:57 PDT
The issue is in RenderFieldset logic that needs to account for the <legend>'s margins in the block flow direction. Note that using margin-top doesn't work either.
Comment 6 Julien Chaffraix 2012-06-10 07:49:43 PDT
Created attachment 146736 [details]
Proposed fix, take the margin after into account.
Comment 7 Abhishek Arya 2012-06-11 11:05:33 PDT
Comment on attachment 146736 [details]
Proposed fix, take the margin after into account.

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

r=me

> Source/WebCore/ChangeLog:3
> +        Can't apply a bottom-margin to the legend element

nit: probably like "Add bottom-margin support to legend element".

> Source/WebCore/rendering/RenderFieldset.cpp:108
> +        LayoutUnit collapsedLegendExtend;

nit: probably s/Extend/Extent
Comment 8 Julien Chaffraix 2012-06-11 15:03:19 PDT
Created attachment 146923 [details]
Patch for landing
Comment 9 Abhishek Arya 2012-06-11 15:06:32 PDT
Just a very friendly reminder on this cleanup in the testcase :)

-moz-writing-mode: vertical-lr;
writing-mode: vertical-lr;
Comment 10 Julien Chaffraix 2012-06-11 15:35:10 PDT
Comment on attachment 146923 [details]
Patch for landing

> Just a very friendly reminder on this cleanup in the testcase :)

Right, let me remove the -moz* ones.
Comment 11 Julien Chaffraix 2012-06-11 15:43:28 PDT
Created attachment 146936 [details]
Patch for landing
Comment 12 WebKit Review Bot 2012-06-11 16:14:54 PDT
Comment on attachment 146936 [details]
Patch for landing

Clearing flags on attachment: 146936

Committed r120017: <http://trac.webkit.org/changeset/120017>
Comment 13 WebKit Review Bot 2012-06-11 16:14:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 mitz 2012-06-11 18:17:51 PDT
(In reply to comment #12)
> (From update of attachment 146936 [details])
> Clearing flags on attachment: 146936
> 
> Committed r120017: <http://trac.webkit.org/changeset/120017>

This caused fast/block/basic/fieldset-stretch-to-legend.html to fail. See for example <http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r120021%20(8106)/results.html>
Comment 15 mitz 2012-06-11 18:30:18 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 146936 [details] [details])
> > Clearing flags on attachment: 146936
> > 
> > Committed r120017: <http://trac.webkit.org/changeset/120017>
> 
> This caused fast/block/basic/fieldset-stretch-to-legend.html to fail. See for example <http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r120021%20(8106)/results.html>

This test needs its expected results updated.
Comment 16 Julien Chaffraix 2012-06-11 18:31:40 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (From update of attachment 146936 [details] [details] [details])
> > > Clearing flags on attachment: 146936
> > > 
> > > Committed r120017: <http://trac.webkit.org/changeset/120017>
> > 
> > This caused fast/block/basic/fieldset-stretch-to-legend.html to fail. See for example <http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r120021%20(8106)/results.html>
> 
> This test needs its expected results updated.

Yes, it looks like I missed it during testing but the new results make sense. I will do the update as EFL needs the same treatment.
Comment 17 mitz 2012-06-11 18:43:13 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #12)
> > > > (From update of attachment 146936 [details] [details] [details] [details])
> > > > Clearing flags on attachment: 146936
> > > > 
> > > > Committed r120017: <http://trac.webkit.org/changeset/120017>
> > > 
> > > This caused fast/block/basic/fieldset-stretch-to-legend.html to fail. See for example <http://build.webkit.org/results/Lion%20Release%20(WebKit2%20Tests)/r120021%20(8106)/results.html>
> > 
> > This test needs its expected results updated.
> 
> Yes, it looks like I missed it during testing but the new results make sense. I will do the update as EFL needs the same treatment.

I updated the Mac expected results in <http://trac.webkit.org/r120026>.
Comment 18 Julien Chaffraix 2012-06-11 18:49:48 PDT
> I updated the Mac expected results in <http://trac.webkit.org/r120026>.

And updated / skipped the rest of the platforms in http://trac.webkit.org/changeset/120027.