Bug 134674 - [EFL][WK2] Add a "focus,notfound" signal.
Summary: [EFL][WK2] Add a "focus,notfound" signal.
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-06 21:44 PDT by Sanghyup Lee
Modified: 2014-09-23 00:35 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.86 KB, patch)
2014-07-06 21:52 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (9.45 KB, patch)
2014-07-07 22:43 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (11.46 KB, patch)
2014-07-16 02:48 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (11.41 KB, patch)
2014-07-16 03:04 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (12.17 KB, patch)
2014-07-16 21:04 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (11.56 KB, patch)
2014-07-16 23:03 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (11.55 KB, patch)
2014-07-16 23:12 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (11.43 KB, patch)
2014-07-16 23:36 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch for landing (11.45 KB, patch)
2014-07-16 23:47 PDT, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch for landing (11.45 KB, patch)
2014-07-17 03:15 PDT, Sanghyup 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 Sanghyup Lee 2014-07-06 21:44:07 PDT
Add a "focus,control,handover" signal to report that user can take the focus from the ewk_view.
Comment 1 Sanghyup Lee 2014-07-06 21:52:08 PDT
Created attachment 234477 [details]
Patch
Comment 2 Ryuan Choi 2014-07-06 21:56:56 PDT
Comment on attachment 234477 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Add a "focus,control,handover" signal to handover its focus control because there are no elements 
> +        of webview to focus on the given direction.

Can we test this?

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:103
> +    Ewk_Focus_Direction direction = static_cast<Ewk_Focus_Direction>(wkDirection);

Nowadays, we didn't just cast WK enum to ewk enum.
Please use switch or if statement.

> Source/WebKit2/UIProcess/efl/PageUIClientEfl.cpp:104
> +    toPageUIClientEfl(clientInfo)->m_view->smartCallback<FocusControlHandover>().call(&direction);

Why & ?
You just have to pass direction.
Comment 3 Gyuyoung Kim 2014-07-06 22:02:26 PDT
Comment on attachment 234477 [details]
Patch

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

> Source/WebKit2/ChangeLog:11
> +        Remove partial implementation of PageUIClientEfl::takeFocus cause focused evas object has lost 

"Remove partial implementation of PageUIClientEfl::takeFocus cause focused evas object has lost the focus when the other evas object take the focus."

