Summary: | [EFL]Background Style of element not visible because of default theme style | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Santosh Mahto <santosh.ma> | ||||||||||||||||||||||||||||||
Component: | WebKit EFL | Assignee: | 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: |
|
Created attachment 204155 [details]
Patch
Comment on attachment 204155 [details]
Patch
layout test?
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. (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 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. Created attachment 205565 [details]
Patch
Created attachment 205572 [details]
select with background color(patched 205565)
I am not sure whether it is good enough
(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 ?? 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. (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. (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. Created attachment 207386 [details]
Patch
Created attachment 207391 [details]
Image :comparison of select element display after patch
(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 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(). (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 ?? (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). Created attachment 209955 [details]
Patch
Created attachment 209959 [details]
Background display of select element
209959 Display after applying id=209955 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. (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 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. (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 (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. (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. Created attachment 210087 [details]
Incomplete_patch
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.
Created attachment 210282 [details]
Patch
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. (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? Created attachment 210368 [details]
Patch
> > 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.. (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. (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. (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. Created attachment 210595 [details]
Patch
> > + 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 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. Created attachment 211441 [details]
Patch
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. Created attachment 211512 [details]
patch for Landing
Comment on attachment 211512 [details] patch for Landing Clearing flags on attachment: 211512 Committed r155674: <http://trac.webkit.org/changeset/155674> All reviewed patches have been landed. Closing bug. |
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