Bug 78684 – Layout issue with fieldset legend element

• Component: Layout and Rendering
• Status: CLOSED FIXED

[SravanKumar S(:sravan)]

Created attachment 127137 [details]
Test case to validate the buggy behaviour described above

Overview:
On applying padding style information to fieldset's "legend" element, the bottom border(of fieldset) gets clipped-off in Safari and Chrome. There is no such issue on FF & IE irrespective of padding value. 

Steps to Reproduce:
1. PFA test case to reproduce the same(fieldset-legend-padding.html)
2. Run it either on Safari or Chrome.


On analyzing further, I found that clipping of legend element is buggy in WebKit, and i may have the fix for it.
I will upload the patch once the bug is moved on to NEW state.

[Alexey Proskuryakov]

> I will upload the patch once the bug is moved on to NEW state.

We don't make much distinction between UNCONFIRMED and NEW, so it's perfectly fine to work on an UNCONFIRMED bug.

[SravanKumar S(:sravan)]

Thanks for the info. Will upload the patch, once layout test is ready.

[SravanKumar S(:sravan)]

Created attachment 127574 [details]
Patch to fix the clipping issue.

Patch to fix the clipping issue.

[WebKit Review Bot]

Comment on attachment 127574 [details]
Patch to fix the clipping issue.

Attachment 127574 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11539496

New failing tests:
fast/borders/fieldsetBorderRadius.html
fast/block/basic/fieldset-stretch-to-legend.html

[SravanKumar S(:sravan)]

Created attachment 127691 [details]
Attachment showing actual output

Hi,

I executed these failing tests(pixel tests) on win-7 and the first test is passing, but the second test(fast/block/basic/fieldset-stretch-to-legend.html) is failing with 0.61% of difference in images. How ever Following are my observations on both the test results, 

1. The difference is "actual image"(Attached actual image rendered through win launcher) has full border, where as the expected image has little clipped regions when ever the "legend" element touches left or right borders of fieldset.

2. I checked this behavior on all major browsers and only Chrome&Safari browser seems to be clipping the left and right borders.

3. Also, there is no specification saying that left and right borders should be clipped like the way Chrome&Safari browsers are doing right now.

So, i feel the present patch is also fixing these two tests and they need to be re-baselined, based on above data.

[SravanKumar S(:sravan)]

Comment on attachment 127691 [details]
Attachment showing actual output

Incorrect format

[SravanKumar S(:sravan)]

Created attachment 127692 [details]
Correct attachment showing un-clipped borders for fieldset-stretch-to-legend.html Test case

[SravanKumar S(:sravan)]

Created attachment 127791 [details]
New patch that updates previously failing pixel tests

Updated -expected files after re-baselining 

fast/borders/fieldsetBorderRadius.html
fast/block/basic/fieldset-stretch-to-legend.html

[WebKit Review Bot]

Comment on attachment 127791 [details]
New patch that updates previously failing pixel tests

Attachment 127791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11550150

New failing tests:
fast/borders/fieldsetBorderRadius.html
fast/block/basic/fieldset-stretch-to-legend.html

[Hajime Morrita]

Comment on attachment 127791 [details]
New patch that updates previously failing pixel tests

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

Thanks for taking this! 
The code basically looks OK. What's left is bureaucratic part...

1. It looks you don't set the mime type of the png files correctly. 
    Please check your svn configuration.
    http://trac.webkit.org/wiki/UsingGitWithWebKit

2. On chromium bot (cr-linux) redness, you can mark "fail" to these failing test to make the bot OK.
    Someone from Chromium land will care about that then.
    http://trac.webkit.org/wiki/TestExpectations

> Source/WebCore/rendering/RenderFieldset.cpp:165
> +        LayoutUnit clipWidth = (max(static_cast<LayoutUnit>(style()->borderLeftWidth()), legend->width()));

Apparently this change isn't required.

[SravanKumar S(:sravan)]

Created attachment 127933 [details]
Patch to fix the clipping issue and updated chromiums test expectation file.

