Bug 114862

Summary: [EFL]<select> element's text is clipped when a height is specified along with CSS line-height.
Product: WebKit Reporter: Rashmi Kulakarni <rashmi.vijay>
Component: FormsAssignee: Rashmi Kulakarni <rashmi.vijay>
Status: RESOLVED FIXED    
Severity: Normal CC: arpitabahuguna, gyuyoung.kim, pravind, ryuan.choi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
This is test case in which the element height specified is less that line height computed.
none
Patch
none
Patch with comments incorporated.
none
simple select test page
none
Patch
none
Patch with style corrected
none
Patch
none
Patch with comments incorporated.
none
Patch
none
Patch
none
Manual Patch file
none
Final Patch
none
Patch
none
Patch with ChangeLog updated
none
Patch
none
Patch
none
Patch none

Description Rashmi Kulakarni 2013-04-19 02:35:45 PDT
Created attachment 198830 [details]
This is test case in which the element height specified is less that line height computed.

Consider a test case where in height of the element is mentioned along with the CSS line-height.The issue is seen when element's height becomes less than the CSS line-height (computed) when certain amount padding gets applied when EFL theme is applied,the text gets clipped.
This issue can be seen in live sites like:
http://www.britishairways.com
http://www.easemytrip.com
Also I have attached simple test case which depicts this.
Comment 1 Rashmi Kulakarni 2013-04-19 02:38:00 PDT
Comment on attachment 198830 [details]
This is test case in which the element height specified is less that line height computed.

>This tests that select element's text gets clipped when height is specified along with line-height for styled popup buttons.<br>
><select style="background-color:lightblue;font-size:18px;line-height:7px;height:26px">
><option>This text should not be clipped.
></select>
Comment 2 Rashmi Kulakarni 2013-04-19 05:24:34 PDT
Comment on attachment 198830 [details]
This is test case in which the element height specified is less that line height computed.

>This is test case in which the element height specified is less that line height computed..<br>
><select style="background-color:lightblue;font-size:18px;line-height:7px;height:26px;width:200px">
><option>This text should not be clipped.
></select>
Comment 3 Rashmi Kulakarni 2013-04-19 06:32:36 PDT
Created attachment 198849 [details]
Patch
Comment 4 Ryuan Choi 2013-04-22 21:41:29 PDT
Comment on attachment 198849 [details]
Patch

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

I think that you should take care of layout test results for other ports (At least, added new test case in skipped)

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:856
> +    int elemHeight = style->height().intValue();

Webkit like full name.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:859
> +        adjustSizeConstraints(style, ComboBox);

At least, we need right padding for arrow area.
Comment 5 Gyuyoung Kim 2013-04-22 23:46:24 PDT
I wonder if other ports can pass "menulist-line-height_with_element_height.html" on their layout test ? Did you test this on other ports ?  

Beside you miss changelog for your new expectation files.
Comment 6 Rashmi Kulakarni 2013-04-23 06:03:31 PDT
Created attachment 199222 [details]
Patch with comments incorporated.
Comment 7 Ryuan Choi 2013-04-23 22:27:22 PDT
Created attachment 199387 [details]
simple select test page

Please check this test page.
Your patch will break rendering attached <select> examples.
Comment 8 Rashmi Kulakarni 2013-05-10 06:36:38 PDT
Created attachment 201340 [details]
Patch

Thanks Ryuan for pointing out the issues with the patch.
Now handled all the scenarios that occurred in most of the sites with combo boxes. Checked for the minimum padding values to be set for a <select> element text to visible clearly without getting clipped.
Comment 9 Rashmi Kulakarni 2013-05-13 04:08:33 PDT
Created attachment 201545 [details]
Patch with style corrected
Comment 10 Rashmi Kulakarni 2013-05-13 05:00:24 PDT
Created attachment 201546 [details]
Patch
Comment 11 Gyuyoung Kim 2013-05-13 06:52:43 PDT
Comment on attachment 201546 [details]
Patch

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

It looks this patch looks like work-around for specific cases. I think you need to find more general solution.

> Source/WebCore/ChangeLog:3
> +        [EFL][WK2]<select> element's text is clipped when a height is specified along with CSS line-height.

I think this patch is for both EFL WK1 and WK2. We haven't used [WK2] prefix when patch only touch WebCore/platform/efl.

> Source/WebCore/ChangeLog:7
> +

Missing patch description.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You should mention test cases which covered by this patch. If there is no new test, you can said there is no new test or remove this line.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:856
> +    // Case 1:We need to apply theme specific values for padding only when the top padding padding goes beyond theme padding value defined

