1045270 - Dynamically inserting a block into an inline node causes number input to lose focus

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Caitlin Potter [:caitp]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.125 Safari/537.36

Steps to reproduce:

Visit http://jsfiddle.net/fMw58/, enter any value you like


Actual results:

Focus will be lost after each keypress when entering a valid number value


Expected results:

Dynamically inserting a node should not affect the focus of a different form control.

This does not occur when the elements are inside of display: block elements, such as a div: http://jsfiddle.net/7RHJB/ --- this also does not seem to occur for text inputs http://jsfiddle.net/LeyDK/.

This issue was originally reported at https://github.com/angular/angular.js/issues/8365

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the issue here is that block-inside-inline splits trigger recreation of the box tree when you insert more blocks, and presumably we're losing the focus in the process of doing that.

I expect we somehow special-case text inputs to avoid the focus lossage and should do the same with number ones...

Comment 2

[(no longer active)]

Hmm, I suspect there are some things missing from http://jsfiddle.net/fMw58/.  There is an <html-unknown-element> in the HTML pane, the variable |text| is undefined, etc...

Caitlin, do you mind taking a look at the fiddle please?  I'd like to reproduce this so that I can look at the behavior difference between <input type=text> and <input type=number>.

(In reply to Boris Zbarsky [:bz] from comment #1)
> I expect we somehow special-case text inputs to avoid the focus lossage and
> should do the same with number ones...

Hmm, I can't think of anything we do to avoid losing focus for text inputs...  We do restore the selection after the frame reconstruction, but I expect that is not what you mean?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> There is an <html-unknown-element> in the HTML pane

Yes, that's the inline element in question.

> the variable |text| is undefined

Which means you get window.text, which is the element with id="text".  Yes, that's annoying.

> I'd like to reproduce this

1)  Load the fiddle.
2)  Click the number input.
3)  Type "1".

At this point the input loses focus.

Comment 4

[Caitlin Potter [:caitp]]

It's a terrible feature of the platform, but it's less typing than getElementById() or querySelector for quick reproductions =)

And yes, <html-unknown-element> can be any inline element, but in the original issue it was an HTMLUnknownElement causing the problem.

Should be able to reproduce it with bz's steps

Comment 5

[(no longer active)]

Oh, sorry, yeah I was mistakenly typing in non-digit characters, which is why I thought I couldn't reproduce.

What's happening here is that this code <http://dxr.mozilla.org/mozilla-central/source/layout/forms/nsNumberControlFrame.cpp#370> is failing because the anonymous element is not in a document yet.

Comment 6

[(no longer active)]

I have a fix, this needs a bit of work, because the text editor state's selection properties also needs to be cached.

Comment 7

[(no longer active)]

Attachment: Part 1: Cache the selection state for the native anonymous text box inside number controls on the number control itself; r=smaug

This is needed because the text control and its nsTextEditorState will
die together with the text control's frame.

Comment 8

[(no longer active)]

Attachment: Part 2: Focus the text control inside a number control when recreating frames for a focused number control off of the event loop; r=smaug

This is needed because the text control doesn't have a document yet in
CreateAnonymousContent, so setting the focus to it will fail.

Comment 9

[(no longer active)]

Attachment: Part 3: Add a test case; r=smaug

Comment 10

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=6946d5e1e14b

Comment 11

[(no longer active)]

So this test hits the assertion in SetSelectionProperties: <https://tbpl.mozilla.org/php/getParsedLog.php?id=44852048&tree=Try&full=1#error0>.

I debugged this, and it happens because this function might be called during the frame reconstruction after changing the input's type from number to something else.  So I need to modify GetParentNumberControl to check for the type explicitly.

Comment 12

[(no longer active)]

Attachment: Part 1: Cache the selection state for the native anonymous text box inside number controls on the number control itself; r=smaug

This is needed because the text control and its nsTextEditorState will
die together with the text control's frame.

Comment 13

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=ded3fa916a1b

Comment 14

[(no longer active)]

Comment on attachment 8464430 [details] [diff] [review]
Part 1: Cache the selection state for the native anonymous text box inside number controls on the number control itself; r=smaug

I also need to initialize mSelectionCached to true as in nsTextEditorState.

Comment 15

[(no longer active)]

Attachment: Part 1: Cache the selection state for the native anonymous text box inside number controls on the number control itself; r=smaug

This is needed because the text control and its nsTextEditorState will
die together with the text control's frame.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8464666 [details] [diff] [review]
Part 1: Cache the selection state for the native anonymous text box inside number controls on the number control itself; r=smaug

>+  void SetSelectionProperties(const nsTextEditorState::SelectionProperties& props)
aProps

