Bug 102209 - [EFL][WK2] Fix tooltip bugs in MiniBrowser.
Summary: [EFL][WK2] Fix tooltip bugs in MiniBrowser.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Byungwoo Lee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-14 04:03 PST by Byungwoo Lee
Modified: 2013-01-02 20:49 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.73 KB, patch)
2012-11-14 04:20 PST, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (6.93 KB, patch)
2012-12-21 01:23 PST, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (6.94 KB, patch)
2012-12-27 17:11 PST, Byungwoo Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Byungwoo Lee 2012-11-14 04:03:23 PST
1. There is no delay on displaying tooltip.
2. There are elementary error messages when tooltip is created.
   ERR<17980>:elementary elm_label.c:400 elm_label_add() could not add 0x1ca3890 as sub object of 0x1cf9810
Comment 1 Byungwoo Lee 2012-11-14 04:20:03 PST
Created attachment 174126 [details]
Patch
Comment 2 Ryuan Choi 2012-11-18 18:32:13 PST
Comment on attachment 174126 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:916
> +    window->tooltip_timer = ecore_timer_add(1, on_tooltip_show, window);

Can I know the reason we should have delay and why it is 1?

Can we use ecore_idle_add?
Comment 3 Byungwoo Lee 2012-11-18 20:05:19 PST
(In reply to comment #2)
> (From update of attachment 174126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174126&action=review
> 
> > Tools/MiniBrowser/efl/main.c:916
> > +    window->tooltip_timer = ecore_timer_add(1, on_tooltip_show, window);
> 
> Can I know the reason we should have delay and why it is 1?
> 
> Can we use ecore_idle_add?

Because all the other browsers behave like this.

Currently, MiniBrowser always shows the tooltip window when the mouse pointer is over a html element. But most browsers show tooltip when mouse pointer stays on an element for some time.
(I think application can set the delay, and MiniBrowser uses 1 second for the delay in this patch.)

When we use ecore_idle_add() for this, there can be a problem with below issues.

1. elm object tooltip should not be set to the ewk_view but to other elm_object such as elm_window.
(setting tooltip to the ewk_view makes elm error messages while using MiniBrowser)

2. When elm_object_tooltip_show is called, tooltip window is displayed at the current pointer position.

Problem is that, if there is no delay or use ecore_idle_add(), the tooltip window will be mostly displayed at the edge of the html element that has tooltip.
Comment 4 Thiago Marcos P. Santos 2012-11-26 11:34:52 PST
What about this patch? This tooltip is really annoying. :)
Comment 5 Byungwoo Lee 2012-11-26 22:44:06 PST
(In reply to comment #4)
> What about this patch? This tooltip is really annoying. :)

I made a patch to remove the error messages from MiniBrowser.
But ryuan pointed that, there are some another bugs with this patch.
(I think it is about the behavior of tooltip itself)

To implement the tooltip behavior like common browsers,
checking mouse event is required in MiniBrowser.

Because most browsers show the tooltip window when mouse pointer is staying
over an object for some time, and hide tooltip window when mouse pointer
starts to move. (elm tooltip doesn't monitor mouse event to show or hide)

Is it better to do those with this patch?

I think that might be ok to apply this first and fix the tooltip behavior itself later.
(because the current tooltip always returns error messages and follows the mouse pointer)

Please let me know your opinions :)
Comment 6 Byungwoo Lee 2012-12-21 01:23:59 PST
Created attachment 180490 [details]
Patch
Comment 7 Byungwoo Lee 2012-12-21 01:28:02 PST
I uploaded new patch to apply ryuan's opinion.
Now, MiniBrowser checks mouse events to show/hide tooltip.

I re-open the review flag because the patch is changed.
Comment 8 Build Bot 2012-12-21 01:51:04 PST
Comment on attachment 180490 [details]
Patch

Attachment 180490 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15464236

New failing tests:
svg/as-image/img-relative-height.html
Comment 9 Byungwoo Lee 2012-12-25 17:33:07 PST
(In reply to comment #8)
> (From update of attachment 180490 [details])
> Attachment 180490 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15464236
> 
> New failing tests:
> svg/as-image/img-relative-height.html

This is false alarm because the patch only touched main.c of EFL MiniBrowser.
Comment 10 Byungwoo Lee 2012-12-27 08:14:12 PST
ryuan, Could you look the patch again?
Comment 11 Chris Dumez 2012-12-27 08:21:18 PST
Comment on attachment 180490 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:32
> +static const int TOOLTIP_DELAY_SECONDS = 1.0;

int and 1.0 are somewhat conflicting

> Tools/MiniBrowser/efl/main.c:83
> +    Eina_Bool show;

"is_shown" or "shown" for consistency with "activated" ?
Comment 12 Byungwoo Lee 2012-12-27 16:57:52 PST
(In reply to comment #11)
> (From update of attachment 180490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180490&action=review
> 
> > Tools/MiniBrowser/efl/main.c:32
> > +static const int TOOLTIP_DELAY_SECONDS = 1.0;
> 
> int and 1.0 are somewhat conflicting
Oops, Thanks!

> 
> > Tools/MiniBrowser/efl/main.c:83
> > +    Eina_Bool show;
> 
> "is_shown" or "shown" for consistency with "activated" ?
Ok, it'll be better. Will apply it.
Comment 13 Byungwoo Lee 2012-12-27 17:11:24 PST
Created attachment 180831 [details]
Patch
Comment 14 Chris Dumez 2012-12-27 22:20:06 PST
Comment on attachment 180831 [details]
Patch

LGTM.
Comment 15 Ryuan Choi 2013-01-01 17:58:14 PST
(In reply to comment #10)
> ryuan, Could you look the patch again?

LGTM now.
Comment 16 Jussi Kukkonen (jku) 2013-01-02 03:09:53 PST
Comment on attachment 180831 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:225
> +static void
> +on_mouse_move(void *user_data, Evas *e, Evas_Object *ewk_view, void *event_info)
> +{
> +    window_tooltip_update((Browser_Window *)user_data);
> +}

Am I correct that on every mouse move event this adds a new ecore timer?

What should happen is that the existing timer should be reset to TOOLTIP_DELAY_SECONDS.
Comment 17 Byungwoo Lee 2013-01-02 03:20:08 PST
Comment on attachment 180831 [details]
Patch

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

>> Tools/MiniBrowser/efl/main.c:225
>> +}
> 
> Am I correct that on every mouse move event this adds a new ecore timer?
> 
> What should happen is that the existing timer should be reset to TOOLTIP_DELAY_SECONDS.

If there is an existing timer and user moves mouse again, the previous timer will be removed and new timer will be created with the TOOLTIP_DELAY_SECONDS.
I think this is the 'reset' mechanism that you asked. (is it correct?)
Comment 18 Jussi Kukkonen (jku) 2013-01-02 06:45:22 PST
(In reply to comment #17)
> If there is an existing timer and user moves mouse again, the previous timer will be removed and new timer will be created with the TOOLTIP_DELAY_SECONDS.
> I think this is the 'reset' mechanism that you asked. (is it correct?)

You are correct, sorry: I missed the window_tooltip_hide() call.
Comment 19 WebKit Review Bot 2013-01-02 20:49:04 PST
Comment on attachment 180831 [details]
Patch

Clearing flags on attachment: 180831

Committed r138697: <http://trac.webkit.org/changeset/138697>
Comment 20 WebKit Review Bot 2013-01-02 20:49:09 PST
All reviewed patches have been landed.  Closing bug.