Bug 82864 - [EFL] LayoutTestController needs implementation of setTabKeyCyclesThroughElements
Summary: [EFL] LayoutTestController needs implementation of setTabKeyCyclesThroughElem...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexander Shalamov
URL:
Keywords:
Depends on: 86093
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-02 01:39 PDT by Alexander Shalamov
Modified: 2015-05-06 22:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.17 KB, patch)
2012-04-02 09:51 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (6.16 KB, patch)
2012-04-03 01:02 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (6.19 KB, patch)
2012-05-09 04:49 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (6.19 KB, patch)
2012-05-09 13:40 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (6.92 KB, patch)
2012-05-09 14:32 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (7.21 KB, patch)
2012-05-10 07:53 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (7.15 KB, patch)
2012-05-10 11:50 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (6.92 KB, patch)
2012-05-22 03:54 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (6.92 KB, patch)
2012-05-30 11:49 PDT, Alexander Shalamov
gyuyoung.kim: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Shalamov 2012-04-02 01:39:38 PDT
LayoutTestController needs implementation of setTabKeyCyclesThroughElements to unskip fast/events/keypress-insert-tab.html
Comment 1 Alexander Shalamov 2012-04-02 09:51:05 PDT
Created attachment 135123 [details]
Patch

DRT support for setTabKeyCyclesThroughElements
Comment 2 Gyuyoung Kim 2012-04-02 17:39:42 PDT
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.
Comment 3 Alexander Shalamov 2012-04-03 01:02:37 PDT
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.
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-04-05 09:04:36 PDT
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?
Comment 5 Alexander Shalamov 2012-04-05 12:53:35 PDT
(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.
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-04-05 15:08:26 PDT
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.
Comment 7 Alexander Shalamov 2012-05-09 04:49:51 PDT
Created attachment 140916 [details]
Patch

Rebased to master
Comment 8 Antonio Gomes 2012-05-09 11:24:22 PDT
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
Comment 9 Alexander Shalamov 2012-05-09 13:40:46 PDT
Created attachment 141008 [details]
Patch

Removed extra line
Comment 10 Alexander Shalamov 2012-05-09 14:32:20 PDT
Created attachment 141014 [details]
Patch

Reset WebCore::Page::m_tabKeyCyclesThroughElements to default
Comment 11 Alexander Shalamov 2012-05-09 14:32:56 PDT
(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
Comment 12 WebKit Review Bot 2012-05-10 05:27:33 PDT
Comment on attachment 141014 [details]
Patch

Clearing flags on attachment: 141014

Committed r116633: <http://trac.webkit.org/changeset/116633>
Comment 13 WebKit Review Bot 2012-05-10 05:27:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Thiago Marcos P. Santos 2012-05-10 06:02:24 PDT
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
Comment 15 WebKit Review Bot 2012-05-10 06:21:35 PDT
Re-opened since this is blocked by 86093
Comment 16 Alexander Shalamov 2012-05-10 07:53:52 PDT
Created attachment 141171 [details]
Patch

Fixes event grabbing problem. Focus was set to the frame instead of the view.
Comment 17 Alexander Shalamov 2012-05-10 11:50:51 PDT
Created attachment 141214 [details]
Patch

Updated changelog after rebasing
Comment 18 Alexander Shalamov 2012-05-22 03:54:08 PDT
Created attachment 143260 [details]
Patch

Rebased to master
Comment 19 Gyuyoung Kim 2012-05-22 22:24:30 PDT
(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.
Comment 20 Alexander Shalamov 2012-05-30 11:49:19 PDT
Created attachment 144885 [details]
Patch

Rebased to master
Comment 21 Eric Seidel (no email) 2012-08-22 15:45:55 PDT
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?
Comment 22 Alexander Shalamov 2012-08-24 04:38:49 PDT
(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.
Comment 23 Gyuyoung Kim 2012-09-02 22:43:38 PDT
Comment on attachment 144885 [details]
Patch

Looks fine.
Comment 24 WebKit Review Bot 2012-09-03 03:55:04 PDT
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
Comment 25 Alexander Shalamov 2012-09-03 04:52:06 PDT
(In reply to comment #23)
> (From update of attachment 144885 [details])
> Looks fine.

Patch is quite old, I need to rebase it to master.
Comment 26 Gyuyoung Kim 2013-01-06 17:17:50 PST
(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 ?
Comment 27 Martin Robinson 2015-05-06 22:43:32 PDT
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.