AFAIK, Case 1, 2 isn't WebKit style. No need.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:858
> +    if ((style->paddingTop().intValue() > desc->padding.top().intValue() &&  style->appearance() == MenulistButtonPart) || (style->paddingTop().intValue() > desc->padding.top().intValue() &&  style->appearance() == MenulistPart ))

Too long line. line breaking is needed.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:860
> +    // Case 2:We need to handle scenarios when a particular webkit style is sepcified.

ditto.
Comment 12 Gyuyoung Kim 2013-05-14 03:47:02 PDT
Comment on attachment 201546 [details]
Patch

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

I think we're able to use specific padding values as window port if there is reasonable reason. However, this patch uses too work-around conditions looks like you fix specific problem for specific cases. See also other port's implementation. Even win port uses few conditions, for instance, to check height, direction or bgcolor, probably, it might be based on reasonable reason. But, you don't explain why we should use those conditions detailed. Please consider we should set padding values according to hacky conditions again.

>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:858
>> +    if ((style->paddingTop().intValue() > desc->padding.top().intValue() &&  style->appearance() == MenulistButtonPart) || (style->paddingTop().intValue() > desc->padding.top().intValue() &&  style->appearance() == MenulistPart ))
> 
> Too long line. line breaking is needed.

I don't see why do you handle MenulistButtonPart in here. Can't we handle it in adjustMenuListButtonStyle() ?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:861
> +    else if (style->appearance() == MenulistPart || style->appearance() == MenulistButtonPart) {

Ditto. Why should we handle MenulistPart and MenulistButtonPart together in adjustMenuListStyle() ?
Comment 13 Rashmi Kulakarni 2013-05-17 05:22:56 PDT
Created attachment 202066 [details]
Patch with comments incorporated.

In this patch, As per you comments, the patch handles cases related to menulist and menulist-button style separately with specific padding values.I have added comments as required.Each port handles them as special cases.Also I have set a control font size for EFL port for <select> element's text to display without getting clipped.Hence the font size change.
Comment 14 Gyuyoung Kim 2013-05-20 01:30:32 PDT
Comment on attachment 202066 [details]
Patch with comments incorporated.

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:-858
> -

Do not modify a line unrelated to this patch.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:879
> +    if (style->appearance() == MenulistPart || style->appearance() == MenulistButtonPart) {

As far as I know, this function can be called when appearance is MenulistButtonPart. So, this condition is not meaningful.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:880
> +        style->setPaddingTop(Length(dropDownBoxPaddingTop , Fixed));

Do not add a space in front of ,

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:881
> +        style->setPaddingRight(Length(dropDownBoxPaddingRight , Fixed));

ditto

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:882
> +        style->setPaddingBottom(Length(dropDownBoxPaddingBottom , Fixed));

ditto

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:883
> +        style->setPaddingLeft(Length(dropDownBoxPaddingLeft , Fixed));

ditto

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:884
> +        }

Wrong indentation.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:886
> +    // We need to set a control font size for EFL port when the font size specified in the content goes beyond certain value(i.e 12px)

ditto.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:888
> +        } 

ditto.
Comment 15 Rashmi Kulakarni 2013-05-23 06:53:36 PDT
Created attachment 202704 [details]
Patch

Thanks for your comments Gyuyoung, I have fixed the issue related to  <select> element's height issue when it becomes less than line height.As discussed with you, I will raise another bug for padding issues for combo boxes.Hence in this patch I will put only those changes related to the bug I raised initially and not the padding changes as it needs to be handled separately like other ports in EFL as well.
Comment 16 Ryuan Choi 2013-05-23 08:27:18 PDT
Comment on attachment 202704 [details]
Patch

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

I think that we should check efl layout test not to have more regression.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:872
> +        style->setFontSize(RENDER_THEME_EFL_CONTROL_FONT_SIZE);

I think that we should more test about this.

When increased font size, Our results are fixed to 13 but others (chrome and ff on linux) looks different.

And your changelog looks missing about this, do you really want to change this for bug?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:874
> +        style->setHeight(Length(Auto));

Indentation is one of important coding style rules.

please fix them.
Comment 17 Rashmi Kulakarni 2013-05-24 06:55:22 PDT
Created attachment 202813 [details]
Patch

Thanks Ryuan for your comments.Rebaselined the test case afer this fix and like you pointed out font size change should go in as part of different patch as part of refactoring theme logic for EFL.Control size for font needs to set in EFL as well like other ports.Hence this pacth includes only height realted changes.
Comment 18 Ryuan Choi 2013-05-24 17:07:44 PDT
Comment on attachment 202813 [details]
Patch