>+  /**
>+   * The selection properties cache for number controls.  This is needed because
>+   * the number controls recycle their text field, so the normal cache in
>+   * nsTextEditorState cannot do its job.
I don't understand the comment about recycling. They explicitly don't recycle anything but
recreate the text field whenever frame is created for the input type="number" element.


>+  bool selectionCached = mSelectionCached;
>+  HTMLInputElement* number = GetParentNumberControl(mBoundFrame);
>+  if (number) {
>+    selectionCached = number->IsSelectionCached();
>+  }
>+  if (selectionCached) {

This would be a bit simpler, IMO, if you did
HTMLInputElement* number = GetParentNumberControl(mBoundFrame);
if (number ? number->IsSelectionCached() : mSelectionCached) {
but either way


>+nsTextEditorState::GetParentNumberControl(nsFrame* aFrame) const
>+{
>+  MOZ_ASSERT(aFrame);
>+  do {
>+    aFrame = aFrame->GetParent();
>+    nsNumberControlFrame* numberFrame = do_QueryFrame(aFrame);
>+    if (numberFrame) {
>+      HTMLInputElement* input =
>+        HTMLInputElement::FromContent(numberFrame->GetContent());
>+      // This function might be called during frame reconstruction as a result
>+      // of changing the input control's type from number to something else. In
>+      // that situation, the type of the control has changed, but its frame has
>+      // not been reconstructed yet.  So we need to check the type of the input
>+      // control in addition to the type of the frame.
>+      return (input->GetType() == NS_FORM_INPUT_NUMBER) ? input : nullptr;
>+    }
>+  } while (aFrame);
>+
>+  return nullptr;
>+}
So we may end up traversing up to the frame tree root :/
Do we really need to play with frame tree here?
Why aFrame->GetContent()->GetParent()->GetParent() doesn't return the right value?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8464385 [details] [diff] [review]
Part 2: Focus the text control inside a number control when recreating frames for a focused number control off of the event loop; r=smaug


>+class FocusTextField : public nsRunnable {
{ to its own line

>   if (mContent->AsElement()->State().HasState(NS_EVENT_STATE_FOCUS)) {
>     // We don't want to focus the frame but the text field.
>-    nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>-    nsCOMPtr<nsIDOMElement> element = do_QueryInterface(mTextField);
>-    NS_ASSERTION(element, "Really, this should be a nsIDOMElement!");
>-    fm->SetFocus(element, 0);
>+    nsRefPtr<FocusTextField> focusJob = new FocusTextField(mContent, mTextField);
>+    NS_DispatchToMainThread(focusJob);
Why you want thread runnable, and not a script runner?
The latter should be less error prone: if there are tons of runnables in the queue, we might
process some key events from OS before processing FocusTextField, and the key events might get 
lost.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8464386 [details] [diff] [review]
Part 3: Add a test case; r=smaug

So this shouldn't use then executeSoon, but after setting display to '', perhaps
force a layout flush, and then synthesizeKey("2", {}); should just work.

Comment 19

[(no longer active)]

Comment on attachment 8464386 [details] [diff] [review]
Part 3: Add a test case; r=smaug

(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8464386 [details] [diff] [review]
> Part 3: Add a test case; r=smaug
> 
> So this shouldn't use then executeSoon, but after setting display to '',
> perhaps
> force a layout flush, and then synthesizeKey("2", {}); should just work.

No, I did that very intentionally, to simulate what happens when a user types into the field.  I'd like to test the real configuration.

Comment 20

[(no longer active)]

Attachment: Part 1: Cache the selection state for the native anonymous text box inside number controls on the number control itself; r=smaug

This is needed because the text control and its nsTextEditorState will
die together with the text control's frame.

Comment 21

[(no longer active)]

Attachment: Part 2: Focus the text control inside a number control when recreating frames for a focused number control off of the event loop; r=smaug

This is needed because the text control doesn't have a document yet in
CreateAnonymousContent, so setting the focus to it will fail.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

executeSoon doesn't test reality more than just sync handling here.
executeSoon spins event loop, but key event dispatch doesn't require new runnables to be processed
between key events, IIRC. (I should re-check appshell code)

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8464386 [details] [diff] [review]
Part 3: Add a test case; r=smaug

But fine, we can do this too.
Though, shouldn't you ensure that frames are recreated.
So add a document.body.offsetLeft or some such after 
document.body.style.display = "";

Comment 24

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #23)
> Though, shouldn't you ensure that frames are recreated.
> So add a document.body.offsetLeft or some such after 
> document.body.style.display = "";

Will do.

Comment 25

[(no longer active)]

Attachment: Part 1: Cache the selection state for the native anonymous text box inside number controls on the number control itself; r=smaug

This is needed because the text control and its nsTextEditorState will
die together with the text control's frame.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8465403 [details] [diff] [review]
Part 1: Cache the selection state for the native anonymous text box inside number controls on the number control itself; r=smaug

This all could use some cleanups so that number doesn't need to be special cased
in so many places. But that could happen when more input types are implemented.

Comment 27

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be15df0608d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e7c384f7f39
https://hg.mozilla.org/integration/mozilla-inbound/rev/834d7d50b741

Comment 28

[Caitlin Potter [:caitp]]

☆.。.：・゜☆ Muchas gracias! ☆.。.：・゜☆

Comment 29

[(no longer active)]

Thanks for the great and detailed bug report!

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1be15df0608d
https://hg.mozilla.org/mozilla-central/rev/4e7c384f7f39
https://hg.mozilla.org/mozilla-central/rev/834d7d50b741

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1be15df0608d
https://hg.mozilla.org/mozilla-central/rev/4e7c384f7f39
https://hg.mozilla.org/mozilla-central/rev/834d7d50b741