Bug 6862 – Input's value doesn't get updated after typing in new text field

• Component: Forms
• Status: RESOLVED FIXED

[Adele Peterson]

[Adele Peterson]

Created attachment 6007 [details]
test case

[Adele Peterson]

Created attachment 6008 [details]
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?

[Eric Seidel (no email)]

Comment on attachment 6008 [details]
patch

To answer adele's question, "textContent()" can be used instead of innerText.

[Maciej Stachowiak]

Comment on attachment 6008 [details]
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).

[Darin Adler]

Comment on attachment 6008 [details]
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).

[Maciej Stachowiak]

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.

[Adele Peterson]

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

[Justin Garcia]

(In reply to comment #4)
> (From update of attachment 6008 [details] [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>).

[Alexey Proskuryakov]

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

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

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.

[Adele Peterson]

we decided to reevaluate the performance problems before we flip the switch.