6862 2006-01-26 21:03:42 -0800 Input's value doesn't get updated after typing in new text field 2006-01-31 18:51:52 -0800 1 1 1 Unclassified WebKit Forms 420+ Mac OS X 10.4 RESOLVED FIXED P2 Normal --- 1 adele adele ap darin oldest_to_newest 30243 0 adele 2006-01-26 21:03:42 -0800 30244 1 6007 adele 2006-01-26 21:07:52 -0800 Created attachment 6007 test case 30247 2 6008 adele 2006-01-26 21:26:32 -0800 Created attachment 6008 patch I'm not so sure we should use div->firstChild() to get to the text node. Justin and I were discussing the fact that the editing code has the ability to add more text nodes. I wonder if there's a good way to get the text of all the children of the div, and to use that to set the value instead. Something better than innerText? 30248 3 6008 eric 2006-01-26 21:47:56 -0800 Comment on attachment 6008 patch To answer adele's question, "textContent()" can be used instead of innerText. 30268 4 6008 mjs 2006-01-26 23:00:19 -0800 Comment on attachment 6008 patch It could hurt performance to have a mutation event listener attached any time there is a text field in the DOM. We specifically try to optimize and avoid work when no mutation events are attached. The DOM goes out of its way to avoid dispatching mutation events at all when none are attached. It might be possible to scope this more narrowly and only dispatch when some ancestor has a mutation event listener. Alternately, wouldn't it be sufficient to attach the mutation listener only when the text field has focus? The shadow DOM contents can't change when it does not have focus, right? Also, textContents as suggested by Eric will not give the same results as innerText. It doesn't understand styling so it won't understand things like newline <br>s. That shouldn't happen for text field anyway, but I'm not sure if the editing code is smart enough to put in raw newlines when the white-space mode is pre (I think you can force a newline in a text field with option-enter). 30311 5 6008 darin 2006-01-27 08:44:31 -0800 Comment on attachment 6008 patch Dave and Maciej were talking about this last night on IRC. They're concerned that since we have a special optimization for when there are no listeners to DOM mutation events that adding handlers could cause a speed problem -- and if this is on any page with a <input type=text> that could be a real problem. They believe that we need to either: 1) do measurements to prove that event listeners are OK, performance-wise, and if so, plan to eventually remove the optimization for "no event listeners for mutation events on this document" or 2) use an ad-hoc scheme other than a mutation listener to address this requirement: Dave suggested a hook in RenderText I think for this case Tiny code structure thing: If we end up going with (1), I'd suggest putting the removeEventListener call in ~RenderTextField inside the same if that's used for the detach call. In RenderTextField::subtreeHasChanged, we probably want to use node() rather than element(). In general, element() is just a slower node() that does something special for anonymous cases. Marking this review+, but please do not land until doing the performance tests because of the DOM event issue. You should discuss this with Dave, Maciej, and Justin (since Justin is working on a patch that uses mutation events for a similar purpose). 30354 6 mjs 2006-01-27 15:40:44 -0800 Besides the two possibilities Darin mentioned, we also discussed the following: 3) Come up with a new, lightweight change notification mechanism solely for internal use in the DOM. If such a mechanism was fast enough, it could be used for the <input type="text">, for detecting changes to selection boundaries, for NodeLists, for HTMLCollections, for <textarea> in the future, for cleaner invalidation of the undo chain, and maybe for other purposes. Yet another possibility is: 4) Even if mutation events are slow now, maybe they can be sped up enough that a mutation event listener in an one part of the DOM tree will not affect performance of changes in other parts of the DOM. Another issue with DOM mutation events is that they do not fire in response to changes that happen as a result of parsing, this would be bad for some potential use cases. I personally approach 3 is the most promising, even though it involves a bit of up-front work. 30368 7 adele 2006-01-27 18:00:05 -0800 I'm trying to run some performance tests to see what the impact would be, but I'm running into some crashes that are preventing me from doing this. More updates later... 30370 8 justin.garcia 2006-01-27 18:30:10 -0800 (In reply to comment #4) > (From update of attachment 6008 [edit]) > It could hurt performance to have a mutation event listener attached any time > there is a text field in the DOM. We specifically try to optimize and avoid > work when no mutation events are attached. The DOM goes out of its way to avoid > dispatching mutation events at all when none are attached. Typical page loads don't do anything that would ever send character modification events. Set a breakpoint on the first line of CharacterDataImpl::dispatchModifiedEvent and run the PLT. > Alternately, wouldn't it be sufficient to attach the mutation listener only > when the text field has focus? The shadow DOM contents can't change when it > does not have focus, right? No, you can set the contents of an input field programmatically from JS. > It doesn't understand styling so it won't understand things like > newline <br>s. That shouldn't happen for text field anyway, but I'm not sure if > the editing code is smart enough to put in raw newlines when the white-space > mode is pre (I think you can force a newline in a text field with > option-enter). I don't think a textfield can have newlines in it (option-enter didn't work for me). I think that it will be safe to assume that the shadow div will only ever have a single child, but that child won't always be a text node (when you empty out the text field, editing will insert a placeholder <br>). 30384 9 ap 2006-01-27 23:57:14 -0800 >I don't think a textfield can have newlines in it (option-enter didn't work for me). I also couldn't insert a newline with copy/paste or drag&drop - but shouldn't this be considered a bug? In my experience, most single-line input fields in Mac applications do accept newlines through pasting. 30428 10 darin 2006-01-28 17:10:05 -0800 (In reply to comment #9) > I also couldn't insert a newline with copy/paste or drag&drop - but shouldn't > this be considered a bug? Yes, if that happened it would be a bug. The implementation of that for our current text field is in these methods: -[KWQTextFieldController textView:shouldChangeTextInRange:replacementString:] -[KWQTextFieldController preprocessString:] > In my experience, most single-line input fields in Mac applications do accept > newlines through pasting. Maybe, but not the one in WebKit. 30429 11 darin 2006-01-28 17:10:57 -0800 Why is this a P1 bug? The new text field is still off by default, right? I don't think bugs in the new text field implementation should be considered P1 until it's on by default. 30434 12 darin 2006-01-28 17:12:38 -0800 These bugs that block us switching to the new text field shoul not be marked P1. Once we turn it on, these would be P1/major bugs, of course, but at the moment they are just part of the "switch to a new text field" task and should not be in the P1 list. 30830 13 adele 2006-01-31 18:42:52 -0800 we decided to reevaluate the performance problems before we flip the switch. 6007 2006-01-26 21:07:52 -0800 2006-01-26 21:07:52 -0800 test case test3.html text/html 275 adele VHlwZSBpbiB0ZXh0IGZpZWxkLiAgQ2xpY2sgdGhlICJHZXQgVmFsdWUiIGJ1dHRvbiBhbmQgeW91 IHNob3VsZCBzZWUgYW4gYWxlcnQgd2l0aCB0aGUgdHlwZWQgdmFsdWUuCjxpbnB1dCB0eXBlPSJ0 ZXh0IiBpZD0ic3AyIiBzdHlsZT0iLWtodG1sLWFwcGVhcmFuY2U6dGV4dGZpZWxkOyI+PC9pbnB1 dD4KPGlucHV0IHR5cGU9ImJ1dHRvbiIgb25jbGljaz0iYWxlcnQoZG9jdW1lbnQuZ2V0RWxlbWVu dEJ5SWQoJ3NwMicpLnZhbHVlKSIgdmFsdWU9IkdldCBWYWx1ZSI+PC9pbnB1dD4= 6008 2006-01-26 21:26:32 -0800 2006-01-27 08:44:31 -0800 patch patch_value.txt text/plain 4808 adele SW5kZXg6IHJlbmRlcmluZy9SZW5kZXJUZXh0RmllbGQuaAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSByZW5kZXJp bmcvUmVuZGVyVGV4dEZpZWxkLmgJKHJldmlzaW9uIDEyNDAxKQorKysgcmVuZGVyaW5nL1JlbmRl clRleHRGaWVsZC5oCSh3b3JraW5nIGNvcHkpCkBAIC0yMCwxMiArMjAsMjkgQEAKICNpZm5kZWYg UmVuZGVyVGV4dEZpZWxkX0gKICNkZWZpbmUgUmVuZGVyVGV4dEZpZWxkX0gKIAorI2luY2x1ZGUg ImRvbTJfZXZlbnRzLmgiCiAjaW5jbHVkZSAicmVuZGVyX2ZsZXhib3guaCIKICNpbmNsdWRlICJo dG1sX2Jsb2NraW1wbC5oIgogCiBuYW1lc3BhY2UgV2ViQ29yZQogewogCitjbGFzcyBSZW5kZXJU ZXh0RmllbGQ7CisKK2NsYXNzIElucHV0TXV0YXRpb25MaXN0ZW5lciA6IHB1YmxpYyBFdmVudExp c3RlbmVyCit7CitwdWJsaWM6CisgICAgSW5wdXRNdXRhdGlvbkxpc3RlbmVyKCkgeyBtX3JlbmRl clRleHRGaWVsZCA9IDA7IH0KKyAgICBJbnB1dE11dGF0aW9uTGlzdGVuZXIoUmVuZGVyVGV4dEZp ZWxkICpyKSB7IG1fcmVuZGVyVGV4dEZpZWxkID0gcjsgfQorICAgIFJlbmRlclRleHRGaWVsZCAq cmVuZGVyVGV4dEZpZWxkKCkgY29uc3QgeyByZXR1cm4gbV9yZW5kZXJUZXh0RmllbGQ7IH0KKyAg ICB2b2lkIHNldElucHV0RWxlbWVudChSZW5kZXJUZXh0RmllbGQgKnIpIHsgbV9yZW5kZXJUZXh0 RmllbGQgPSByOyB9CisgICAgCisgICAgdmlydHVhbCB2b2lkIGhhbmRsZUV2ZW50KEV2ZW50TGlz dGVuZXJFdmVudCBldnQsIGJvb2wgaXNXaW5kb3dFdmVudCk7CisgICAgCitwcml2YXRlOgorICAg IFJlbmRlclRleHRGaWVsZCAqbV9yZW5kZXJUZXh0RmllbGQ7Cit9OworCiBjbGFzcyBSZW5kZXJU ZXh0RmllbGQgOiBwdWJsaWMgUmVuZGVyQmxvY2sKIHsKIHB1YmxpYzoKQEAgLTQ2LDggKzYzLDEx IEBACiAgICAgdm9pZCBzZWxlY3QoKTsKICAgICB2b2lkIHNldFNlbGVjdGlvblJhbmdlKGludCwg aW50KTsKIAorICAgIHZvaWQgc3VidHJlZUhhc0NoYW5nZWQoKTsKKwogcHJvdGVjdGVkOgogICAg IFJlZlB0cjxIVE1MRGl2RWxlbWVudEltcGw+IG1fZGl2OworICAgIFJlZlB0cjxJbnB1dE11dGF0 aW9uTGlzdGVuZXI+IG1fbXV0YXRpb25MaXN0ZW5lcjsKIH07CiAKIH07CkluZGV4OiByZW5kZXJp bmcvUmVuZGVyVGV4dEZpZWxkLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSByZW5kZXJpbmcvUmVuZGVyVGV4 dEZpZWxkLmNwcAkocmV2aXNpb24gMTI0MDEpCisrKyByZW5kZXJpbmcvUmVuZGVyVGV4dEZpZWxk LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjYsMTggKzI2LDM0IEBACiAjaW5jbHVkZSAiUmVuZGVy VGV4dC5oIgogI2luY2x1ZGUgImh0bWxuYW1lcy5oIgogI2luY2x1ZGUgIkhUTUxJbnB1dEVsZW1l bnRJbXBsLmgiCisjaW5jbHVkZSAieG1sL2RvbV9lbGVtZW50aW1wbC5oIgorI2luY2x1ZGUgInht bC9FdmVudE5hbWVzLmgiCisjaW5jbHVkZSAieG1sL2RvbTJfZXZlbnRzaW1wbC5oIgogCiBuYW1l c3BhY2UgV2ViQ29yZSB7CiAKIHVzaW5nIG5hbWVzcGFjZSBIVE1MTmFtZXM7Cit1c2luZyBuYW1l c3BhY2UgRXZlbnROYW1lczsKIAordm9pZCBJbnB1dE11dGF0aW9uTGlzdGVuZXI6OmhhbmRsZUV2 ZW50KEV2ZW50SW1wbCAqZXZlbnQsIGJvb2wgaXNXaW5kb3dFdmVudCkKK3sKKyAgICBpZiAoIW1f cmVuZGVyVGV4dEZpZWxkKQorICAgICAgICByZXR1cm47CisgICAgCisgICAgaWYgKGV2ZW50LT50 eXBlKCkgPT0gRE9NQ2hhcmFjdGVyRGF0YU1vZGlmaWVkRXZlbnQpCisgICAgICAgIG1fcmVuZGVy VGV4dEZpZWxkLT5zdWJ0cmVlSGFzQ2hhbmdlZCgpOworfQorCiBSZW5kZXJUZXh0RmllbGQ6OlJl bmRlclRleHRGaWVsZChOb2RlSW1wbCogbm9kZSkKLTpSZW5kZXJCbG9jayhub2RlKQorOlJlbmRl ckJsb2NrKG5vZGUpLCBtX211dGF0aW9uTGlzdGVuZXIobmV3IElucHV0TXV0YXRpb25MaXN0ZW5l cih0aGlzKSkKIHsKIH0KIAogUmVuZGVyVGV4dEZpZWxkOjp+UmVuZGVyVGV4dEZpZWxkKCkKIHsK KyAgICBpZiAobV9kaXYpCisgICAgICAgIG1fZGl2LT5yZW1vdmVFdmVudExpc3RlbmVyKERPTUNo YXJhY3RlckRhdGFNb2RpZmllZEV2ZW50LCBtX211dGF0aW9uTGlzdGVuZXIuZ2V0KCksIGZhbHNl KTsKKwogICAgIC8vIFRoZSByZW5kZXJlciBmb3IgdGhlIGRpdiBoYXMgYWxyZWFkeSBiZWVuIGRl c3Ryb3llZCBieSBkZXN0cm95TGVmdG92ZXJDaGlsZHJlbgogICAgIGlmIChtX2RpdikKICAgICAg ICAgbV9kaXYtPmRldGFjaCgpOwpAQCAtNDksOCArNjUsNiBAQAogICAgIGlmIChtX2Rpdikgewog ICAgICAgICBSZW5kZXJCbG9jayogZGl2UmVuZGVyZXIgPSBzdGF0aWNfY2FzdDxSZW5kZXJCbG9j ayo+KG1fZGl2LT5yZW5kZXJlcigpKTsKICAgICAgICAgUmVuZGVyU3R5bGUqIGRpdlN0eWxlID0g Y3JlYXRlRGl2U3R5bGUoc3R5bGUpOwotICAgICAgICBpZiAoZGl2UmVuZGVyZXItPmZpcnN0Q2hp bGQoKSkKLSAgICAgICAgICAgIGRpdlJlbmRlcmVyLT5maXJzdENoaWxkKCktPnNldFN0eWxlKGRp dlN0eWxlKTsKICAgICAgICAgZGl2UmVuZGVyZXItPnNldFN0eWxlKGRpdlN0eWxlKTsKICAgICB9 CiAgICAgc2V0UmVwbGFjZWQoaXNJbmxpbmUoKSk7CkBAIC0xMDIsNiArMTE2LDcgQEAKICAgICAg ICAgICAgIHRleHQgPSBuZXcgVGV4dEltcGwoZG9jdW1lbnQoKSwgdmFsdWUuaW1wbCgpKTsKICAg ICAgICAgICAgIGludCBleGNlcHRpb24gPSAwOwogICAgICAgICAgICAgbV9kaXYtPmFwcGVuZENo aWxkKHRleHQsIGV4Y2VwdGlvbik7CisgICAgICAgICAgICBtX2Rpdi0+YWRkRXZlbnRMaXN0ZW5l cihET01DaGFyYWN0ZXJEYXRhTW9kaWZpZWRFdmVudCwgbV9tdXRhdGlvbkxpc3RlbmVyLmdldCgp LCBmYWxzZSk7CiAgICAgICAgIH0KICAgICAgICAgCiAgICAgICAgIGlmICghaW5wdXQtPnZhbHVl TWF0Y2hlc1JlbmRlcmVyKCkpIHsKQEAgLTE1Miw0ICsxNjcsMTIgQEAKICAgICAvLyBGSVhNRTog SW1wbGVtZW50IHRoaXMuCiB9CiAKK3ZvaWQgUmVuZGVyVGV4dEZpZWxkOjpzdWJ0cmVlSGFzQ2hh bmdlZCgpCit7CisgICAgSFRNTElucHV0RWxlbWVudEltcGwqIGlucHV0ID0gc3RhdGljX2Nhc3Q8 SFRNTElucHV0RWxlbWVudEltcGwqPihlbGVtZW50KCkpOworICAgIFRleHRJbXBsKiB0ZXh0ID0g c3RhdGljX2Nhc3Q8VGV4dEltcGwqPihtX2Rpdi0+Zmlyc3RDaGlsZCgpKTsKKyAgICBpZiAoaW5w dXQgJiYgdGV4dCkKKyAgICAgICAgaW5wdXQtPnNldFZhbHVlRnJvbVJlbmRlcmVyKHRleHQtPmRh dGEoKSk7CiB9CisKK30KSW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJ KHJldmlzaW9uIDEyNDA1KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEs MjUgQEAKKzIwMDYtMDEtMjYgIEFkZWxlIFBldGVyc29uICA8YWRlbGVAYXBwbGUuY29tPgorCisg ICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIGh0dHA6Ly9idWd6 aWxsYS5vcGVuZGFyd2luLm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Njg2MgorICAgICAgICBJbnB1dCdz IHZhbHVlIGRvZXNuJ3QgZ2V0IHVwZGF0ZWQgYWZ0ZXIgdHlwaW5nIGluIG5ldyB0ZXh0IGZpZWxk CisKKyAgICAgICAgQWRkZWQgYW4gZXZlbnQgbGlzdGVuZXIgZm9yIGEgRE9NQ2hhcmFjdGVyRGF0 YU1vZGlmaWVkRXZlbnQgb24gdGhlIGlubmVyIGRpdi4KKyAgICAgICAgV2hlbiB3ZSBnZXQgdGhl IGV2ZW50LCB0aGVuIHdlIHVwZGF0ZSB0aGUgaW5wdXQgZWxlbWVudCdzIHZhbHVlIGZpZWxkLgor CisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlclRleHRGaWVsZC5jcHA6CisgICAgICAgIChXZWJD b3JlOjpJbnB1dE11dGF0aW9uTGlzdGVuZXI6OmhhbmRsZUV2ZW50KTogY2FsbHMgc3VidHJlZUhh c0NoYW5nZWQKKyAgICAgICAgKFdlYkNvcmU6Om1fbXV0YXRpb25MaXN0ZW5lcik6CisgICAgICAg IChXZWJDb3JlOjpSZW5kZXJUZXh0RmllbGQ6On5SZW5kZXJUZXh0RmllbGQpOiByZW1vdmUgdGhl IGV2ZW50IGxpc3RlbmVyLgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyVGV4dEZpZWxkOjpzZXRT dHlsZSk6IFJlbW92ZSB1bm5lY2Vzc2FyeSBzZXRTdHlsZSBvbiB0ZXh0IG5vZGUuICBJdCBzaG91 bGQgaW5oZXJpdCBzdHlsZSBmcm9tIHRoZSBkaXYgYWxyZWFkeS4KKyAgICAgICAgKFdlYkNvcmU6 OlJlbmRlclRleHRGaWVsZDo6dXBkYXRlRnJvbUVsZW1lbnQpOiBhZGQgdGhlIGV2ZW50IGxpc3Rl bmVyLgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyVGV4dEZpZWxkOjpzdWJ0cmVlSGFzQ2hhbmdl ZCk6IHVwZGF0ZXMgdGhlIGlucHV0IGVsZW1lbnQncyB2YWx1ZS4KKyAgICAgICAgKiByZW5kZXJp bmcvUmVuZGVyVGV4dEZpZWxkLmg6CisgICAgICAgIChXZWJDb3JlOjpJbnB1dE11dGF0aW9uTGlz dGVuZXI6OklucHV0TXV0YXRpb25MaXN0ZW5lcik6IEFkZGVkLgorICAgICAgICAoV2ViQ29yZTo6 SW5wdXRNdXRhdGlvbkxpc3RlbmVyOjpyZW5kZXJUZXh0RmllbGQpOiBBZGRlZC4KKyAgICAgICAg KFdlYkNvcmU6OklucHV0TXV0YXRpb25MaXN0ZW5lcjo6c2V0SW5wdXRFbGVtZW50KTogQWRkZWQu CisKIDIwMDYtMDEtMjYgIEJldGggRGFraW4gIDxiZGFraW5AYXBwbGUuY29tPgogCiAgICAgICAg IFJldmlld2VkIGJ5IEh5YXR0Lgo=