Bug 94595 – [EFL] Range input ignores padding

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

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.

[Kenneth Rohde Christiansen]

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. [--------------------]

[Kenneth Rohde Christiansen]

Make that

width:10px; padding: 0 200px; ie. [========================================]

[Thiago Marcos P. Santos]

Created attachment 160700 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Thiago Marcos P. Santos]

Created attachment 160724 [details]
Patch

Rebased.

[Gyuyoung Kim]

CC'ing Leandro, Leandro and Lucas, could you guys take a look this patch ?

[Ryuan Choi]

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.

[Gyuyoung Kim]

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.

[Thiago Marcos P. Santos]

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

[Thiago Marcos P. Santos]

Created attachment 160942 [details]
Patch

Addressed comments from the review.

[KwangYong Choi]

*** Bug 93924 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 160942 [details]
Patch

Clearing flags on attachment: 160942

Committed r126864: <http://trac.webkit.org/changeset/126864>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[KwangYong Choi]

*** Bug 93449 has been marked as a duplicate of this bug. ***

[Gyuyoung Kim]

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.

[Thiago Marcos P. Santos]

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.

[Gyuyoung Kim]

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.

[Thiago Marcos P. Santos]

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

[Gyuyoung Kim]

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