Caret/Cursor jumps around while entering text in etherpad's rich text editor

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
7 years ago
6 years ago

People

(Reporter: blassey, Assigned: snorp)

Tracking

Trunk
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

STR:
1) load an etherpad (go here http://etherpad.mozilla.com:9000/ and click "create new pad")
2) click in the rich text editor
3) hold the menu key to bring up the softkeyboard
4) type

ER: entered text goes where the caret is
AR: text goes where ever it feels like

The caret seems to have a preference for being at the top of the editor, so placing it at the bottom is the most reliable way to reproduce
If the fix is safe, we might consider for Fx7
tracking-fennec: ? → 8+
(Reporter)

Updated

7 years ago
Assignee: nobody → alexp
tracking-fennec: 8+ → +

Comment 2

7 years ago
Is this something to be fixed in the core editor?  Do you have a regression range?
For some reason we don't get IME selection change events when the caret is positioned.  I'll take this one.
Assignee: alexp → snorp
Created attachment 555731 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME
Depends on: 669995
Attachment #555731 - Flags: review?(alexp)
Comment on attachment 555731 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

I believe someone who really knows this code should review this change to make sure we are doing the right thing. I guess enn or smaug.
Attachment #555731 - Flags: review?(alexp) → review?(enndeakin)
Created attachment 556093 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME
Attachment #555731 - Attachment is obsolete: true
Attachment #555731 - Flags: review?(enndeakin)
Attachment #556093 - Flags: review?(enndeakin)
Can you explain why the first change is needed?

Masayuki Nakano is the better reviewer for the IME change.
The first change is necessary because the focus is first cleared, which sets the mFocusedContent to null (in Blur).  This causes us to short circuit early in Focus when we have the doc focused.

The second change is necessary because that's just where the editable doc is picked up for focus.
Attachment #556093 - Flags: review?(enndeakin) → review?(masayuki)
The patch on Bug 670694 obsoletes the first part of this patch, so only the additional IME call is needed.
I don't understand why it's necessary... At removing focus, calling nsIMEStateManager::OnTextStateBlur(presContext, nsnull); should be enough for nsIMEStateManager. Why does CheckIfFocusable() return NULL when an editor has focus?
(In reply to Masayuki Nakano (Mozilla Japan)(offline: 8/31 and 9/1) from comment #10)
> I don't understand why it's necessary... At removing focus, calling
> nsIMEStateManager::OnTextStateBlur(presContext, nsnull); should be enough
> for nsIMEStateManager.

We could make it work, perhaps, but it's necessary with the current logic.  In OnTextStateBlur, we don't try to find an editable doc to focus.  We do that in OnTextStateFocus (really, GetRootEditableNode) when a null element is passed.

> Why does CheckIfFocusable() return NULL when an
> editor has focus?

Because CheckIfFocusable requires a non-null element.  When a document has focus, the element is null.  At one point I tried making that return the document if it was editable, but ran in to some other issue (that I don't remember now).
Hmm, I think that the approach is wrong.

I guess that when aContent is NULL but the document is editable, nsIMEStateManager should think that the root element (or the body element?) gets focus. I mean, I think you should make another else if block before the block which you changed.

I.e., you should make second block of following code.

if (CheckIfFocusable() && ...) {

} else if (doc->editable) {

} else {

}

And also we might be able to remove the special code for this case in nsIMEStateManager. E.g., http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#259
Created attachment 557229 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME
Attachment #557229 - Flags: review?(masayuki)
Attachment #556093 - Attachment is obsolete: true
Attachment #556093 - Flags: review?(masayuki)
(In reply to Masayuki Nakano (Mozilla Japan)(offline: 8/31 and 9/1) from comment #12)

Updated patch which takes the recommended approach.  Mostly it's the same code, though, since the special case in nsIMEEventStateManager can't easily be removed (see comment in patch).
Comment on attachment 557229 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

Sorry for the delay.

> +  } else if (mFocusedWindow == aWindow && mFocusedContent == nsnull &&
> +             presContext->Document()->IsEditable()) {

I think that |mFocusedWindow == aWindow && mFocusedContent == nsnull| should be set to a PRBool variable before |if|.  It's checked in all blocks.

> +    SetFocusedContent(nsnull, aFlags);

I'm not sure whether this is necessary.

> +    nsIMEStateManager::OnChangeFocus(presContext, nsnull, reason);
> +    nsIMEStateManager::OnTextStateFocus(presContext, nsnull);
> +
> +    if (!aWindowRaised)
> +      aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));

In the if block, aWindow->UpdateCommands() is called before nsIMEStateManager::OnTextStateFocus(). I'm not sure whether the order isn't matter or not so.

Therefore, I need Enn's review.
Attachment #557229 - Flags: review?(enndeakin)
Created attachment 558467 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME (v2)
Attachment #558467 - Attachment description: Bug 670776 - fix focus issue with contentEditable documents in iframes and IME → Bug 670776 - fix focus issue with contentEditable documents in iframes and IME (v2)
(In reply to Masayuki Nakano (Mozilla Japan) from comment #15)
> Comment on attachment 557229 [details] [diff] [review]
> Bug 670776 - fix focus issue with contentEditable documents in iframes and
> IME
> 
> Sorry for the delay.
> 
> > +  } else if (mFocusedWindow == aWindow && mFocusedContent == nsnull &&
> > +             presContext->Document()->IsEditable()) {
> 
> I think that |mFocusedWindow == aWindow && mFocusedContent == nsnull| should
> be set to a PRBool variable before |if|.  It's checked in all blocks.
> 
> > +    SetFocusedContent(nsnull, aFlags);
> 
> I'm not sure whether this is necessary.
> 
> > +    nsIMEStateManager::OnChangeFocus(presContext, nsnull, reason);
> > +    nsIMEStateManager::OnTextStateFocus(presContext, nsnull);
> > +
> > +    if (!aWindowRaised)
> > +      aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));
> 
> In the if block, aWindow->UpdateCommands() is called before
> nsIMEStateManager::OnTextStateFocus(). I'm not sure whether the order isn't
> matter or not so.
> 
> Therefore, I need Enn's review.
Depends on: 670694
(In reply to Masayuki Nakano (Mozilla Japan) from comment #15)
> Comment on attachment 557229 [details] [diff] [review]
> Bug 670776 - fix focus issue with contentEditable documents in iframes and
> IME
> 
> Sorry for the delay.
> 
> > +  } else if (mFocusedWindow == aWindow && mFocusedContent == nsnull &&
> > +             presContext->Document()->IsEditable()) {
> 
> I think that |mFocusedWindow == aWindow && mFocusedContent == nsnull| should
> be set to a PRBool variable before |if|.  It's checked in all blocks.

Done.

> 
> > +    SetFocusedContent(nsnull, aFlags);
> 
> I'm not sure whether this is necessary.

That's actually from 670694, which this bug really should depend on (fixed).  We have to set the flags there so that they are correct when we compare them next time (see 670694).

> 
> > +    nsIMEStateManager::OnChangeFocus(presContext, nsnull, reason);
> > +    nsIMEStateManager::OnTextStateFocus(presContext, nsnull);
> > +
> > +    if (!aWindowRaised)
> > +      aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));
> 
> In the if block, aWindow->UpdateCommands() is called before
> nsIMEStateManager::OnTextStateFocus(). I'm not sure whether the order isn't
> matter or not so.
> 
> Therefore, I need Enn's review.
Comment on attachment 557229 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

