[webkit-reviews] review denied: [Bug 45889] Style visibility: hidden on <br/> tags causes input fields to lose focus after deleting all content : [Attachment 80639] revised patch by making changes with respect to comments from Ryosuke Niwa

Mon Jan 31 23:08:42 PST 2011

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 80639: revised patch by making changes with respect to comments from
Ryosuke Niwa
https://bugs.webkit.org/attachment.cgi?id=80639&action=review

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

> Source/WebCore/ChangeLog:9
> +	   After deleting all characters in text input field, cursor 
> +	   focus is being lost when style br{visibility:hidden} is set.
> +	   Actually, placeholder <BR> element not needed for Input Text Field
when content empty.
> +	   So, additional validation included to skip adding placeholder when
the editing node is Input Text field
> +	   https://bugs.webkit.org/show_bug.cgi?id=45889

You shouldn't delete the bug title.  Description should come AFTER the bug
title and bug url.  So it should be something like (all indented
appropriately):

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

The bug was caused by DeleteSelectionCommand's inserting a placeholder br
element into a text field
when the text field becomes empty. Fixed DeleteSelectionCommand to not insert
the placeholder
when the command is executed inside a text field.

r- because of the format here

> Source/WebCore/editing/DeleteSelectionCommand.cpp:753
>	   Node* ancestorNode = startNode ? startNode->shadowAncestorNode() :
0;
>	   if (ancestorNode && ancestorNode->hasTagName(inputTag)
>		   &&
static_cast<HTMLInputElement*>(ancestorNode)->isTextField()
> -		   && ancestorNode->focused())
> +		   && ancestorNode->focused()) {
>	      
document()->frame()->editor()->textWillBeDeletedInTextField(static_cast<Element
*>(ancestorNode));
> +	       isFocusedNodeTextInputElement = true;
> +	   }

Why do we care that the input field is focused?

> LayoutTests/ChangeLog:8
> +
> +	   This testcase insert characters "ABC" in input text field where
style br{visibility:hidden} is set,
> +	   Select all the text and delete selection. Then, insert characters
"XYZ". 
> +	   With this fix, characters "XYZ" can be inserted as focus should not
be lost after deletion.
> +	   https://bugs.webkit.org/show_bug.cgi?id=45889

Ditto about the bug title and url appearing first followed by a blank line.

I think the description is a little verbose.  Try something along the line of:

Added a test to make sure deleting text from text field doesn't lose focus even
if br's visibility is hidden.

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

Missing <!DOCTYPE html>

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:11
> +<script src="../editing.js"></script>

Why are you including this file if you're not calling any functions in
editing.js?

> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:27
> +document.execCommand("InsertText",false,'XYZ');
> +document.write(document.getElementById("t").value == "XYZ" ? "PASS" :
"FAIL");

Wait, this doesn't test what the test claims to test.  Shouldn't we be testing
that t is still focused?