Bug 78684

Summary: Layout issue with fieldset legend element
Product: WebKit Reporter: SravanKumar S(:sravan) <ssandela>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: CLOSED FIXED    
Severity: Normal CC: abarth, commit-queue, dglazkov, dpranke, eric, kbolisetty, mrahaman, ssandela, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case to validate the buggy behaviour described above
none
Patch to fix the clipping issue.
webkit.review.bot: commit-queue-
Attachment showing actual output
none
Correct attachment showing un-clipped borders for fieldset-stretch-to-legend.html Test case
none
New patch that updates previously failing pixel tests
morrita: review-, webkit.review.bot: commit-queue-
Patch to fix the clipping issue and updated chromiums test expectation file.
none
Updated patch as there was merge conflict in test_expectations.txt file
webkit.review.bot: commit-queue-
Patch with -expected png file.
none
Patch after resolving merge conflict on chromium/test_expectations.txt
none
Patch after resolving merge conflict on chromium/test_expectations.txt and removing style issue
webkit.review.bot: commit-queue-
Patch with svn mime type set
none
Patch
none
Created patch on MAC none

Description SravanKumar S(:sravan) 2012-02-15 01:46:20 PST
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.
Comment 1 Alexey Proskuryakov 2012-02-15 10:57:30 PST
> 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.
Comment 2 SravanKumar S(:sravan) 2012-02-15 17:22:32 PST
Thanks for the info. Will upload the patch, once layout test is ready.
Comment 3 SravanKumar S(:sravan) 2012-02-17 05:12:53 PST
Created attachment 127574 [details]
Patch to fix the clipping issue.

Patch to fix the clipping issue.
Comment 4 WebKit Review Bot 2012-02-17 08:36:16 PST
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
Comment 5 SravanKumar S(:sravan) 2012-02-17 20:41:39 PST
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.
Comment 6 SravanKumar S(:sravan) 2012-02-17 20:45:39 PST
Comment on attachment 127691 [details]
Attachment showing actual output

Incorrect format
Comment 7 SravanKumar S(:sravan) 2012-02-17 20:55:39 PST
Created attachment 127692 [details]
Correct attachment showing un-clipped borders for fieldset-stretch-to-legend.html Test case
Comment 8 SravanKumar S(:sravan) 2012-02-20 02:44:48 PST
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
Comment 9 WebKit Review Bot 2012-02-20 03:50:42 PST
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
Comment 10 Hajime Morrita 2012-02-20 21:52:43 PST
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.
Comment 11 SravanKumar S(:sravan) 2012-02-21 01:20:08 PST
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.
Comment 12 SravanKumar S(:sravan) 2012-02-21 19:05:21 PST
Created attachment 128107 [details]
Updated patch as there was merge conflict in test_expectations.txt file
Comment 13 WebKit Review Bot 2012-02-21 23:32:47 PST
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
Comment 14 SravanKumar S(:sravan) 2012-02-22 00:54:32 PST
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.
Comment 15 Dirk Pranke 2012-02-22 17:52:43 PST
(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.
Comment 16 SravanKumar S(:sravan) 2012-02-22 23:14:37 PST
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.
Comment 17 SravanKumar S(:sravan) 2012-02-23 06:13:04 PST
Created attachment 128469 [details]
Patch after resolving merge conflict on chromium/test_expectations.txt
Comment 18 SravanKumar S(:sravan) 2012-02-23 06:56:21 PST
Created attachment 128476 [details]
Patch after resolving merge conflict on chromium/test_expectations.txt and removing style issue
Comment 19 WebKit Review Bot 2012-02-23 08:07:17 PST
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
Comment 20 SravanKumar S(:sravan) 2012-02-23 17:41:42 PST
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.
Comment 21 WebKit Commit Bot 2012-02-23 18:36:30 PST
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.
Comment 22 SravanKumar S(:sravan) 2012-02-23 20:30:21 PST
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?.
Comment 23 Dirk Pranke 2012-02-23 22:18:41 PST
(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.
Comment 24 Adam Barth 2012-02-23 23:38:14 PST
Sorry for the noise.  I'm working on replacing some guts in the style-queue and was running some experiments today.
Comment 25 SravanKumar S(:sravan) 2012-02-23 23:40:56 PST
:), 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
Comment 26 SravanKumar S(:sravan) 2012-02-24 06:57:53 PST
Apart from that last query, 

Adam,
please let me know when is it safe to upload the patch?
Comment 27 Adam Barth 2012-02-24 10:10:33 PST
> please let me know when is it safe to upload the patch?

Please feel free to upload patches whenever you want.
Comment 28 SravanKumar S(:sravan) 2012-02-27 01:47:22 PST
Created attachment 128985 [details]
Patch with svn mime type set
Comment 29 SravanKumar S(:sravan) 2012-02-27 06:22:47 PST
Created attachment 129027 [details]
Patch
Comment 30 SravanKumar S(:sravan) 2012-02-27 08:18:31 PST
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.
Comment 31 SravanKumar S(:sravan) 2012-03-01 02:54:50 PST
Created attachment 129674 [details]
Created patch on MAC

Footer issue found previously is not visible when patch is generated through MAC Machine(LION.)
Comment 32 SravanKumar S(:sravan) 2012-03-02 03:12:54 PST
Hi,

Finally i am able to see the bots "Green" :-), can some one please kindly review it?
Comment 33 WebKit Review Bot 2012-03-02 11:46:37 PST
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
Comment 34 Dirk Pranke 2012-03-02 13:59:17 PST
Committed r109612: <http://trac.webkit.org/changeset/109612>
Comment 35 Dirk Pranke 2012-03-02 14:00:00 PST
Comment on attachment 129674 [details]
Created patch on MAC

landed by hand as http://trac.webkit.org/changeset/109612 .
Comment 36 SravanKumar S(:sravan) 2012-03-02 21:33:38 PST
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.