Focus on div element after DOM manipulation

RESOLVED FIXED in mozilla35



3 years ago
3 years ago


(Reporter: Ryan Adler, Assigned: mats)



Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(3 attachments)



3 years ago
Created attachment 8482564 [details]

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.
Keywords: qawanted


3 years ago
Component: Untriaged → DOM
Product: Firefox → Core
Flags: needinfo?(ehsan.akhgari)

Comment 1

3 years ago
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?
Depends on: 1060579
Flags: needinfo?(ehsan.akhgari) → needinfo?(bugs)

Comment 2

3 years ago
Don't have time right now, and Mats might actually have this code in mind given the other patches he has written lately.
Flags: needinfo?(bugs)

Comment 3

3 years ago
Mats, would you like to take this, please? :)
Flags: needinfo?(mats)

Comment 4

3 years ago
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.
Component: DOM → Editor
No longer depends on: 1060579
Flags: needinfo?(mats)

Comment 5

3 years ago
Created attachment 8486068 [details] [diff] [review]

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.
Assignee: nobody → mats
Attachment #8486068 - Flags: review?(ehsan.akhgari)

Comment 6

3 years ago
Created attachment 8486070 [details] [diff] [review]

Try run (that also included bug 739396):

Comment 7

3 years ago
Comment on attachment 8486068 [details] [diff] [review]

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.
Attachment #8486068 - Flags: review?(ehsan.akhgari) → review+

Comment 8

3 years ago
With nits addressed as suggested:
Flags: in-testsuite+
Keywords: qawanted → testcase
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 35 Branch → unspecified
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.