Bug 82864 – [EFL] LayoutTestController needs implementation of setTabKeyCyclesThroughElements

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

[Alexander Shalamov]

LayoutTestController needs implementation of setTabKeyCyclesThroughElements to unskip fast/events/keypress-insert-tab.html

[Alexander Shalamov]

Created attachment 135123 [details]
Patch

DRT support for setTabKeyCyclesThroughElements

[Gyuyoung Kim]

Comment on attachment 135123 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:464
> +    Evas_Object* mainFrame = browser->mainFrame();

Missing const keyword. For example,

const Evas_Object* mainFrame = browser->mainFrame();

> Tools/DumpRenderTree/efl/EventSender.cpp:468
> +    evas_object_focus_set(mainFrame, EINA_TRUE);

Use standard boolean type instead of EINA_TRUE.

[Alexander Shalamov]

Created attachment 135286 [details]
Patch

Fixed: EINA_TRUE -> true
Unfortunately I can't make mainFrame const, since it is passed to setter and it expects non-const parameter.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135286 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:467
> +    // Focus of the main frame could be lost during the test
> +    // Therefore, setting focus to the main frame

Could you clarify this comment? What kind of scenario would cause the focus to be moved elsewhere? And what would this elsewhere be?

[Alexander Shalamov]

(In reply to comment #4)
> (From update of attachment 135286 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135286&action=review
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:467
> > +    // Focus of the main frame could be lost during the test
> > +    // Therefore, setting focus to the main frame
> 
> Could you clarify this comment? What kind of scenario would cause the focus to > be moved elsewhere? And what would this elsewhere be?

There is 1 focusable node in a test case, which is an input element. In the beginning of a test case, we focus on an input element and then we emulate keypress "tab". Since there is no focusable element, focus controller will ask "chrome" to take focus. In case of EFL, whole view would be unfocused, all consecutive events would be omitted, since the view is unfocused. Similar "solution" is used in gtk DRT code.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 135286 [details]
Patch

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

>>> Tools/DumpRenderTree/efl/EventSender.cpp:467
>>> +    // Therefore, setting focus to the main frame
>> 
>> Could you clarify this comment? What kind of scenario would cause the focus to be moved elsewhere? And what would this elsewhere be?
> 
> There is 1 focusable node in a test case, which is an input element. In the beginning of a test case, we focus on an input element and then we emulate keypress "tab". Since there is no focusable element, focus controller will ask "chrome" to take focus. In case of EFL, whole view would be unfocused, all consecutive events would be omitted, since the view is unfocused. Similar "solution" is used in gtk DRT code.

OK, looks fine then.

[Alexander Shalamov]

Created attachment 140916 [details]
Patch

Rebased to master

[Antonio Gomes]

Comment on attachment 140916 [details]
Patch

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

do you need to "reset to default" before each test test?

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:468
> +    WebCore::Page* page = EWKPrivate::corePage(ewkView);
> +
> +    if (!page)

bogus extra line

[Alexander Shalamov]

Created attachment 141008 [details]
Patch

Removed extra line

[Alexander Shalamov]

Created attachment 141014 [details]
Patch

Reset WebCore::Page::m_tabKeyCyclesThroughElements to default

[Alexander Shalamov]

(In reply to comment #8)
> (From update of attachment 140916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140916&action=review
> 
> do you need to "reset to default" before each test test?
Fixed

> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:468
> > +    WebCore::Page* page = EWKPrivate::corePage(ewkView);
> > +
> > +    if (!page)
> 
> bogus extra line
Fixed

[WebKit Review Bot]

Comment on attachment 141014 [details]
Patch

Clearing flags on attachment: 141014

Committed r116633: <http://trac.webkit.org/changeset/116633>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

New failures after this patch:

fast/events/onchange-searchfield.html
fast/events/check-defocus-event-order-when-triggered-by-mouse-click.html
fast/events/check-defocus-event-order-when-triggered-by-tab.html
fast/events/onchange-passwordfield.html
fast/events/inputText-never-fired-on-keydown-cancel.html
fast/events/onchange-select-popup.html
fast/events/onchange-textfield.html
fast/events/autoscroll-should-not-stop-on-keypress.html
fast/forms/textfield-no-linebreak.html
fast/spatial-navigation/snav-iframe-flattening-simple.html
fast/spatial-navigation/snav-iframe-nested.html
fast/spatial-navigation/snav-iframe-recursive-offset-parent.html
fast/spatial-navigation/snav-iframe-with-offscreen-focusable-element.html

[WebKit Review Bot]

Re-opened since this is blocked by 86093

[Alexander Shalamov]

Created attachment 141171 [details]
Patch

Fixes event grabbing problem. Focus was set to the frame instead of the view.

[Alexander Shalamov]

Created attachment 141214 [details]
Patch

Updated changelog after rebasing

[Alexander Shalamov]

Created attachment 143260 [details]
Patch

Rebased to master

[Gyuyoung Kim]

(In reply to comment #18)
> Created an attachment (id=143260) [details]
> Patch
> 
> Rebased to master

What is difference from r116633 ? It looks almost this patch was already landed by r116633. Was this patch roll out ? If so, it looks REOPEN is correct status.

[Alexander Shalamov]

Created attachment 144885 [details]
Patch

Rebased to master

[Eric Seidel (no email)]

Comment on attachment 144885 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:548
> +    // Focus of the main frame could be lost during the test
> +    // Therefore, setting focus to the view
> +    evas_object_focus_set(view, true);

Will this end up changing the focus away from a control?  what does this set the focus to?

[Alexander Shalamov]

(In reply to comment #21)
> (From update of attachment 144885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144885&action=review
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:548
> > +    // Focus of the main frame could be lost during the test
> > +    // Therefore, setting focus to the view
> > +    evas_object_focus_set(view, true);
> 
> Will this end up changing the focus away from a control?

From HTML controls, no.

>>what does this set the focus to?

To the main efl view (ewk_view)

When we cycle thru elements and last element is reached, in 
WebCore::FocusController::advanceFocusInDocumentOrder focus is passed to Chrome. Code snippet from focus controller:

// We didn't find a node to focus, so we should try to pass focus to Chrome.
m_page->chrome()->takeFocus(direction);

in EFL port, takeFocus removes focus from the view

void ChromeClientEfl::takeFocus(FocusDirection)
{
    unfocus();
}

In DRT context, focus is never set back to the view, therefore, all events are dismissed, since the view is unfocused.

This patch forces canvas to be always in focused state, so that EventSender could send events to the view.

[Gyuyoung Kim]

Comment on attachment 144885 [details]
Patch

Looks fine.

[WebKit Review Bot]

Comment on attachment 144885 [details]
Patch

Rejecting attachment 144885 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
LED -- saving rejects to file Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp.rej
patching file Tools/DumpRenderTree/efl/EventSender.cpp
patching file Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp
Hunk #1 FAILED at 241.
1 out of 1 hunk FAILED -- saving rejects to file Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Gyuyoung K..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13739156

[Alexander Shalamov]

(In reply to comment #23)
> (From update of attachment 144885 [details])
> Looks fine.

Patch is quite old, I need to rebase it to master.

[Gyuyoung Kim]

(In reply to comment #25)
> (In reply to comment #23)
> > (From update of attachment 144885 [details] [details])
> > Looks fine.
> 
> Patch is quite old, I need to rebase it to master.

Could you please check if this patch is still needed ?

[Martin Robinson]

This patch seems to address the efl version of DumpRenderTree, which no longer exists. I'm going to close it, but feel free to reopen it if something is needed for WebKitTestRunner.