Bug 117405 – [EFL]Background Style of element not visible because of default theme style

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Santosh Mahto]

Created attachment 204153 [details]
Test page for issue

The Background-style specified for  form element (e.g <select>, <input type=search>
whose style are choosen from theme are ignored. 
So even though user specifies css background color for element , the default theme always appear for element
ignoring the backgound-color

[Santosh Mahto]

Created attachment 204155 [details]
Patch

[Chris Dumez]

Comment on attachment 204155 [details]
Patch

layout test?

[Gyuyoung Kim]

Comment on attachment 204155 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Missing patch description.

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

Do not touch unrelated this patch.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:892
> +    bool success = paintMenuList(object, info, rect);

Could you explain why paintMenuList() should be called first ?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:898
> +        IntPoint pnt1(maxX - 1 - 15, centerYOfRect - 3);

This logic looks like work-around-ish.

[Santosh Mahto]

(In reply to comment #3)
> (From update of attachment 204155 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204155&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> Missing patch description.
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:-29
> > -
> 
> Do not touch unrelated this patch.
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:892
> > +    bool success = paintMenuList(object, info, rect);
> 
> Could you explain why paintMenuList() should be called first ?
  currently paintMenuList() basically paint the theme style.  since  we have background style specified for theme rendered objects we paint on top of theme painting by css specfic background color. so its like.
       1. render the element  using theme
       2. paint the background color of css if any (added code in this patch)

But once background is painted over the arrow is lost in select menu list.
So below workaround is to paint the arrow

any suggestion??

> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:898
> > +        IntPoint pnt1(maxX - 1 - 15, centerYOfRect - 3);
> 
> This logic looks like work-around-ish.

[Gyuyoung Kim]

Comment on attachment 204155 [details]
Patch

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

>>> Source/WebCore/ChangeLog:7
>>> +
>> 
>> Missing patch description.
> 
> currently paintMenuList() basically paint the theme style.  since  we have background style specified for theme rendered objects we paint on top of theme painting by css specfic background color. so its like.
>        1. render the element  using theme
>        2. paint the background color of css if any (added code in this patch)
> 
> But once background is painted over the arrow is lost in select menu list.
> So below workaround is to paint the arrow
> 
> any suggestion??

Hmm, it looks mac port has similar logic. I think you need to use more general calculation logic instead of hardcoded value. And, you should check if there is problem on layout test when using this patch. BTW, it would be good if you show a case when css background isn't shown on menu list.

[Santosh Mahto]

Created attachment 205565 [details]
Patch

[Ryuan Choi]

Created attachment 205572 [details]
select with background color(patched 205565)

I am not sure whether it is good enough

[Santosh Mahto]

(In reply to comment #7)
> Created an attachment (id=205572) [details]
> select with background color(patched 205565)
> 
> I am not sure whether it is good enough

Hi Ryuan
What's doubt you have in your mind ??

[Santosh Mahto]

The current patch only changes look and feel adjustment (painting only ), none of css property are modified, 
current layout test case handle the background color test.
So no extra layout test case is required.

[Ryuan Choi]

(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=205572) [details] [details]
> > select with background color(patched 205565)
> > 
> > I am not sure whether it is good enough
> 
> Hi Ryuan
> What's doubt you have in your mind ??

This approach is little bit hackish and results looks uglier than other browsers.
Indeed, this intrdocude new limitation that combo.edc bt_combo_clipper part should be contained in widget.

How about adding new description in edc for transparect combo?

(In reply to comment #9)
> The current patch only changes look and feel adjustment (painting only ), none of css property are modified, 
> current layout test case handle the background color test.
> So no extra layout test case is required.

Did you try pixel-tests?

Although we can't enable pixel-tests yet, I want to follow it as possible as we can.

[Gyuyoung Kim]

(In reply to comment #9)
> The current patch only changes look and feel adjustment (painting only ), none of css property are modified, 
> current layout test case handle the background color test.
> So no extra layout test case is required.

You need to run whole layout test with this patch. This function may be influence on other layout test.

[Santosh Mahto]

Created attachment 207386 [details]
Patch

[Santosh Mahto]

Created attachment 207391 [details]
Image :comparison of select element display after patch

[Gyuyoung Kim]

(In reply to comment #13)
> Created an attachment (id=207391) [details]
> Image :comparison of select element display after patch

I think we need to draw a boundary line as other browsers.

[Gyuyoung Kim]

Comment on attachment 207386 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [EFL]Background Style of element not visible because of  default theme style

Nit: [EFL]Background => [EFL] Background ? , of  default theme -> of default theme ?

> Source/WebCore/ChangeLog:7
> +

Missing patch description.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:380
> +        edje_object_signal_emit(entry->edje(), "bg_styled", "");

I think it would be better to add new function to emit new signal as media button behavior.

Look at RenderThemeEfl::emitMediaButtonSignal().

[Santosh Mahto]

(In reply to comment #14)
> (In reply to comment #13)
> > Created an attachment (id=207391) [details] [details]
> > Image :comparison of select element display after patch
> 
> I think we need to draw a boundary line as other browsers.
Isn't it enough without boundary line??. it look fine to me with minimal code adjustment.any suggestion ??

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #13)
> Created an attachment (id=207391) [details]
> Image :comparison of select element display after patch

The "after" image looks like a regression, not an improvement, to me: not only does the background color still overflows the widget's boundaries, but most of the widget is simply not shown at all.

What if you add some variants (or change the existing ones) of the combo_*.png images with a transparent background? This way if the form component sets a specific background color you can switch to some Edje states that use these transparent images and use the background color you want to fill them (and hopefully stop drawing a colored square behind the widget).

[Santosh Mahto]

Created attachment 209955 [details]
Patch

[Santosh Mahto]

Created attachment 209959 [details]
Background display of select element

[Santosh Mahto]

209959 Display after applying id=209955

[Ryuan Choi]

Comment on attachment 209955 [details]
Patch

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

Did you run layout test with pixel-tests?

> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:35
> +        image: "widget/combo/combo_press_button_styled.png" COMP;

Why do you only add these for press state ?

I think that we also need normal (at least).

> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:144
> +		image {

Indentation looks wrong

> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:331
> +        }

Like other signals, we can use show() to change state.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:297
> +void RenderThemeEfl::applyEdjeStateFromForm(Evas_Object* object, ControlStates states, bool isStyled)

ambiguous variable name to me.

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

It looks not bad to me, but you should send new signal without touching current signals.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:379
> +    bool bgStyled = object && object->hasBackground() && object->style()->visitedDependentColor(CSSPropertyBackgroundColor) != Color::white;

I am not sure, but I didn't see this coding style in WebKit.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #19)
> Created an attachment (id=209959) [details]
> Background display of select element

That looks better and is indeed an improvement, but isn't there a way to get rid of the colored rectangle behind the widget?

[Santosh Mahto]

Comment on attachment 209955 [details]
Patch

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

>> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:35
>> +        image: "widget/combo/combo_press_button_styled.png" COMP;
> 
> Why do you only add these for press state ?
> 
> I think that we also need normal (at least).

sorry, combo_styled.png would have better name here.

Actually there are 4 state(hover/focus/press/default) and 4 image change.
When background color is specified I am going for only 1 state as there is not much display difference.
To manipulate the state in background color also I need to double the behaviour in the combo.edc again 
for 4 new states images.
so after your comment i have 3 choice in case of styled combo:
1.No state.
2.Only default(normal).
3. All 4 state 
If 2nd option is ok I would like  go ahead.

>> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:331
>> +        }
> 
> Like other signals, we can use show() to change state.

So again How many state change is enough (0 or 1 or all 4)

>> Source/WebCore/platform/efl/RenderThemeEfl.cpp:379
>> +    bool bgStyled = object && object->hasBackground() && object->style()->visitedDependentColor(CSSPropertyBackgroundColor) != Color::white;
> 
> I am not sure, but I didn't see this coding style in WebKit.

Hmm, will check for something better. probably can use RenderTheme::isControlStyled.

[Santosh Mahto]

(In reply to comment #22)
> (In reply to comment #19)
> > Created an attachment (id=209959) [details] [details]
> > Background display of select element
> 
> That looks better and is indeed an improvement, but isn't there a way to get rid of the colored rectangle behind the widget?
I wanted to stick to original(without background styled display), in older case
The size of combo widget is smaller than select rect. if we want to fill the complete rect with combo widget we should do for normal case(without bg) also.
Incase of styled we can fit the rect but will be biasing with repsect to nonstyled case.

So I am not sure about fitting widget to element rect.
Will ask Ruyan about this

[Ryuan Choi]

(In reply to comment #24)
> (In reply to comment #22)
> > (In reply to comment #19)
> > > Created an attachment (id=209959) [details] [details] [details]
> > > Background display of select element
> > 
> > That looks better and is indeed an improvement, but isn't there a way to get rid of the colored rectangle behind the widget?
> I wanted to stick to original(without background styled display), in older case
> The size of combo widget is smaller than select rect. if we want to fill the complete rect with combo widget we should do for normal case(without bg) also.
> Incase of styled we can fit the rect but will be biasing with repsect to nonstyled case.
> 
> So I am not sure about fitting widget to element rect.
> Will ask Ruyan about this

It's because of current images which combo.edc use. image itself contains unnecessary empty space.

FYI, We have plan to improve image and edc files including combo.

[Ryuan Choi]

(In reply to comment #23)
> (From update of attachment 209955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209955&action=review
> 
> >> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:35
> >> +        image: "widget/combo/combo_press_button_styled.png" COMP;
> > 
> > Why do you only add these for press state ?
> > 
> > I think that we also need normal (at least).
> 
> sorry, combo_styled.png would have better name here.
> 
> Actually there are 4 state(hover/focus/press/default) and 4 image change.
> When background color is specified I am going for only 1 state as there is not much display difference.
> To manipulate the state in background color also I need to double the behaviour in the combo.edc again 
> for 4 new states images.
> so after your comment i have 3 choice in case of styled combo:
> 1.No state.
> 2.Only default(normal).
> 3. All 4 state 
> If 2nd option is ok I would like  go ahead.
> 

maybe, 5 states for combo theme now. (you looks missing disabled).

Our default combo theme draw them as three styles, now.
(pressed,hover,focus are quite similar for default theme)
I know that we need to improve them, but it's not related to your bug.

So, you may need to add at least three descriptions and two images.

> >> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:331
> >> +        }
> > 
> > Like other signals, we can use show() to change state.
> 
> So again How many state change is enough (0 or 1 or all 4)

When we got new signals, we should also consider other signals.
So we'd better to centralize changes to show().

> 
> >> Source/WebCore/platform/efl/RenderThemeEfl.cpp:379
> >> +    bool bgStyled = object && object->hasBackground() && object->style()->visitedDependentColor(CSSPropertyBackgroundColor) != Color::white;
> > 
> > I am not sure, but I didn't see this coding style in WebKit.
> 
> Hmm, will check for something better. probably can use RenderTheme::isControlStyled.

[Santosh Mahto]

Created attachment 210087 [details]
Incomplete_patch

[WebKit Commit Bot]

Attachment 210087 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/efl/DefaultTheme/widget/check.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo_focus_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo_focus_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo_hover_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo_hover_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo_normal_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo_normal_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo_press_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo_press_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo_focus.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo_focus_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo_hover.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo_hover_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo_normal.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo_normal_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo_press.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/combo_press_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo1/icon.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo_focus.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo_focus_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo_hover.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo_hover_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo_normal.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo_normal_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo_press.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/combo_press_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_2/icon.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_focus.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_focus1.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_focus_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_focus_button1.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_hover.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_hover_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_normal.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_normal2.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_normal_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_normal_button1.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_press.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/combo_press_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_bkp/icon.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo_focus.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo_focus_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo_hover.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo_hover_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo_normal.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo_normal_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo_press.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/combo_press_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final/icon.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo_focus_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo_focus_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo_hover_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo_hover_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo_normal_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo_normal_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo_press_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/combo_press_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_final_done/icon.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_focus.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_focus_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_focus_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_focus_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_hover.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_hover_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_hover_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_hover_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_normal.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_normal_bg (copy).png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_normal_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_normal_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_normal_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_press.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_press_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_press_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/combo_press_button_bg.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_final/icon.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo_focus.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo_focus_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo_hover.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo_hover_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo_normal.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo_normal_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo_press.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/combo_press_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/bkp/icon.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/check.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo.edc', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_button_styled.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_focus.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_focus_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_hover.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_hover_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_normal.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_normal_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_press.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_press_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_styled.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/combo_styled_button.png', u'Source/WebCore/platform/efl/DefaultTheme/widget/combo_last_patch/icon.png', u'Source/WebCore/platform/efl/RenderThemeEfl.cpp', u'Source/WebCore/platform/efl/RenderThemeEfl.h', u'Source/cmake/OptionsEfl.cmake']" exit_code: 1
Source/WebCore/platform/efl/RenderThemeEfl.cpp:313:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/efl/RenderThemeEfl.cpp:313:  signals_bg is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/efl/RenderThemeEfl.cpp:317:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/platform/efl/RenderThemeEfl.cpp:319:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/efl/RenderThemeEfl.cpp:320:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/efl/RenderThemeEfl.cpp:321:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/efl/RenderThemeEfl.cpp:322:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 7 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Santosh Mahto]

Created attachment 210282 [details]
Patch

[Ryuan Choi]

Comment on attachment 210282 [details]
Patch

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

> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:104
> +            if(get_int(styled) == 1) {
> +                new state[30];
> +                new Float:v;
> +                get_state(PART:"combo", state, 30, v);
> +                if(strcmp(state, "default")) {
> +                    set_state(PART:"combo", "default_styled", 0.0);
> +                    set_state(PART:"combo_button", "default_styled", 0.0);
> +                }
> +                if(strcmp(state, "disabled")) {
> +                    set_state(PART:"combo", "disabled_styled", 0.0);
> +                    set_state(PART:"combo_button", "disabled_styled", 0.0);
> +                }
> +                if(strcmp(state, "focused")) {
> +                    set_state(PART:"combo", "focused_styled", 0.0);
> +                    set_state(PART:"combo_button", "focused_styled", 0.0);
> +                }
> +                if(strcmp(state, "hovered")) {
> +                    set_state(PART:"combo", "hovered_styled", 0.0);
> +                    set_state(PART:"combo_button", "hovered_styled", 0.0);
> +                }
> +                if(strcmp(state, "pressed")) {
> +                    set_state(PART:"combo", "pressed_styled", 0.0);
> +                    set_state(PART:"combo_button", "pressed_styled", 0.0);
> +                }
> +            }

What if focused with disabled?
I think that some state may have priority.

And I hope that we have more optimized statements including previous conditions.

> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:159
>              description {
> +               state: "disabled" 0.0;
> +               inherit: "default_styled" 0.0;

s/disabled/disabled_styled ?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:318
> +            if (haveBackground)
> +                edje_object_signal_emit(object, "styled", "");

I think this should be out of loop.

> Source/WebCore/rendering/RenderTheme.cpp:680
> +    case MenulistButtonPart:
> +        return true;

Do you really need this?

This is not EFL specific.

> LayoutTests/fast/forms/menulist_background_color_test.html:4
> +<!DOCTYPE html>
> +<html>
> +<body>
> +    <select style="background-color:green">

I think that we don't need new test case.
fast/forms/menulist-style-color.html is enough.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > (In reply to comment #19)
> > > > Created an attachment (id=209959) [details] [details] [details] [details]
> > > > Background display of select element
> > > 
> > > That looks better and is indeed an improvement, but isn't there a way to get rid of the colored rectangle behind the widget?
> > I wanted to stick to original(without background styled display), in older case
> > The size of combo widget is smaller than select rect. if we want to fill the complete rect with combo widget we should do for normal case(without bg) also.
> > Incase of styled we can fit the rect but will be biasing with repsect to nonstyled case.
> > 
> > So I am not sure about fitting widget to element rect.
> > Will ask Ruyan about this
> 
> It's because of current images which combo.edc use. image itself contains unnecessary empty space.
> 
> FYI, We have plan to improve image and edc files including combo.

Isn't it possible to fix this issue here, or merge this patch with the rest of the improvements you guys are planning on making?

[Santosh Mahto]

Created attachment 210368 [details]
Patch

[Santosh Mahto]

> > Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:104
> > +            if(get_int(styled) == 1) {
> > +                new state[30];
> > +                new Float:v;
> > +                get_state(PART:"combo", state, 30, v);
> > +                if(strcmp(state, "default")) {
> > +                    set_state(PART:"combo", "default_styled", 0.0);
> > +                    set_state(PART:"combo_button", "default_styled", 0.0);
> > +                }
> > +                if(strcmp(state, "disabled")) {
> > +                    set_state(PART:"combo", "disabled_styled", 0.0);
> > +                    set_state(PART:"combo_button", "disabled_styled", 0.0);
> > +                }
> > +                if(strcmp(state, "focused")) {
> > +                    set_state(PART:"combo", "focused_styled", 0.0);
> > +                    set_state(PART:"combo_button", "focused_styled", 0.0);
> > +                }
> > +                if(strcmp(state, "hovered")) {
> > +                    set_state(PART:"combo", "hovered_styled", 0.0);
> > +                    set_state(PART:"combo_button", "hovered_styled", 0.0);
> > +                }
> > +                if(strcmp(state, "pressed")) {
> > +                    set_state(PART:"combo", "pressed_styled", 0.0);
> > +                    set_state(PART:"combo_button", "pressed_styled", 0.0);
> > +                }
> > +            }
> 
> What if focused with disabled?
In disabled case there is only one case: disabled, no disabled +  focus/press/hover   in original implementation also.
> I think that some state may have priority.
Current patch behavior is no different than original except transparent image.
original implementation has no priority kind of states.

> And I hope that we have more optimized statements including previous conditions.
I did some changes, but cant think of any other optimization until we change complete function show() and I didn't wished to touch original menu list behavior.
Suggestion are welcomed.
> > Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:159
> >              description {
> > +               state: "disabled" 0.0;
> > +               inherit: "default_styled" 0.0;
> 
> s/disabled/disabled_styled ?
my mistake .. corrected.
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:318
> > +            if (haveBackground)
> > +                edje_object_signal_emit(object, "styled", "");
> 
> I think this should be out of loop.
right..
> > Source/WebCore/rendering/RenderTheme.cpp:680
> > +    case MenulistButtonPart:
> > +        return true;
> 
> Do you really need this?
> 
> This is not EFL specific.
I removed in new  patch,Thanks for pointing.
> > LayoutTests/fast/forms/menulist_background_color_test.html:4
> > +<!DOCTYPE html>
> > +<html>
> > +<body>
> > +    <select style="background-color:green">
> 
> I think that we don't need new test case.
> fast/forms/menulist-style-color.html is enough.
ok Killed it..

[Santosh Mahto]

(In reply to comment #31)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #22)
> > > > (In reply to comment #19)
> > > > > Created an attachment (id=209959) [details] [details] [details] [details] [details]
> > > > > Background display of select element
> > > > 
> > > > That looks better and is indeed an improvement, but isn't there a way to get rid of the colored rectangle behind the widget?
> > > I wanted to stick to original(without background styled display), in older case
> > > The size of combo widget is smaller than select rect. if we want to fill the complete rect with combo widget we should do for normal case(without bg) also.
> > > Incase of styled we can fit the rect but will be biasing with repsect to nonstyled case.
> > > 
> > > So I am not sure about fitting widget to element rect.
> > > Will ask Ruyan about this
> > 
> > It's because of current images which combo.edc use. image itself contains unnecessary empty space.
> > 
> > FYI, We have plan to improve image and edc files including combo.
> 
> Isn't it possible to fix this issue here, or merge this patch with the rest of the improvements you guys are planning on making?
It looks better and safe to make separate bugs for "updating Images" and another enhancement because:
It will be easy to work/optimize on small patches and would ensure we don't break current one.
updating images itself will require update of all Layout test expectation. So it will become big patch.
Bug Title also will become ambiguous.
I will try to land other enhancement on  control element asap.

[Ryuan Choi]

(In reply to comment #33)
> > > Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:104
> > > +            if(get_int(styled) == 1) {
> > > +                new state[30];
> > > +                new Float:v;
> > > +                get_state(PART:"combo", state, 30, v);
> > > +                if(strcmp(state, "default")) {
> > > +                    set_state(PART:"combo", "default_styled", 0.0);
> > > +                    set_state(PART:"combo_button", "default_styled", 0.0);
> > > +                }
> > > +                if(strcmp(state, "disabled")) {
> > > +                    set_state(PART:"combo", "disabled_styled", 0.0);
> > > +                    set_state(PART:"combo_button", "disabled_styled", 0.0);
> > > +                }
> > > +                if(strcmp(state, "focused")) {
> > > +                    set_state(PART:"combo", "focused_styled", 0.0);
> > > +                    set_state(PART:"combo_button", "focused_styled", 0.0);
> > > +                }
> > > +                if(strcmp(state, "hovered")) {
> > > +                    set_state(PART:"combo", "hovered_styled", 0.0);
> > > +                    set_state(PART:"combo_button", "hovered_styled", 0.0);
> > > +                }
> > > +                if(strcmp(state, "pressed")) {
> > > +                    set_state(PART:"combo", "pressed_styled", 0.0);
> > > +                    set_state(PART:"combo_button", "pressed_styled", 0.0);
> > > +                }
> > > +            }
> > 
> > What if focused with disabled?
> In disabled case there is only one case: disabled, no disabled +  focus/press/hover   in original implementation also.
> > I think that some state may have priority.
> Current patch behavior is no different than original except transparent image.
> original implementation has no priority kind of states.
> 

OK, I misread it.
Because you compare applied state, priority is meaningless.

View in context: https://bugs.webkit.org/attachment.cgi?id=210368&action=review
> Source/WebCore/platform/efl/DefaultTheme/widget/combo/combo.edc:41
> +        image: "widget/combo/combo_normal_transparent.png" COMP;
> +        image: "widget/combo/combo_normal_button_transparent.png" COMP;
> +        image: "widget/combo/combo_hover_transparent.png" COMP;
> +        image: "widget/combo/combo_hover_button_transparent.png" COMP;
> +        image: "widget/combo/combo_focus_transparent.png" COMP;
> +        image: "widget/combo/combo_focus_button_transparent.png" COMP;
> +        image: "widget/combo/combo_press_transparent.png" COMP;
> +        image: "widget/combo/combo_press_button_transparent.png" COMP;

You also have to add these files into dependency list in Source/WebCore/platform/efl/DefaultTheme/CMakeLists.txt

And did you also check other layout test cases?

Others looks fine to me.

[Ryuan Choi]

(In reply to comment #34)
> (In reply to comment #31)
> > (In reply to comment #25)
> > > (In reply to comment #24)
> > > > (In reply to comment #22)
> > > > > (In reply to comment #19)
> > > > > > Created an attachment (id=209959) [details] [details] [details] [details] [details] [details]
> > > > > > Background display of select element
> > > > > 
> > > > > That looks better and is indeed an improvement, but isn't there a way to get rid of the colored rectangle behind the widget?
> > > > I wanted to stick to original(without background styled display), in older case
> > > > The size of combo widget is smaller than select rect. if we want to fill the complete rect with combo widget we should do for normal case(without bg) also.
> > > > Incase of styled we can fit the rect but will be biasing with repsect to nonstyled case.
> > > > 
> > > > So I am not sure about fitting widget to element rect.
> > > > Will ask Ruyan about this
> > > 
> > > It's because of current images which combo.edc use. image itself contains unnecessary empty space.
> > > 
> > > FYI, We have plan to improve image and edc files including combo.
> > 
> > Isn't it possible to fix this issue here, or merge this patch with the rest of the improvements you guys are planning on making?
> It looks better and safe to make separate bugs for "updating Images" and another enhancement because:
> It will be easy to work/optimize on small patches and would ensure we don't break current one.
> updating images itself will require update of all Layout test expectation. So it will become big patch.
> Bug Title also will become ambiguous.
> I will try to land other enhancement on  control element asap.

This bug is little bit different so I'd like to have new bug for it.
And it will take more time to replace image.

[Santosh Mahto]

Created attachment 210595 [details]
Patch

[Santosh Mahto]

> > +        image: "widget/combo/combo_focus_button_transparent.png" COMP;
> > +        image: "widget/combo/combo_press_transparent.png" COMP;
> > +        image: "widget/combo/combo_press_button_transparent.png" COMP;
> 
> You also have to add these files into dependency list in Source/WebCore/platform/efl/DefaultTheme/CMakeLists.txt
Added..

> And did you also check other layout test cases?

I checked Layout/fast/forms/ related layout test cases and updated the expected image in latest patch.
 
> Others looks fine to me.
:)

[Gyuyoung Kim]

Comment on attachment 210595 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Missing patch description.

> LayoutTests/ChangeLog:7
> +

Missing patch description.

[Santosh Mahto]

Created attachment 211441 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 211441 [details]
Patch

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

LGTM, please fix my trivial comment before landing.

> LayoutTests/ChangeLog:9
> +        background image visible over combo box.

Add a new line.

[Santosh Mahto]

Created attachment 211512 [details]
patch for Landing

[WebKit Commit Bot]

Comment on attachment 211512 [details]
patch for Landing

Clearing flags on attachment: 211512

Committed r155674: <http://trac.webkit.org/changeset/155674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.