[webkit-reviews] review denied: [Bug 40338] display:none element should not retain focus : [Attachment 107039] Updated Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Vineet N Chaudhary
<rgf748 at motorola.com>'s request for review:
Bug 40338: display:none element should not retain focus
https://bugs.webkit.org/show_bug.cgi?id=40338

Attachment 107039: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=107039&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107039&action=review


> Source/WebCore/ChangeLog:8
> +	   When elements gets hidden its renderer gets drop, such element
should

the word hidden makes me think of visibility: hidden.  Should we remove focus
in that case as well?  If so, we need a test for that.

> Source/WebCore/page/FrameView.cpp:2212
> +    // As HTMLAreaElement doesn't have renderer() we dont want to set focus
to 0.

Is this tested somewhere?

> LayoutTests/ChangeLog:8
> +	   Added test case to check the behavior.

"check the behavior" is too general.

> LayoutTests/fast/events/remove-focus-on-hidden-elelment.html:4
> +

Why do we need this line?

> LayoutTests/fast/events/remove-focus-on-hidden-elelment.html:12
> +if (window.layoutTestController) {
> + layoutTestController.dumpAsText();
> +}

No curly brackets around a single line statement.

> LayoutTests/fast/events/remove-focus-on-hidden-elelment.html:28
> +testArea.addEventListener('keydown', function (event) { keycount++ ;});
> +
> +testArea.style.display = 'none';
> +
> +// Send keyboard event
> +eventSender.keyDown("A");
> +
> +logResult(keycount ? "FAIL" : "PASS");
> +
> +function logResult(str) {
> + document.getElementById('result').innerHTML = str;
> +}

Why do we check that the node no longer retain focus by counting keydown event?
It seems like we should directly check focused property.