[Webkit-unassigned] [Bug 45889] Style visibility: hidden on <br/> tags causes input fields to lose focus after deleting all content

Mon Jan 31 23:08:43 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=45889

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #80639|review?                     |review-
               Flag|                            |

--- Comment #17 from Ryosuke Niwa <rniwa at webkit.org>  2011-01-31 23:08:42 PST ---
(From update of attachment 80639)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.