Bug 99149 – [EFL][WTR] Perform blur before focus on each test in WebKitTestRunner

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Mariusz Grzegorczyk]

Tests, in which focus is switched to subframe left FrameSelection on main frame unfocussed. It affects tests which follow broken one - "shouldBeginEditing" callbacks are not generated.

[Mariusz Grzegorczyk]

Created attachment 168374 [details]
patch

[Kangil Han]

Code itself looks good to me since it's done in reset stage between run of each test case. However, as focus/blur has strong impact on layout test, I would recommend you to run full layout test and check if regression happens. :-)

[Chris Dumez]

Comment on attachment 168374 [details]
patch

You say it fixes a problem but you don't unskip any test. So how do we know it is better now?

[Mariusz Grzegorczyk]

I've ran full layout tests without http ones, and this patch fixes ~15 tests. What's more important it fixes random failures in other tests(when child processes are greater than 1)

[Chris Dumez]

(In reply to comment #4)
> I've ran full layout tests without http ones, and this patch fixes ~15 tests. What's more important it fixes random failures in other tests(when child processes are greater than 1)

Then please unskip them in the same patch.

[Mariusz Grzegorczyk]

(In reply to comment #5)
> (In reply to comment #4)
> > I've ran full layout tests without http ones, and this patch fixes ~15 tests. What's more important it fixes random failures in other tests(when child processes are greater than 1)
> 
> Then please unskip them in the same patch.

The problem is that this patch doesn't fix specified tests, but general flow. Problematic test is passed, but has influence on next one, and this next one is randomic.

[Kangil Han]

(In reply to comment #4)
> I've ran full layout tests without http ones, and this patch fixes ~15 tests. What's more important it fixes random failures in other tests(when child processes are greater than 1)

This would prove improvement on flaky test cases. :-)

[Kangil Han]

Please unskip passed flaky test cases here. :-)

[Chris Dumez]

Comment on attachment 168374 [details]
patch

All this patch does it call:
evas_object_focus_set(m_view, false);
evas_object_focus_set(m_view, true);

in a row, instead of simply calling:
evas_object_focus_set(m_view, true);

right?

I don't get why this has any impact, why we need it, and why other ports don't. This fix does not look right to me.

[Kangil Han]

(In reply to comment #9)
> All this patch does it call:
> evas_object_focus_set(m_view, false);
> evas_object_focus_set(m_view, true);
> 

These two lines of code is not so strange to me in practical project.
Generally due to 'code guarding from another unknown bug' or 'library defect'.
But, it couldn't be a solution. :-)

@Mariusz,
Would you please verify how focus works by using 'evas_focus_get'?
You can refer to 'http://docs.enlightenment.org/auto/evas/Example_Evas_Events.html' for code details. :)

Maybe, another component steal focus or efl has defect.

[Mariusz Grzegorczyk]

evas_object_focus_set makes no effect on already focused Evas_Object. Same on widgets in gtk.
Problem exists also at least on gtk port.
So making only focus at the beginning of test does nothing.
Problem is inside webkit: There is focus on webview(Evas_Object), Frame, FocusController and FrameSelection
If some test using subframe switch focus to it, subframe takes focus, and on test exit it is destroyed, so none frame is focused, but webview is.
The problem is that after this operation in following test main frame has FrameSelection unfocussed so editing callbacks don't come.
Patch makes unfocus on webview so all components are blurred, than on focus all of them takes focus: FocusController and FrameSelection.

[Chris Dumez]

Comment on attachment 168374 [details]
patch

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

You should unskip the flaky tests that it fixes in the same patch otherwise we cannot tell that it changes anything.

> Tools/WebKitTestRunner/TestController.cpp:518
> +    m_mainWebView->blur();

We - at least - need a comment here to explain why the EFL port needs this.

[Kangil Han]

I think gtk+ port reviewers should know this kind of behavior since you mentioned gtk+ port has same issue.

[Mariusz Grzegorczyk]

(In reply to comment #12)
> (From update of attachment 168374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168374&action=review
> 
> You should unskip the flaky tests that it fixes in the same patch otherwise we cannot tell that it changes anything.

In editing category there's a lot of skipped subcategories(deleting, inserting, selection, spelling, style). A lot have also missing expected results. Some of them have different cause of being skipped.

To check this change you can run following 2 tests one after other:
editing/undo/undo-iframe-location-change.html
LayoutTests/editing/undo/undo-indent.html
Without this patch you can see following lines in diff file: 
-EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of OL > DIV > BODY > HTML > #document to 9 of OL > DIV > BODY > HTML > #document
-EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification

I suggest make gardening in separate patch including this change.

> 
> > Tools/WebKitTestRunner/TestController.cpp:518
> > +    m_mainWebView->blur();
> 
> We - at least - need a comment here to explain why the EFL port needs this.

In DRT following code at the beginning of each test solves this problem: ewk_frame_feed_focus_in(mainFrame());

[Gyuyoung Kim]

Any update ?

[Gyuyoung Kim]

Comment on attachment 168374 [details]
patch

Cleared review? from attachment 168374 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.