[webkit-reviews] review denied: [Bug 45889] Style visibility: hidden on <br/> tags causes input fields to lose focus after deleting all content : [Attachment 80600] latest patch for proposed changes incorporating review comment from adam barth

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Srikumar B <srikumar.b at gmail.com>'s
request for review:
Bug 45889: Style visibility: hidden on <br/> tags causes input fields to lose
focus after deleting all content
https://bugs.webkit.org/show_bug.cgi?id=45889

Attachment 80600: latest patch for proposed changes incorporating review
comment from adam barth
https://bugs.webkit.org/attachment.cgi?id=80600&action=review

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

> Source/WebCore/ChangeLog:8
> +

Please explain what caused the bug and how you fixed it.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:800
> +    // For text input elements, BreakElement place holder is not needed.
> +    // Because any BR styles should not affect text field properties. 

This comment is redundant it repeats what code says.

> LayoutTests/ChangeLog:8
> +

Please explain what kind of test you're adding.

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:16
> +<script>
> +if (window.layoutTestController)
> +	layoutTestController.dumpEditingCallbacks();
> +</script>
> +
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.waitUntilDone();
> +    layoutTestController.dumpAsText();
> +}
> +</script>

Please combine these two script elements.  And also, I don't think you need to
call dumpEditingCallbacks in this test unless there are some delegate callbacks
you want to test.

Also, why are you calling waitUntilDone() here?  waitUntilDone() is called when
the test needs to continue to run after page load event, and I don't see any
reason we want such a behavior in this test.

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:22
> +<div><br></div>

What is this div & br doing here?

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:38
> +if(text === "XYZ")
> +	document.write("<div> test SUCCESS <\div>");
> +else
> +	document.write("<div> test FAILED <\div>");

Better written as:
document.write(text == "XYZ" ? "PASS" : "FAIL");

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:45
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.notifyDone()
> +}
> +</script>

If we stop calling waitUntilDone(), then this entire script can go away.