Bug 102209 – [EFL][WK2] Fix tooltip bugs in MiniBrowser.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Byungwoo Lee]

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

[Byungwoo Lee]

Created attachment 174126 [details]
Patch

[Ryuan Choi]

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?

[Byungwoo Lee]

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

[Thiago Marcos P. Santos]

What about this patch? This tooltip is really annoying. :)

[Byungwoo Lee]

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

[Byungwoo Lee]

Created attachment 180490 [details]
Patch

[Byungwoo Lee]

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.

[Build Bot]

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

[Byungwoo Lee]

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

[Byungwoo Lee]

ryuan, Could you look the patch again?

[Chris Dumez]

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" ?

[Byungwoo Lee]

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

[Byungwoo Lee]

Created attachment 180831 [details]
Patch

[Chris Dumez]

Comment on attachment 180831 [details]
Patch

LGTM.

[Ryuan Choi]

(In reply to comment #10)
> ryuan, Could you look the patch again?

LGTM now.

[Jussi Kukkonen (jku)]

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.

[Byungwoo Lee]

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

[Jussi Kukkonen (jku)]

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

[WebKit Review Bot]

Comment on attachment 180831 [details]
Patch

Clearing flags on attachment: 180831

Committed r138697: <http://trac.webkit.org/changeset/138697>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.