Steps to replicate: -Create an HTML with style "br {visibility: hidden}" (sample included below) -Include an input field on the page -Load the HTML file -Type text in the input field -Use the backspace key to delete the text. Once all text is deleted, form field loses focus Sample HTML to replicate: <html> <head> <style type="text/css"> br {visibility:hidden} </style> </head> <form> <fieldset> <input/> </fieldset> </form> </html>
The issue is because we add child node HTMLBRElement when the characters are deleted from the text field. As br style set to hidden without any class, this is considered as default RenderStyle.
I think the right fix is to make sure style from outside the text field can’t affect the elements used inside the field.
Hyatt, any ideas on the best way to do that?
(In reply to comment #2) > I think the right fix is to make sure style from outside the text field can’t affect the elements used inside the field. I agree with you too that that will be the ultimate fix. Adding br should be fine in this case because that'll remove all dependencies on br styles. On the other hand, the same technique won't work for the bug 27683.
Created attachment 71694 [details] proposed changes to fix the bug As per the discussion on IRC with editing experts, For text input elements, BreakElement place holder is not needed. So, i have added a validation before creating BR element because any BR styles should not affect text field properties.
@rniwa: Any interest in reviewing this editing patch?
Comment on attachment 71694 [details] proposed changes to fix the bug View in context: https://bugs.webkit.org/attachment.cgi?id=71694&action=review > WebCore/ChangeLog:8 > + Tests: Covered with other input text editing testcases If this is covered with other test cases, then the patch still needs to contain the change to expected results. Otherwise, the fix is not covered. Please include a test.
(In reply to comment #6) > @rniwa: Any interest in reviewing this editing patch? The change looks sane but we definitely need a layout test as darin just pointed out.
Created attachment 80297 [details] proposed changes with layout test to validate the fix proposed changes with layout test to validate the fix
I have written a valid layout test and tested with and without changes to made sure the fix is valid and working. Kindly review and do let me know for any further information -Sri
Comment on attachment 80297 [details] proposed changes with layout test to validate the fix View in context: https://bugs.webkit.org/attachment.cgi?id=80297&action=review > Source/WebCore/editing/DeleteSelectionCommand.cpp:751 > && ancestorNode->focused()) > + { Nit: These two lines should be combined.
Created attachment 80600 [details] latest patch for proposed changes incorporating review comment from adam barth latest patch for proposed changes incorporating review comment from adam barth
Comment on attachment 80600 [details] latest patch for proposed changes incorporating review comment from adam barth 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.
Comment on attachment 80297 [details] proposed changes with layout test to validate the fix Same issues.
Created attachment 80639 [details] revised patch by making changes with respect to comments from Ryosuke Niwa revised patch by making changes with respect to comments from Ryosuke Niwa
Hi Ryosuke Niwa, I modified the patch with respect to all the comments posted in the review. Kindly review do let me know your comments...
Comment on attachment 80639 [details] revised patch by making changes with respect to comments from Ryosuke Niwa 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?
Any update on this?
Comment on attachment 80639 [details] revised patch by making changes with respect to comments from Ryosuke Niwa 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.
Comment on attachment 80639 [details] revised patch by making changes with respect to comments from Ryosuke Niwa View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review > LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:20 > +</form> removed <form> & </form> tags in the latest patch as it does not have any specific use with this testcase > LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:29 > +<body> changed to </body> on latest patch
Comment on attachment 80639 [details] revised patch by making changes with respect to comments from Ryosuke Niwa View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review >>> 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. So are you saying that we can insert br into a text field if it's not focused? I'm skeptical about that. I don't think we should ever insert a br into the shadow DOM of an input element. >>> 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. I think you should still check that t has focus because that's the bug is about. You can check both that has focus and you can successfully insert text.
Created attachment 81414 [details] issue test page issue test page
Comment on attachment 80639 [details] revised patch by making changes with respect to comments from Ryosuke Niwa View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review >>>> 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. > > So are you saying that we can insert br into a text field if it's not focused? I'm skeptical about that. I don't think we should ever insert a br into the shadow DOM of an input element. The actual issue is, When you click on text field==>enter text=> delete complete text==>Now, I cannot enter text as i am not able to edit the focusing node==> Even i cannot gain the cursor by clicking on the text field==> Now if i click outside the textfield with mouse and click on text field again, i Can enter the text. So the issue is that, till the focus is on text field and while editing the text only. When the focus is lost and gain back, There is no issue. I have attached the issue test page (45889_issue.htm). Kindly try to reproduce the issue. Do let me know your comment. >>>> 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. > > I think you should still check that t has focus because that's the bug is about. You can check both that has focus and you can successfully insert text. Sure. I will test both conditions. Could you please let me know how to validate the focus is on text field? Is it document.activeElement or any better way to get the focus node which is editable or not?
(In reply to comment #23) > (From update of attachment 80639 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80639&action=review > The actual issue is, When you click on text field==>enter text=> delete complete text==>Now, I cannot enter text as i am not able to edit the focusing node==> Even i cannot gain the cursor by clicking on the text field==> Now if i click outside the textfield with mouse and click on text field again, i Can enter the text. So the issue is that, till the focus is on text field and while editing the text only. When the focus is lost and gain back, There is no issue. I have attached the issue test page (45889_issue.htm). Kindly try to reproduce the issue. Do let me know your comment. I know what the problem you're trying to solve. However, my point is that you're only fixing the very special case and not taking care of others. Namely, we're still inserting BR into non-focused text field and that's just wrong. We should never be inserting BR into text field. So your condition for inserting BR should be that it's not inside a multi-line shadow DOM.
I talked with sri on IRC and it seems like we should be turning on the flag regardless of the focus. However, we can't call textWillBeDeletedInTextField when the text field doesn't have focus due to http://trac.webkit.org/changeset/19672.
Created attachment 81454 [details] revised patch with respect to comments from Ryosuke Niwa attached the revised patch with respect to comments from Ryosuke Niwa and discussing further on IRC. I have executed the regression on editing text fields with this fix. I have manually tested editing the text in input field using javascript and made sure that BR element is not being inserted when text is empty in input field.
Attachment 81454 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/editing/DeleteSelectionCommand.cpp:751: Missing space before ( in if( [whitespace/parens] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 81455 [details] revised patch with respect to comments from Ryosuke Niwa corrected styling error
Attachment 81455 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2 perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LANG = "en_US.US-ASCII" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). Updating OpenSource perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LANG = "en_US.US-ASCII" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 129. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 81495 [details] revised patch with respect to comments from Ryosuke Niwa uploaded the patch again as the styling failed because the style bot had an internal error
Comment on attachment 81495 [details] revised patch with respect to comments from Ryosuke Niwa View in context: https://bugs.webkit.org/attachment.cgi?id=81495&action=review Thanks for fixing this bug! > LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:5 > +br{visibility:hidden} Let's put spaces between words and semi-colon at the end as in br { visibility: hidden; } > LayoutTests/editing/deleting/textfield-loose-focus-with-br.html:11 > +<script> > +if (window.layoutTestController) { > + layoutTestController.dumpAsText(); > +} > +</script> You should put this in the script element inside body so that you don't have to have a separate script element.
Created attachment 81572 [details] revised patch with respect to comments from Ryosuke Niwa
Created attachment 81575 [details] revised patch with respect to comments from Ryosuke Niwa
Comment on attachment 81575 [details] revised patch with respect to comments from Ryosuke Niwa Rejecting attachment 81575 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-..." exit_code: 2 Last 500 characters of output: ............ fast/forms ......................................................................................................................................................................................................................................... fast/forms/input-placeholder-visibility-3.html -> failed Exiting early after 1 failures. 8370 tests run. 199.19s total testing time 8369 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7820005
Created attachment 87868 [details] updated patch with re-baselined failing testcase Hi Ryosuje Niwa, Here with i attached the updated patch. This does not have any additional code changes. It just include the one failed testcase (platform/mac/fast/forms/input-placeholder-visibility-3-expected.txt) that i re-baselined as per this fix. I made sure no other layout tests are failed with this fix. Kindly review and approve for commit-queue.
Comment on attachment 87868 [details] updated patch with re-baselined failing testcase Rejecting attachment 87868 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build-..." exit_code: 2 Last 500 characters of output: ....... fast/forms .............................................................................................................................................................................................................................................. fast/forms/input-placeholder-visibility-3.html -> failed Exiting early after 1 failures. 8564 tests run. 205.17s total testing time 8563 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8321659
I tested your patch and I'm getting a crash on editing/execCommand/indent-node-to-split-to-crash.html with the following stack trace. SHOULD NEVER BE REACHED /Volumes/Data/webkit2/Source/WebCore/editing/ApplyBlockElementCommand.cpp(142) : virtual void WebCore::ApplyBlockElementCommand::formatSelection(const WebCore::VisiblePosition&, const WebCore::VisiblePosition&) 1 WebCore::ApplyBlockElementCommand::formatSelection(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&) 2 WebCore::IndentOutdentCommand::formatSelection(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&) 3 WebCore::ApplyBlockElementCommand::doApply() 4 WebCore::EditCommand::apply() 5 WebCore::applyCommand(WTF::PassRefPtr<WebCore::EditCommand>) 6 WebCore::executeIndent(WebCore::Frame*, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) 7 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const 8 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) The other test failure was due to LayoutTests/platform/mac-snowleopard/fast/forms/input-placeholder-visibility-3-expected.txt. You should just remove that file and rebaseline LayoutTests/platform/mac/fast/forms/input-placeholder-visibility-3-expected.txt since they're identical.
Comment on attachment 87868 [details] updated patch with re-baselined failing testcase r- since a test crash.
Comment on attachment 81495 [details] revised patch with respect to comments from Ryosuke Niwa Cleared Ryosuke Niwa's review+ from obsolete attachment 81495 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 94442 [details] adds a regression test
Thanks for the review, Tony. Landing it now.
Committed r87081: <http://trac.webkit.org/changeset/87081>