Bug 92494 – [EFL] Change the scrollbar behavior not to select the text when moving the scrollbar thumb

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Jinwoo Song]

Set mouse pressed to false when the mouse is over the thumb of the scrollbar 
and change the variable name more clearly.

[Jinwoo Song]

Created attachment 154912 [details]
patch

[Jinwoo Song]

Created attachment 155112 [details]
patch2. Remove the unnecessary check statement.

[Gyuyoung Kim]

Why do you only request cq ? Please request r? as well. And, did you check if this patch influences on layout test ? We need to verify it.

[Jinwoo Song]

I'll request r+ for this patch.
I think this does not need layout test because the scrollbar layout tests use the mockup scrollbar and this patch does not change the mockup scrollbar.

[Jinwoo Song]

Created attachment 155113 [details]
patch.

request cq+ and r+.

[Gyuyoung Kim]

(In reply to comment #4)
> I'll request r+ for this patch.
> I think this does not need layout test because the scrollbar layout tests use the mockup scrollbar and this patch does not change the mockup scrollbar.

It seems to me this patch may influence on layout test result. If there are no differences in layout test result or no unskipped test cases, I can review this informally.

See also,
http://trac.webkit.org/wiki/WebKitEFLLayoutTest

[Jinwoo Song]

okay, I'll check the if the regression is happened and let you know.

[Jinwoo Song]

(In reply to comment #6)
> (In reply to comment #4)
> > I'll request r+ for this patch.
> > I think this does not need layout test because the scrollbar layout tests use the mockup scrollbar and this patch does not change the mockup scrollbar.
> 
> It seems to me this patch may influence on layout test result. If there are no differences in layout test result or no unskipped test cases, I can review this informally.
> 
> See also,
> http://trac.webkit.org/wiki/WebKitEFLLayoutTest

I run the layout test and see no regression happened with my patch.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 155113 [details]
patch.

I don't think this is the right place for a "fix".

I might be remembering things wrongly, but I think the current behavior is by design. If you want to change it, you should write another EDC theme.

[Raphael Kubo da Costa (:rakuco)]

CC'ing a few people who might be more familiar with the history behind the current theme/behavior.

[Raphael Kubo da Costa (:rakuco)]

Bug 87950 is related to this one. Technically speaking, this one is even a duplicate of that.

[Jinwoo Song]

(In reply to comment #9)
> (From update of attachment 155113 [details])
> I don't think this is the right place for a "fix".
> 
> I might be remembering things wrongly, but I think the current behavior is by design. If you want to change it, you should write another EDC theme.

Actually, I'm curious about the current behavior is intended thing and by design. Because common users do not expect the text selection while scrolling the thumb.

[Jinwoo Song]

(In reply to comment #11)
> Bug 87950 is related to this one. Technically speaking, this one is even a duplicate of that.

Yes, you are right and I could not find that in advance. But it seems that there is no progress in Bug 87950. If so, I'd like to follow that in this bug.

[Jinwoo Song]

Created attachment 159011 [details]
patch

[Jinwoo Song]

I've changed the title of this patch according to the Kubo's comment. So I propose to change the scrollbar behavior not to select the text while moving the scrollbar thumb. As the event is handled in the ScrollbarEfl.cpp, the evas_object_propagate_events_set() is used not to propagate the event to webview.

[Gyuyoung Kim]

Comment on attachment 159011 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +        while moving the scrollbar thumb with the mouse. But It is not 

s/It/it/g

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:71
> +    ScrollbarEfl* scrollbar = static_cast<ScrollbarEfl*>(data);

This coding style change is not related to this bug. I think it would be good to change when this function is modified.

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:110
> +        evas_object_propagate_events_set(object, EINA_FALSE);

Please use standard false instead of EINA_FALSE.

[Jinwoo Song]

Created attachment 159020 [details]
patch

[Jinwoo Song]

(In reply to comment #16)
> (From update of attachment 159011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159011&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        while moving the scrollbar thumb with the mouse. But It is not 
> 
> s/It/it/g
> 
Done.

> > Source/WebCore/platform/efl/ScrollbarEfl.cpp:71
> > +    ScrollbarEfl* scrollbar = static_cast<ScrollbarEfl*>(data);
> 
> This coding style change is not related to this bug. I think it would be good to change when this function is modified.
> 
Okay, I moved out the codes in this patch.

> > Source/WebCore/platform/efl/ScrollbarEfl.cpp:110
> > +        evas_object_propagate_events_set(object, EINA_FALSE);
> 
> Please use standard false instead of EINA_FALSE.
Done.

[Gyuyoung Kim]

Comment on attachment 159020 [details]
patch

As kubo said, I also think we can decide if this patch is landed after getting original authors's opinions. 

Rafael, Demarchi and Leandro, could you guys take a look this patch ?

[Jinwoo Song]

(In reply to comment #19)
> (From update of attachment 159020 [details])
> As kubo said, I also think we can decide if this patch is landed after getting original authors's opinions. 
> 
> Rafael, Demarchi and Leandro, could you guys take a look this patch ?

Rafael, Demarchi and Leandro, don't you have any feedback?

[Gyuyoung Kim]

Comment on attachment 159020 [details]
patch

I wonder if this patch is still valid.

[Jinwoo Song]

I'll make this bug invalid as there is no agreement and discussions any more. Instead, I'll start to look into the WebKit2/EFL scrollbar implementation and this issue may be discussed in that thread.