Bug 117405

Summary: [EFL]Background Style of element not visible because of default theme style
Product: WebKit Reporter: Santosh Mahto <santosh.ma>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, gyuyoung.kim, kondapallykalyan, lucas.de.marchi, rakuco, ryuan.choi
Priority: P2    
Version: 525.x (Safari 3.2)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test page for issue
none
Patch
none
Patch
none
select with background color(patched 205565)
none
Patch
none
Image :comparison of select element display after patch
none
Patch
none
Background display of select element
none
Incomplete_patch
none
Patch
none
Patch
none
Patch
none
Patch
none
patch for Landing none

Description Santosh Mahto 2013-06-10 05:01:17 PDT
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
Comment 1 Santosh Mahto 2013-06-10 05:06:36 PDT
Created attachment 204155 [details]
Patch
Comment 2 Chris Dumez 2013-06-10 05:52:07 PDT
Comment on attachment 204155 [details]
Patch

layout test?
Comment 3 Gyuyoung Kim 2013-06-10 18:53:48 PDT
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.
Comment 4 Santosh Mahto 2013-06-11 20:08:40 PDT
(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.
Comment 5 Gyuyoung Kim 2013-06-12 19:31:20 PDT
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.
Comment 6 Santosh Mahto 2013-06-26 23:26:51 PDT
Created attachment 205565 [details]
Patch
Comment 7 Ryuan Choi 2013-06-27 00:53:16 PDT
Created attachment 205572 [details]
select with background color(patched 205565)

I am not sure whether it is good enough
Comment 8 Santosh Mahto 2013-07-02 09:39:35 PDT
(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 ??
Comment 9 Santosh Mahto 2013-07-14 22:42:25 PDT
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.
Comment 10 Ryuan Choi 2013-07-15 01:05:54 PDT
(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.
Comment 11 Gyuyoung Kim 2013-07-15 22:11:54 PDT
(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.
Comment 12 Santosh Mahto 2013-07-24 05:16:20 PDT
Created attachment 207386 [details]
Patch
Comment 13 Santosh Mahto 2013-07-24 06:44:14 PDT
Created attachment 207391 [details]
Image :comparison of select element display after patch
Comment 14 Gyuyoung Kim 2013-07-31 22:56:13 PDT
(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.
Comment 15 Gyuyoung Kim 2013-07-31 22:59:17 PDT
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().
Comment 16 Santosh Mahto 2013-08-24 09:38:29 PDT
(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 ??
Comment 17 Raphael Kubo da Costa (:rakuco) 2013-08-24 18:04:50 PDT
(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).
Comment 18 Santosh Mahto 2013-08-29 01:03:56 PDT
Created attachment 209955 [details]
Patch
Comment 19 Santosh Mahto 2013-08-29 01:57:22 PDT
Created attachment 209959 [details]
Background display of select element
Comment 20 Santosh Mahto 2013-08-29 01:59:09 PDT
209959 Display after applying id=209955
Comment 21 Ryuan Choi 2013-08-29 05:40:32 PDT
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.
Comment 22 Raphael Kubo da Costa (:rakuco) 2013-08-29 05:48:33 PDT
(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?
Comment 23 Santosh Mahto 2013-08-29 07:40:26 PDT
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.
Comment 24 Santosh Mahto 2013-08-29 07:55:04 PDT
(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
Comment 25 Ryuan Choi 2013-08-29 16:04:13 PDT
(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.
Comment 26 Ryuan Choi 2013-08-29 21:34:32 PDT
(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.
Comment 27 Santosh Mahto 2013-08-30 04:58:51 PDT
Created attachment 210087 [details]
Incomplete_patch
Comment 28 WebKit Commit Bot 2013-08-30 05:03:25 PDT
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.
Comment 29 Santosh Mahto 2013-09-02 06:48:33 PDT
Created attachment 210282 [details]
Patch
Comment 30 Ryuan Choi 2013-09-02 16:43:13 PDT
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.
Comment 31 Raphael Kubo da Costa (:rakuco) 2013-09-03 04:40:03 PDT
(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?
Comment 32 Santosh Mahto 2013-09-03 06:45:35 PDT
Created attachment 210368 [details]
Patch
Comment 33 Santosh Mahto 2013-09-03 08:41:30 PDT
> > 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..
Comment 34 Santosh Mahto 2013-09-03 08:59:52 PDT
(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.
Comment 35 Ryuan Choi 2013-09-04 18:11:06 PDT
(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.
Comment 36 Ryuan Choi 2013-09-04 18:36:21 PDT
(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.
Comment 37 Santosh Mahto 2013-09-05 03:10:29 PDT
Created attachment 210595 [details]
Patch
Comment 38 Santosh Mahto 2013-09-05 03:24:58 PDT
> > +        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.
:)
Comment 39 Gyuyoung Kim 2013-09-11 20:50:05 PDT
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.
Comment 40 Santosh Mahto 2013-09-12 10:34:30 PDT
Created attachment 211441 [details]
Patch
Comment 41 Gyuyoung Kim 2013-09-12 22:05:42 PDT
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.
Comment 42 Santosh Mahto 2013-09-12 22:57:10 PDT
Created attachment 211512 [details]
patch for Landing
Comment 43 WebKit Commit Bot 2013-09-12 23:41:39 PDT
Comment on attachment 211512 [details]
patch for Landing

Clearing flags on attachment: 211512

Committed r155674: <http://trac.webkit.org/changeset/155674>
Comment 44 WebKit Commit Bot 2013-09-12 23:41:45 PDT
All reviewed patches have been landed.  Closing bug.