Bug 134674 – [EFL][WK2] Add a "focus,notfound" signal.

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

[Sanghyup Lee]

Add a "focus,control,handover" signal to report that user can take the focus from the ewk_view.

[Sanghyup Lee]

Created attachment 234477 [details]
Patch

[Ryuan Choi]

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.

[Gyuyoung Kim]

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 ?

[Sanghyup Lee]

Created attachment 234546 [details]
Patch

[WebKit Commit Bot]

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.

[Sanghyup Lee]

Created attachment 234987 [details]
Patch

[Gyuyoung Kim]

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 ?

[Sanghyup Lee]

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

[Sanghyup Lee]

Created attachment 234990 [details]
Patch

[Gyuyoung Kim]

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.

[Sanghyup Lee]

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

[Gyuyoung Kim]

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.

[Ryuan Choi]

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

[Gyuyoung Kim]

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.

[Sanghyup Lee]

Created attachment 235050 [details]
Patch

[Gyuyoung Kim]

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

[Ryuan Choi]

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

[Sanghyup Lee]

Created attachment 235051 [details]
Patch

[Sanghyup Lee]

Created attachment 235052 [details]
Patch

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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

[Sanghyup Lee]

Created attachment 235055 [details]
Patch

[Gyuyoung Kim]

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

[Sanghyup Lee]

Created attachment 235056 [details]
Patch for landing

[Sanghyup Lee]

Comment on attachment 235056 [details]
Patch for landing

Gyuyoung, I'm sorry. Could you please review again?

[Gyuyoung Kim]

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

[Sanghyup Lee]

Created attachment 235061 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 235061 [details]
Patch for landing

Clearing flags on attachment: 235061

Committed r171182: <http://trac.webkit.org/changeset/171182>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.