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.
Created attachment 102073 [details] patch + unit test
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.
Created attachment 102153 [details] fix header ordering
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.
> > 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?
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.
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?
Created attachment 108229 [details] Refactor helpers into a separate file
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!
Comment on attachment 108229 [details] Refactor helpers into a separate file Clearing flags on attachment: 108229 Committed r96912: <http://trac.webkit.org/changeset/96912>
All reviewed patches have been landed. Closing bug.