Bug 114862 – [EFL]<select> element's text is clipped when a height is specified along with CSS line-height.

• Component: Forms
• Status: RESOLVED FIXED

[Rashmi Kulakarni]

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.

[Rashmi Kulakarni]

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>

[Rashmi Kulakarni]

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>

[Rashmi Kulakarni]

Created attachment 198849 [details]
Patch

[Ryuan Choi]

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.

[Gyuyoung Kim]

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.

[Rashmi Kulakarni]

Created attachment 199222 [details]
Patch with comments incorporated.

[Ryuan Choi]

Created attachment 199387 [details]
simple select test page

Please check this test page.
Your patch will break rendering attached <select> examples.

[Rashmi Kulakarni]

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.

[Rashmi Kulakarni]

Created attachment 201545 [details]
Patch with style corrected

[Rashmi Kulakarni]

Created attachment 201546 [details]
Patch

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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() ?

[Rashmi Kulakarni]

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.

[Gyuyoung Kim]

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.

[Rashmi Kulakarni]

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.

[Ryuan Choi]

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.

[Rashmi Kulakarni]

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.

[Ryuan Choi]

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

[Rashmi Kulakarni]

Created attachment 202950 [details]
Manual Patch file

[Rashmi Kulakarni]

Created attachment 202951 [details]
Final Patch

[Gyuyoung Kim]

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.

[Rashmi Kulakarni]

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.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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.

[Rashmi Kulakarni]

Created attachment 203312 [details]
Patch with ChangeLog updated

Done.

[Gyuyoung Kim]

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

[Rashmi Kulakarni]

Created attachment 203561 [details]
Patch

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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.)

[Rashmi Kulakarni]

Created attachment 203565 [details]
Patch

[Rashmi Kulakarni]

Created attachment 203566 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 203566 [details]
Patch

Clearing flags on attachment: 203566

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