Thanks a lot for the comments. I have fixed all the issues mentioned by you, and updated test_expectations.txt file of chromium platform.

[SravanKumar S(:sravan)]

Created attachment 128107 [details]
Updated patch as there was merge conflict in test_expectations.txt file

[WebKit Review Bot]

Comment on attachment 128107 [details]
Updated patch as there was merge conflict in test_expectations.txt file

Attachment 128107 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11520393

[SravanKumar S(:sravan)]

Hi Morita,

I have done the changes as mentioned by you, but it still looks like i am missing something. Can you please suggest me why i am seeing following line(in output)

"Regressions: Unexpected no expected results found : (1)
  fast/forms/fieldset-legend-padding-unclipped-fieldset-border.html = MISSING"

though i have attached the expected results file.

[Dirk Pranke]

(In reply to comment #14)
> Hi Morita,
> 
> I have done the changes as mentioned by you, but it still looks like i am missing something. Can you please suggest me why i am seeing following line(in output)
> 
> "Regressions: Unexpected no expected results found : (1)
>   fast/forms/fieldset-legend-padding-unclipped-fieldset-border.html = MISSING"
> 
> though i have attached the expected results file.

The error message in this case might be a bit under-informative; your test is not a text-only test (i.e., it will generate a PNG file), but there is no -expected.png file in your patch as far as I can see. I suspect the bot is complaining about that.

[SravanKumar S(:sravan)]

Created attachment 128403 [details]
Patch with -expected png file.

Thanks for the info, I have generated the -expected png file for the test and uploading the new patch.

[SravanKumar S(:sravan)]

Created attachment 128469 [details]
Patch after resolving merge conflict on chromium/test_expectations.txt

[SravanKumar S(:sravan)]

Created attachment 128476 [details]
Patch after resolving merge conflict on chromium/test_expectations.txt and removing style issue

[WebKit Review Bot]

Comment on attachment 128476 [details]
Patch after resolving merge conflict on chromium/test_expectations.txt and removing style issue

Attachment 128476 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11610266

New failing tests:
fast/forms/fieldset-legend-padding-unclipped-fieldset-border.html

[SravanKumar S(:sravan)]

Hi,

I do not see in the attached output that the test is failing, but in the last comments i see that test is resulting in regression, can some please tell me whats going on, i have spent more time with patch than on bug fix. 

I just want to know where i am going terribly wrong that its almost 4 days and cr-linux bot is still RED.

Thanks in anticipation.

[WebKit Commit Bot]

Attachment 127692 [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

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.

[SravanKumar S(:sravan)]

Hi,

The error report does'nt really show on what file did it apply check-webkit-style, so what kind of bug should i file with this information?.

[Dirk Pranke]

(In reply to comment #22)
> Hi,
> 
> The error report does'nt really show on what file did it apply check-webkit-style, so what kind of bug should i file with this information?.

Hi,

I would file an error along the lines of "style bot failed but check-webkit-style didn't report any errors" :).

Also, I have no explanation for the "new failing test" result you got in comment #19, since the link to http://queues.webkit.org/results/11610266 doesn't even mention that file.

Adam, Eric, either of you have any idea what's going on with the bots here?

SravanKumar, it might be worth trying again ... unfortunately, there's no "retry" button on the bugzilla form, so you would need to clear the review? flag from your attachment and re-upload it. Sorry!

I realize this whole process is a bit frustrating; there is definitely still a learning curve to getting all of the contributor tools working for you, even when you don't hit bugs.

[Adam Barth]

Sorry for the noise.  I'm working on replacing some guts in the style-queue and was running some experiments today.

[SravanKumar S(:sravan)]

:), some response in between is tempting enough to keep persisting with trials.
Ok, any how before i upload the patch again, i need one quick clarification, one of the patch has following lines in it, will it go through style bot?

Index: LayoutTests/fast/forms/fieldset-legend-padding-unclipped-fieldset-border-expected.png
===================================================================
Cannot display: file marked as a binary type.
svn:mime-type = image/png
Index: LayoutTests/fast/forms/fieldset-legend-padding-unclipped-fieldset-border-expected.png
===================================================================
- LayoutTests/fast/forms/fieldset-legend-padding-unclipped-fieldset-border-expected.png (revision 0)
+++ LayoutTests/fast/forms/fieldset-legend-padding-unclipped-fieldset-border-expected.png (working copy)

Property changes on: LayoutTests/fast/forms/fieldset-legend-padding-unclipped-fieldset-border-expected.png
___________________________________________________________________
Added: svn:mime-type
## -0,0 +1 ##
+image/png

I am sure it wont go through, so i 
1. back up -expected.png
2. svn delete "xyz-expected.png"
3. touch "xyz-expected.png"
4. svn add "xyz-expected.png"
5. re-baseline to regenerate "xyz-expected.png"
6. Now re-generate the patch again,

Then i dont see above lines in the patch, but patch size is increased and png file has some non-readable characters in patch file. And i upload this file as patch.

So based on my above explanation can you let me know how to counter these issues and create a safe patch and upload it. I am using cygwin(CYGWIN_NT-6.1-WOW64) svn1.7 on Win7

[SravanKumar S(:sravan)]

Apart from that last query, 

Adam,
please let me know when is it safe to upload the patch?

[Adam Barth]

> please let me know when is it safe to upload the patch?

Please feel free to upload patches whenever you want.

[SravanKumar S(:sravan)]

Created attachment 128985 [details]
Patch with svn mime type set

[SravanKumar S(:sravan)]

Created attachment 129027 [details]
Patch

[SravanKumar S(:sravan)]

Hi,

i found that when we run 'svn-create-patch', this script executes svn diff on the png file in one of the iterations. When i tried to do the same exclusively from cygwin command line and normal command prompt,
i.e,
svn diff --diff-cmd diff LayoutTests/platform/win/fast/forms/fieldset-legend-
padding-unclipped-fieldset-border-expected.png

i see that following as the footer
___________________________________________________________________
Added: svn:mime-type
## -0,0 +1 ##
+image/png

And on chasing .Tools/Scripts/svn-apply i found that script is actually dying because it does'nt get propertyValue(from VCSUtils.pm file), as the condition in while loop is not getting satisisfied (while (defined($_) && /$svnPropertyValueStartRegEx/) {). It is expecting any one of /^   (\+|-|Merged|Reverse-merged) ([^\r\n]+)/  But $_ is ## -0,0 +1 ##.

But since this is a new file it will never be Merged or Reverse-merged, and i dont see any + or - with mime-type:image/png .

Hence my patch will never go through style bots, and i am sorry for uploading so many failing patches.

I just want to know if my cygwin settings are wrong? or svn configuration settings are wrong? or do i have to leave this and find an alternate method to upload the patch?.

Please kindly let me know what else can be done to upload the patch.

[SravanKumar S(:sravan)]

Created attachment 129674 [details]
Created patch on MAC

Footer issue found previously is not visible when patch is generated through MAC Machine(LION.)

[SravanKumar S(:sravan)]

Hi,

Finally i am able to see the bots "Green" :-), can some one please kindly review it?

[WebKit Review Bot]

Comment on attachment 129674 [details]
Created patch on MAC

Rejecting attachment 129674 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
pped-fieldset-border.html
patching file LayoutTests/platform/chromium/test_expectations.txt
Hunk #1 FAILED at 4437.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file LayoutTests/platform/win/fast/forms/fieldset-legend-padding-unclipped-fieldset-border-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Dirk Pranke']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11800073

[Dirk Pranke]

Committed r109612: <http://trac.webkit.org/changeset/109612>

[Dirk Pranke]

Comment on attachment 129674 [details]
Created patch on MAC

landed by hand as http://trac.webkit.org/changeset/109612 .

[SravanKumar S(:sravan)]

Thanks a lot for landing the patch, and i have filed a bug on svn footer issue, with decent information at https://bugs.webkit.org/show_bug.cgi?id=80104.