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

Sat Feb 5 20:06:19 PST 2011

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

--- Comment #19 from Srikumar B <srikumar.b at gmail.com>  2011-02-05 20:06:18 PST ---
(From update of attachment 80639)
View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review

>> Source/WebCore/ChangeLog:9
>> +        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

I will make the changes as suggested in the new patch

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:753
>> +        }
> 
> Why do we care that the input field is focused?

The text is being deleted in this validation.
So i have added the flag isFocusedNodeTextInputElement required to be set only when the input text field is focused and characters in the input field are being deleted which can be used in the below condition to make sure placeholder is needed or not. Hence we will call createBreakElement().

Please share me if you have any further comments on this.

>> LayoutTests/ChangeLog:8
>> +        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.

Sure. I will make the changes as per your suggestion in the next patch

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

I have added this in the latest patch

>> 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?

I thought editing.js needed to call execute commands "InsertText", "SelectAll", "Delete" etc using document.execCommand() but i am able to call those APIs without including this. So i will remove this in latest patch

>> LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:27
>> +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?

Without these code changes, after "Delete", i am unable to insert any characters even though focus ring is still on text field as hidden style of BR does not let enter data.
hence the test is failing because document.execCommand("InsertText",false,'XYZ') statement fails.

With this fix, the cursor still remain on input text field and document.execCommand("InsertText",false,'XYZ') succeed.

Do let me know your comments on it.

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