You still ignore coding style.

When you make a patch,
1. rebase first.
2. check your coding style(http://www.webkit.org/coding/coding-style.html). At least you can run ./Tools/Scripts/check-webkit-style
Comment 19 Rashmi Kulakarni 2013-05-26 21:47:29 PDT
Created attachment 202950 [details]
Manual Patch file
Comment 20 Rashmi Kulakarni 2013-05-26 22:06:18 PDT
Created attachment 202951 [details]
Final Patch
Comment 21 Gyuyoung Kim 2013-05-26 22:20:42 PDT
Comment on attachment 202951 [details]
Final Patch

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

Looks much better than before. I still wonder if this patch will be influence on whole EFL layout test. Could you check if there is no difference with/wo this patch on whole EFL layout test ?

> LayoutTests/ChangeLog:7
> +

It would be good if you say this test need to be rebaseline.

> Source/WebCore/ChangeLog:8
> +        Tests: rebaselined LayoutTests/platform/efl/fast/forms/001.html

Generally, "Test: bla bla" is placed below description in Changelog.

> Source/WebCore/ChangeLog:11
> +        height for <select> element

Missing '.' at the end of line.
Comment 22 Rashmi Kulakarni 2013-05-29 03:54:20 PDT
Created attachment 203150 [details]
Patch

Thanks Gyuyoung.
Fixed style issues and rebaselined failing test (fast/forms/001.html) and updated expected.txt and expected.png for the same.Verified my patch after the running the layout tests for the EFL port with my patch.Found no regressions due to my patch.
Comment 23 Gyuyoung Kim 2013-05-29 07:09:28 PDT
Comment on attachment 203150 [details]
Patch

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

LGTM otherwise. Please update changelog again.

> LayoutTests/ChangeLog:3
> +        Rebaselined the test case fast/forms/001.html.

Wrong place for description.

> Source/WebCore/ChangeLog:3
> +        Fix <select> element getting clipped when element height becomes less than the line-height.Set minimum

ditto.
Comment 24 Gyuyoung Kim 2013-05-29 07:13:00 PDT
Comment on attachment 203150 [details]
Patch

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

> LayoutTests/ChangeLog:4
> +        [EFL][WK2]<select> element's text is clipped when a height is specified along with CSS line-height.

One more thing. This patch isn't only for EFL WK2. This is for both WK1 and WK2. So, please remove [WK2] prefix as well.
Comment 25 Rashmi Kulakarni 2013-05-29 23:22:25 PDT
Created attachment 203312 [details]
Patch with ChangeLog updated

Done.
Comment 26 Gyuyoung Kim 2013-05-30 20:13:07 PDT
Comment on attachment 203312 [details]
Patch with ChangeLog updated

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

> LayoutTests/ChangeLog:3
> +        [EFL][WK2]<select> element's text is clipped when a height is specified along with CSS line-height.

Remove [WK2] here as well.

> Source/WebCore/ChangeLog:3
> +        [EFL][WK2]<select> element's text is clipped when a height is specified along with CSS line-height.

ditto
Comment 27 Rashmi Kulakarni 2013-06-03 02:00:14 PDT
Created attachment 203561 [details]
Patch
Comment 28 Gyuyoung Kim 2013-06-03 02:11:31 PDT
Comment on attachment 203561 [details]
Patch

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

LGTM.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:869
> +    // Height is locked to auto if height is not specified

Generally, we have used . at the end of comment.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:872
> +    // The <select> box must be at least 12px high for the button to render the text inside the box without clipping

ditto.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:875
> +    // Calculate min-height of the <select> element

ditto.
Comment 29 Gyuyoung Kim 2013-06-03 02:12:34 PDT
Comment on attachment 203561 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Fix <select> element getting clipped when element height becomes less than the line-height.Set minimum

Patch description is placed below Reviewed by NOBODY (OOPS.)
Comment 30 Rashmi Kulakarni 2013-06-03 02:34:00 PDT
Created attachment 203565 [details]
Patch
Comment 31 Rashmi Kulakarni 2013-06-03 02:51:35 PDT
Created attachment 203566 [details]
Patch
Comment 32 WebKit Commit Bot 2013-06-03 03:30:01 PDT
Comment on attachment 203566 [details]
Patch

Clearing flags on attachment: 203566

Committed r151100: <http://trac.webkit.org/changeset/151100>