Bug 35981 - Account for margin after when laying out <legend> element
: Account for margin after when laying out <legend> element
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: HasReduction
:
:
  Show dependency treegraph
 
Reported: 2010-03-10 12:46 PST by
Modified: 2012-06-11 18:49 PST (History)


Attachments
Demonstration of the bug (1.90 KB, text/html)
2010-03-10 12:46 PST, Patrick Paul-Hus
no flags Details
Screenshot of margins not being applied in Chromium 13 ubuntu 10.04 (54.21 KB, image/png)
2011-09-07 09:01 PST, Pip
no flags Details
Proposed fix, take the margin after into account. (12.39 KB, patch)
2012-06-10 07:49 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (12.43 KB, patch)
2012-06-11 15:03 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (12.28 KB, patch)
2012-06-11 15:43 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2010-03-10 12:46:49 PST
Created an attachment (id=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 From 2011-09-07 09:01:32 PST -------
Created an attachment (id=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 From 2011-09-07 09:04:36 PST -------
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 From 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 From 2012-03-05 17:42:20 PST -------
2 years soon! Happy bday Bug 35981!
------- Comment #5 From 2012-06-06 21:29:57 PST -------
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 From 2012-06-10 07:49:43 PST -------
Created an attachment (id=146736) [details]
Proposed fix, take the margin after into account.
------- Comment #7 From 2012-06-11 11:05:33 PST -------
(From update of attachment 146736 [details])
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 From 2012-06-11 15:03:19 PST -------
Created an attachment (id=146923) [details]
Patch for landing
------- Comment #9 From 2012-06-11 15:06:32 PST -------
Just a very friendly reminder on this cleanup in the testcase :)

-moz-writing-mode: vertical-lr;
writing-mode: vertical-lr;
------- Comment #10 From 2012-06-11 15:35:10 PST -------
(From update of attachment 146923 [details])
> Just a very friendly reminder on this cleanup in the testcase :)

Right, let me remove the -moz* ones.
------- Comment #11 From 2012-06-11 15:43:28 PST -------
Created an attachment (id=146936) [details]
Patch for landing
------- Comment #12 From 2012-06-11 16:14:54 PST -------
(From update of attachment 146936 [details])
Clearing flags on attachment: 146936

Committed r120017: <http://trac.webkit.org/changeset/120017>
------- Comment #13 From 2012-06-11 16:14:59 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #14 From 2012-06-11 18:17:51 PST -------
(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>
------- Comment #15 From 2012-06-11 18:30:18 PST -------
(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.
------- Comment #16 From 2012-06-11 18:31:40 PST -------
(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.
------- Comment #17 From 2012-06-11 18:43:13 PST -------
(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] [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 From 2012-06-11 18:49:48 PST -------
> 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.