1061468 - Focus on div element after DOM manipulation

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Ryan Adler]

Attachment: test.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.102 Safari/537.36

Steps to reproduce:

Create a contenteditable element with focus.  Programmatically move the element to another position.  Focus the element again (or not).


Actual results:

Caret and focus stays in the old position, even if focus() is called again.


Expected results:

Caret and focus should have been on the newly positioned element.

Comment 1

[(no longer active)]

I actually think that this is the same underlying issue as bug 1060579.  Bug 1060579 comment 6 suggests a fix.  I wasn't thinking about nsCaret relying on selection listeners, we should probably fix this.

Olli, do you happen to have the cycles?

Comment 2

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

Don't have time right now, and Mats might actually have this code in mind given the other patches he has written lately.

Comment 3

[(no longer active)]

Mats, would you like to take this, please? :)

Comment 4

[Mats Palmgren (inactive)]

I don't think this is related to bug 1060579 at all.
IMO, this is a bug in Editor since it depends on blur
events to shutdown the selection (FinalizeSelection).
We don't blur the focused element when it's removed from
the document (unlike Chrome).  (fwiw, I think Chrome's
behavior actually makes sense here.)

What happens is that the Range gravitates when the
node is removed, resulting in its start/end container
being the parent of the ancestor-limiter that Editor
assigned.  We could make Ranges aware of the limiter
and not gravitate above that point, but then we'll
have invalid (disconnected) Ranges instead, which isn't
pleasant either.  And it wouldn't be spec compliant,
nor UA compatible (IE, Presto, Chrome gravitates too).

So, I'd prefer to fix this by detecting this specific
case and call FinalizeSelection on the editor as if
the element was blur-ed.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

I piggy-backed nsFocusManager::ContentRemoved since it already has
code to detect when the focused element is removed (and we're only
interested in the focus case since Editor::Blur takes care of it
otherwise).  Alternatively, we could make nsEditor observe mutations
but it would be slower.

Comment 6

[Mats Palmgren (inactive)]

Attachment: test

Try run (that also included bug 739396):
https://tbpl.mozilla.org/?tree=Try&rev=dc301974fe74
https://tbpl.mozilla.org/?tree=Try&rev=b1ba440f2188

Comment 7

[(no longer active)]

Comment on attachment 8486068 [details] [diff] [review]
fix

Review of attachment 8486068 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsISelectionPrivate.idl
@@ +85,1 @@
>      [noscript] void setAncestorLimiter(in nsIContent aContent);

Nit: should we instead have:

[noscript] attribute nsIContent ancestorLimiter;

::: dom/base/nsFocusManager.cpp
@@ +849,5 @@
> +            selection->GetAncestorLimiter(getter_AddRefs(limiter));
> +            if (limiter == content) {
> +              editor->FinalizeSelection();
> +            }
> +          }         

Nit: trailing whitespace.

::: editor/nsIEditor.idl
@@ +44,5 @@
>  
>    /**
> +   * Finalizes selection and caret for the editor.
> +   */
> +  void finalizeSelection();

Nit: [noscript], please.

Comment 8

[Mats Palmgren (inactive)]

With nits addressed as suggested:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/004917420079
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba3143c153c

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/004917420079
https://hg.mozilla.org/mozilla-central/rev/0ba3143c153c