Summary: | Account for margin after when laying out <legend> element | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick Paul-Hus <hydrozen> | ||||||||||||
Component: | Layout and Rendering | Assignee: | 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: |
|
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 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.
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/ 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. Created attachment 146736 [details]
Proposed fix, take the margin after into account.
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 Created attachment 146923 [details]
Patch for landing
Just a very friendly reminder on this cleanup in the testcase :) -moz-writing-mode: vertical-lr; writing-mode: vertical-lr; 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. Created attachment 146936 [details]
Patch for landing
Comment on attachment 146936 [details] Patch for landing Clearing flags on attachment: 146936 Committed r120017: <http://trac.webkit.org/changeset/120017> All reviewed patches have been landed. Closing bug. (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> (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. (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. (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>. > 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. |
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.