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
Created attachment 174126 [details] Patch
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?
(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.
What about this patch? This tooltip is really annoying. :)
(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 :)
Created attachment 180490 [details] Patch
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 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
(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.
ryuan, Could you look the patch again?
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" ?
(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.
Created attachment 180831 [details] Patch
Comment on attachment 180831 [details] Patch LGTM.
(In reply to comment #10) > ryuan, Could you look the patch again? LGTM now.
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 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?)
(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 on attachment 180831 [details] Patch Clearing flags on attachment: 180831 Committed r138697: <http://trac.webkit.org/changeset/138697>
All reviewed patches have been landed. Closing bug.