>-  }
>-  else {
>+  } else if (mFocusedWindow == aWindow && mFocusedContent == nsnull &&
>+             presContext->Document()->IsEditable()) {
>+    SetFocusedContent(nsnull, aFlags);
>+
>+    // Editable document has focus, let IME know about it.  If we pass
>+    // null content (as we do below), IME knows to inspect the document
>+    // to see if it is editable and Do The Right Thing.  We can't pass
>+    // the document directly, because IME expects a nsIContent*.
>+    nsIMEStateManager::OnTextStateBlur(presContext, nsnull);
>+
>+    PRUint32 reason = GetFocusMoveReason(aFlags);
>+    nsIMEStateManager::OnChangeFocus(presContext, nsnull, reason);
>+    nsIMEStateManager::OnTextStateFocus(presContext, nsnull);
>+
>+    if (!aWindowRaised)
>+      aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));
>+  } else {

You should be able to combine some of these lines with the following else block. The call to UpdateCommands for example could be written only once.


>     // If the window focus event (fired above when aIsNewDocument) caused
>     // the plugin not to be focusable, update the system focus by focusing
>     // the root widget.
>     if (aAdjustWidgets && objectFrameWidget &&
>         mFocusedWindow == aWindow && mFocusedContent == nsnull) {
>       nsIViewManager* vm = presShell->GetViewManager();
>       if (vm) {
>         nsCOMPtr<nsIWidget> widget;
>         vm->GetRootWidget(getter_AddRefs(widget));
>         if (widget)
>           widget->SetFocus(PR_FALSE);
>       }
>     }

I would think that this plugin related code should be called in both cases.
Some combination of IME changes have made this work now.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
Attachment #557229 - Flags: review?(enndeakin)
Attachment #557229 - Flags: review?(masayuki)
You need to log in before you can comment on or make changes to this bug.