I don't understand this sentence well ?
Comment 4 Sanghyup Lee 2014-07-07 22:43:37 PDT
Created attachment 234546 [details]
Patch
Comment 5 WebKit Commit Bot 2014-07-07 22:45:27 PDT
Attachment 234546 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:151:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Sanghyup Lee 2014-07-16 02:48:59 PDT
Created attachment 234987 [details]
Patch
Comment 7 Gyuyoung Kim 2014-07-16 02:59:13 PDT
Comment on attachment 234987 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:250
> +/*

Why comment out below test ?
Comment 8 Sanghyup Lee 2014-07-16 03:02:04 PDT
(In reply to comment #7)
> (From update of attachment 234987 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234987&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:250
> > +/*
> 
> Why comment out below test ?

Opps, It's my mistake. I'll fix it.
Comment 9 Sanghyup Lee 2014-07-16 03:04:04 PDT
Created attachment 234990 [details]
Patch
Comment 10 Gyuyoung Kim 2014-07-16 03:17:52 PDT
Comment on attachment 234990 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:42
> + * - "focus,control,handover", Ewk_Focus_Direction*: reports that user can take the focus from the ewk_view because

This signal is to notify if focus direction is changed. EWK_FOCUS_DIRECTION_FORWARD or EWK_FOCUS_DIRECTION_BACKWARD. This signal name doesn't contain meaning for user. "focus,direction,changed" looks better. Besides your description regarding "handover focus control bla bla..." is redundant. We only need to explain what does this signal report.
Comment 11 Sanghyup Lee 2014-07-16 17:29:57 PDT
(In reply to comment #10)
> (From update of attachment 234990 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234990&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:42
> > + * - "focus,control,handover", Ewk_Focus_Direction*: reports that user can take the focus from the ewk_view because
> 
> This signal is to notify if focus direction is changed. EWK_FOCUS_DIRECTION_FORWARD or EWK_FOCUS_DIRECTION_BACKWARD. This signal name doesn't contain meaning for user. "focus,direction,changed" looks better. Besides your description regarding "handover focus control bla bla..." is redundant. We only need to explain what does this signal report.

IMHO, This signal is to notify that there is no focusable element when we press the tab or tab + shift keys regardless of focus direction.
Purpose of this signal is to notify that application can handover focus control.
Therefore I think this signal name is not bad but you still prefer to "focus,direction,changed", I'll fix it.
Comment 12 Gyuyoung Kim 2014-07-16 17:44:11 PDT
Comment on attachment 234990 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:42
>>> + * - "focus,control,handover", Ewk_Focus_Direction*: reports that user can take the focus from the ewk_view because
>> 
>> This signal is to notify if focus direction is changed. EWK_FOCUS_DIRECTION_FORWARD or EWK_FOCUS_DIRECTION_BACKWARD. This signal name doesn't contain meaning for user. "focus,direction,changed" looks better. Besides your description regarding "handover focus control bla bla..." is redundant. We only need to explain what does this signal report.
> 
> IMHO, This signal is to notify that there is no focusable element when we press the tab or tab + shift keys regardless of focus direction.
> Purpose of this signal is to notify that application can handover focus control.
> Therefore I think this signal name is not bad but you still prefer to "focus,direction,changed", I'll fix it.

Doesn't Application get focus direction eventually ? Anyway, I think we should use a signal name from the signal occurrence of view.

Look at existing signals

"title,changed",
"url,changed",
"webprocess,crashed",

If this signal is for notifying there isn't any focusable element, it looks "focus,notfound" can be one of candidates.
Comment 13 Ryuan Choi 2014-07-16 18:16:08 PDT
(In reply to comment #12)
> (From update of attachment 234990 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234990&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:42
> >>> + * - "focus,control,handover", Ewk_Focus_Direction*: reports that user can take the focus from the ewk_view because
> >> 
> >> This signal is to notify if focus direction is changed. EWK_FOCUS_DIRECTION_FORWARD or EWK_FOCUS_DIRECTION_BACKWARD. This signal name doesn't contain meaning for user. "focus,direction,changed" looks better. Besides your description regarding "handover focus control bla bla..." is redundant. We only need to explain what does this signal report.
> > 
> > IMHO, This signal is to notify that there is no focusable element when we press the tab or tab + shift keys regardless of focus direction.
> > Purpose of this signal is to notify that application can handover focus control.
> > Therefore I think this signal name is not bad but you still prefer to "focus,direction,changed", I'll fix it.
> 
> Doesn't Application get focus direction eventually ? Anyway, I think we should use a signal name from the signal occurrence of view.
> 
> Look at existing signals
> 
> "title,changed",
> "url,changed",
> "webprocess,crashed",
> 
> If this signal is for notifying there isn't any focusable element, it looks "focus,notfound" can be one of candidates.

In my understanding,
this signal is fired when user requests to move the focus as specified direction but webview can't move the focus because webvuew already reached the last focusable element of that direction.

So, ewk_view does not change any state of itself. It just propagate to the container or application to know that it's your turn to treat user's navigation action.

As user scenarios,
applications may decide 
- to move the focus to next widget of ewk_view (normal behavior which I think)
- or to move the first focusable element of webview (recursive focus)
- or to do nothing

Anyway, although I agree with you that this signal name(focus,control,handover) is not usual, I don't have good idea for the better name.
Comment 14 Gyuyoung Kim 2014-07-16 19:04:39 PDT
Comment on attachment 234990 [details]
Patch

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

>>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:42
>>>>> + * - "focus,control,handover", Ewk_Focus_Direction*: reports that user can take the focus from the ewk_view because
>>>> 
>>>> This signal is to notify if focus direction is changed. EWK_FOCUS_DIRECTION_FORWARD or EWK_FOCUS_DIRECTION_BACKWARD. This signal name doesn't contain meaning for user. "focus,direction,changed" looks better. Besides your description regarding "handover focus control bla bla..." is redundant. We only need to explain what does this signal report.
>>> 
>>> IMHO, This signal is to notify that there is no focusable element when we press the tab or tab + shift keys regardless of focus direction.
>>> Purpose of this signal is to notify that application can handover focus control.
>>> Therefore I think this signal name is not bad but you still prefer to "focus,direction,changed", I'll fix it.
>> 
>> Doesn't Application get focus direction eventually ? Anyway, I think we should use a signal name from the signal occurrence of view.
>> 
>> Look at existing signals
>> 
>> "title,changed",
>> "url,changed",
>> "webprocess,crashed",
>> 
>> If this signal is for notifying there isn't any focusable element, it looks "focus,notfound" can be one of candidates.
> 
> In my understanding,
> this signal is fired when user requests to move the focus as specified direction but webview can't move the focus because webvuew already reached the last focusable element of that direction.
> 
> So, ewk_view does not change any state of itself. It just propagate to the container or application to know that it's your turn to treat user's navigation action.
> 
> As user scenarios,
> applications may decide 
> - to move the focus to next widget of ewk_view (normal behavior which I think)
> - or to move the first focusable element of webview (recursive focus)
> - or to do nothing
> 
> Anyway, although I agree with you that this signal name(focus,control,handover) is not usual, I don't have good idea for the better name.

Right, ryuan. it would be nicer if we add your suggestion to signal description.
Comment 15 Sanghyup Lee 2014-07-16 21:04:46 PDT
Created attachment 235050 [details]
Patch
Comment 16 Gyuyoung Kim 2014-07-16 21:22:58 PDT
(In reply to comment #15)
> Created an attachment (id=235050) [details]
> Patch

> Anyway, although I agree with you that this signal name(focus,control,handover) is not usual, I don't have good idea for the better name.

Sanghyup, this patch is still using "focus,control,handover"...
Comment 17 Ryuan Choi 2014-07-16 21:26:40 PDT
Comment on attachment 235050 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1216
> +        "<body><input type=\"text\"></body>";

How about using "autofocus" attribute to simplify test test case?

As the nit, ' looks easier to read that \"

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1223
> +    Ewk_Focus_Direction direction;

direction = EWK_FOCUS_DIRECTION_FORWARD; ?
Comment 18 Sanghyup Lee 2014-07-16 23:03:15 PDT
Created attachment 235051 [details]
Patch
Comment 19 Sanghyup Lee 2014-07-16 23:12:20 PDT
Created attachment 235052 [details]
Patch
Comment 20 Gyuyoung Kim 2014-07-16 23:18:28 PDT
Comment on attachment 235051 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Add a "focus,control,handover" signal to handover focus control to application

focus,control,handover -> focus,notfound ?

> Source/WebKit2/ChangeLog:11
> +        Application can dicide to move the focus to next widget of ewk_view or something else

dicide -> decide

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:56
> +    FocusNotfound,

FocusNotfound -> FocusNotFound

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:42
> + * - "focus,notfound", Ewk_Focus_Direction*: reports that there is no elements of webview to focus on the given

> reports that there is no elements of webview to focus on the given direction because webview already reached the last focusable element. Application can dicide to move the focus to next widget of ewk_view or something else by using this signal.

"reports that there was no element to be focused on the given direction. The user can handle focus behavior using the signal."
Comment 21 Gyuyoung Kim 2014-07-16 23:20:38 PDT
(In reply to comment #20)
> "reports that there was no element to be focused on the given direction. The user can handle focus behavior using the signal."

> "reports that there was no element to be focused on the given direction. The user can handle next focus behavior using the signal."
Comment 22 Sanghyup Lee 2014-07-16 23:36:33 PDT
Created attachment 235055 [details]
Patch
Comment 23 Gyuyoung Kim 2014-07-16 23:39:08 PDT
Comment on attachment 235055 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:328
> +    Evas* evas = evas_object_evas_get(m_webView);

It would be good if you add ASSERT(evas).
Comment 24 Sanghyup Lee 2014-07-16 23:47:59 PDT
Created attachment 235056 [details]
Patch for landing
Comment 25 Sanghyup Lee 2014-07-17 02:56:41 PDT
Comment on attachment 235056 [details]
Patch for landing

Gyuyoung, I'm sorry. Could you please review again?
Comment 26 Gyuyoung Kim 2014-07-17 03:10:33 PDT
(In reply to comment #25)
> (From update of attachment 235056 [details])
> Gyuyoung, I'm sorry. Could you please review again?

I already set r+ed. In that case, you don't need to request review again if you fill ChangeLog with "Reviewed by Gyuyoung Kim". Then, you can just request cq? committer can land.
Comment 27 Sanghyup Lee 2014-07-17 03:15:14 PDT
Created attachment 235061 [details]
Patch for landing
Comment 28 WebKit Commit Bot 2014-07-17 03:56:19 PDT
Comment on attachment 235061 [details]
Patch for landing

Clearing flags on attachment: 235061

Committed r171182: <http://trac.webkit.org/changeset/171182>
Comment 29 WebKit Commit Bot 2014-07-17 03:56:26 PDT
All reviewed patches have been landed.  Closing bug.