fast/forms/range/range-hit-test-with-padding.html is an easy way to reproduce this bug. Not matter the size you put on the padding, the range input looks the same.
The padding is supposed to apply to the area the slider thumb can be moved. = padding - movable area width:100px; padding-left:30px; padding-right:20px ie. [===-----==] width:10px; padding: 0 200px; ie. [--------------------]
Make that width:10px; padding: 0 200px; ie. [========================================]
Created attachment 160700 [details] Patch
Comment on attachment 160700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160700&action=review Who is the right guy for reviewing the Edje code? > Source/WebCore/platform/efl/RenderThemeEfl.cpp:-802 > + } else if (part == MediaSliderThumbPart) { > style->setWidth(Length(mediaSliderThumbWidth, Fixed)); > style->setHeight(Length(mediaSliderThumbHeight, Fixed)); > - } Have you tested this part? > LayoutTests/platform/efl/TestExpectations:108 > +// Testcase assumes a thumb width of 12px, default there uses 29px. there?
Created attachment 160724 [details] Patch Rebased.
CC'ing Leandro, Leandro and Lucas, could you guys take a look this patch ?
Comment on attachment 160724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160724&action=review > Source/WebKit/efl/ChangeLog:10 > + Also rename the image files from knob to thumb to reflect the WebKit > + terminology. Make sense, but how about separating this(with indentation) as other bug? Now, it is little bit difficult to review edc side.
Comment on attachment 160724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160724&action=review > Source/WebCore/platform/efl/RenderThemeEfl.cpp:344 > + if (object->style()->direction() == RTL || type == SliderVertical) I think you need to add comment for this condition. > Source/WebCore/platform/efl/RenderThemeEfl.cpp:345 > + msg->val[0] = 1; ditto ? > Source/WebKit/efl/DefaultTheme/widget/slider/slider.edc:42 > + set_state(PART:"img.thumb", "default", 0.0); Nit: "set_setate(" should shift a space to left side. Other is same. > LayoutTests/platform/chromium-win/fast/forms/input-appearance-height-expected.txt:3 > layer at (0,0) size 800x600 This patch should not influence on other ports.
Comment on attachment 160724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160724&action=review Thanks for reviewing. >> Source/WebKit/efl/ChangeLog:10 >> + terminology. > > Make sense, but how about separating this(with indentation) as other bug? > > Now, it is little bit difficult to review edc side. Indeed. >> LayoutTests/platform/chromium-win/fast/forms/input-appearance-height-expected.txt:3 >> layer at (0,0) size 800x600 > > This patch should not influence on other ports. Oooooops! The default port of the rebaseline server :)
Created attachment 160942 [details] Patch Addressed comments from the review.
*** Bug 93924 has been marked as a duplicate of this bug. ***
Comment on attachment 160942 [details] Patch Clearing flags on attachment: 160942 Committed r126864: <http://trac.webkit.org/changeset/126864>
All reviewed patches have been landed. Closing bug.
*** Bug 93449 has been marked as a duplicate of this bug. ***
Comment on attachment 160942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160942&action=review > Source/WebKit/efl/DefaultTheme/widget/slider/slider.edc:46 > + set_state(PART:"img.thumb", "pressed", 0.0); Though I pointed out wrong indentation in Comment #8, this patch was landed. Below is consistent with existing .edc indentation. if (get_int(isEnabled) == 1) { set_state(PART:"img.thumb", "default", 0.0); if (get_int(isFocused) == 1) { set_state(PART:"img.thumb", "pressed", 0.0); WebKit doesn't like to fix only coding style. So, I think it would good to fix this wrong indentation here again.
Comment on attachment 160942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160942&action=review >> Source/WebKit/efl/DefaultTheme/widget/slider/slider.edc:46 >> + set_state(PART:"img.thumb", "pressed", 0.0); > > Though I pointed out wrong indentation in Comment #8, this patch was landed. Below is consistent with existing .edc indentation. > > if (get_int(isEnabled) == 1) { > set_state(PART:"img.thumb", "default", 0.0); > if (get_int(isFocused) == 1) { > set_state(PART:"img.thumb", "pressed", 0.0); > > WebKit doesn't like to fix only coding style. So, I think it would good to fix this wrong indentation here again. Sorry, I didn't ignore you comment. Shifting the line to the left, as Comment #8 said, would have made it 2 spaces indent. I asked people on the IRC what is the rule for .edc files and I got 3 spaces indent as the answer (which is what rasterman uses). On the other files we have a mixture of 3 and 4 spaces, but none uses 2.
I meant .edc file also needs to adhere WebKit coding style because we decided to use WebKit coding style except for public API which is used by EFL application. So, as you know, we have followed WebKit coding style instead of EFL coding style. Though there are existing .edc files which follow WebKit coding style, if you follow EFL coding style in this patch, this will make confusion from now. I would recommend to follow WebKit coding style in .edc file as well. How do you think about this ? >> Shifting the line to the left "to the left" is wrong. I wanted to shift to the right.
(In reply to comment #17) > I meant .edc file also needs to adhere WebKit coding style because we decided to use WebKit coding style except for public API which is used by EFL application. So, as you know, we have followed WebKit coding style instead of EFL coding style. Though there are existing .edc files which follow WebKit coding style, if you follow EFL coding style in this patch, this will make confusion from now. I would recommend to follow WebKit coding style in .edc file as well. How do you think about this ? Theme files are part of the public API. It should be the starting point for developers embedding WebKit EFL that want to customize the browser look and feel. I really don't expect to see anyone shipping a product using the default theme.
(In reply to comment #18) > Theme files are part of the public API. It should be the starting point for developers embedding WebKit EFL that want to customize the browser look and feel. > > I really don't expect to see anyone shipping a product using the default theme. Though I also don't expect EFL application uses default theme, I think default theme files aren't a part of public API. edj files generated from edc files are just used by RenderThemeEfl class. Of course, the edc files can be referred by application. But, I think EFL application should follow efl coding style instead of edc file for WebKit default theme. Anyway, this is true that our .edc files are using a mixed indentation of 3 and 4 spaces. If you don't want to modify this indentation, it looks I can't enforce to modify it. But, I feel EFL coding style is different from general coding style. So, if possible, I prefer to use WebKit style for WebKit internal.