Bug 65220 – [Chromium] WebViewImpl doesn't proper deactivate focus

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Nate Chapin]

Original report: http://code.google.com/p/chromium/issues/detail?id=64846

The problem is in WebViewImpl::setFocus().  We call setActive(true) when we set focus, but don't call setActive(false) when we unset it.

There's a very old (pre-release) comment indicating that this behavior is to avoid sending spurious focus/blur events, but all tests still appear to be passing, so I don't think this is an issue.

[Nate Chapin]

Created attachment 102073 [details]
patch + unit test

[WebKit Review Bot]

Attachment 102073 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/tests/WebViewTest.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/tests/WebViewTest.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/chromium/tests/WebViewTest.cpp:44:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nate Chapin]

Created attachment 102153 [details]
fix header ordering

[Darin Fisher (:fishd, Google)]

Comment on attachment 102153 [details]
fix header ordering

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

> Source/WebKit/chromium/tests/WebViewTest.cpp:72
> +        webkit_support::RegisterMockedURL(WebURL(GURL(baseURL + fileName)), response, WebString::fromUTF8(filePath));

nit: you shouldn't need to manually specify the WebURL constructor.  there should be an implicit conversion constructor for WebURL that takes a GURL.

> Source/WebKit/chromium/tests/WebViewTest.cpp:84
> +        urlRequest.setURL(WebURL(GURL(baseURL + fileName)));

ditto

> Source/WebKit/chromium/tests/WebViewTest.cpp:89
> +    std::string baseURL;

nit: m_baseURL

> Source/WebKit/chromium/tests/WebViewTest.cpp:108
> +    WebFrameImpl* frame = static_cast<WebFrameImpl*>(webView->mainFrame());

it'd be nice if this could be done using only WebKit APIs, which it almost can be:

  WebDocument doc = webView->mainFrame()->document();
  EXPECT_TRUE(doc.isHTMLDocument());

But, we don't have a WebHTMLDocument interface, and we don't have hasFocus().
We do have WebDocument::focusedNode() though.  Could that be used?

It just makes this test more fragile if it depends on WebCore and static casting
to WebKit API implementation types.

> Source/WebKit/chromium/src/WebViewImpl.cpp:-1297
> -        // focus/blur events to be dispatched.

i seem to recall that there was some very tricky focus/blur logic required
to make popup lists work properly.  we should make sure that those don't
regress.  i'm pretty sure that we lack layout test coverage.

[Nate Chapin]

> > Source/WebKit/chromium/src/WebViewImpl.cpp:-1297
> > -        // focus/blur events to be dispatched.
> 
> i seem to recall that there was some very tricky focus/blur logic required
> to make popup lists work properly.  we should make sure that those don't
> regress.  i'm pretty sure that we lack layout test coverage.

I tried the test case linked in http://code.google.com/p/chromium/issues/detail?id=23499 and the events were identical before and after.  Is there anything else I should try?

[Nate Chapin]

Created attachment 103694 [details]
fix style issues

Since focusedNode() means something completely different, we really need to test some variant of an HTMLDocument.  I don't think it makes sense to expose a new class just to test this bug.

[Darin Fisher (:fishd, Google)]

Comment on attachment 103694 [details]
fix style issues

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

> Source/WebKit/chromium/tests/WebViewTest.cpp:50
> +class WebViewTest : public testing::Test {

this looks like it is largely duplicating code from WebFrameTest.cpp.  it
seems like it would be better to refactor so you can share code?

[Nate Chapin]

Created attachment 108229 [details]
Refactor helpers into a separate file

[Darin Fisher (:fishd, Google)]

Comment on attachment 108229 [details]
Refactor helpers into a separate file

R=me

sorry for the delayed review, and thank you for refactoring the test code!

[WebKit Review Bot]

Comment on attachment 108229 [details]
Refactor helpers into a separate file

Clearing flags on attachment: 108229

Committed r96912: <http://trac.webkit.org/changeset/96912>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.