Closed Bug 811755 Opened 7 years ago Closed 7 years ago

Holding backspace in a designMode editor repositions cursor

Categories

(Firefox for Android :: Keyboards and IME, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 20
Tracking Status
firefox19 --- affected
firefox20 --- verified
fennec + ---

People

(Reporter: aaronmt, Assigned: jchen)

References

Details

(Keywords: reproducible, smoketest)

Attachments

(7 files, 5 obsolete files)

1.59 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
1.13 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
1.72 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
13.18 KB, patch
jchen
: review+
Details | Diff | Splinter Review
7.16 KB, patch
jchen
: review+
Details | Diff | Splinter Review
6.10 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
2.41 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Despite bug 805766 landing on mozilla-central, using the same test-case from bug 793696:

i. Visit https://etherpad.mozilla.org and create a new EtherPad
ii. Reposition the cursor anywhere in the default placeholder text
iii. Hold backspace

Notice that the cursor gets repositioned and thus undesirable text gets backspaced.
tracking-fennec: ? → +
Duplicate of this bug: 813848
This is now an issue on Aurora.
Duplicate bug 813848 was filed against 18, so this will need re-testing on that branch.
QA Contact: aaron.train
In SetRangeFromFlatTextOffset, it's necessary to use PreContentIterator instead of ContentIterator.

When the range end is at the end of a node, we set the range to the start of the next node [1]. If the node is the last child of a container, the next node will be different depending on if we use ContentIterator or PreContentIterator. For ContentIterator, the next node is the container itself, which is not what we want. For PreContentIterator, the next node is the node after the container, which is what we want.

[1] http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#428

For example, if | is the end and we want to move it to the start of the next node
> <div>text<br>|</div><br>
If we use ContentIterator, the next node is the container and | is moved back
> |<div>text<br></div><br>
But if we use PreContentIterator, the next node is the one after the container
> <div>text<br></div>|<br>
Attachment #687146 - Flags: review?(masayuki)
Some editor BR nodes are not counted in the length of the text (see IsContentBR [1]). So when editor creates such a BR node (e.g. by setting 'type' attribute to '_moz'), we need to create a text change notification.

[1] http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#129
Attachment #687148 - Flags: review?(masayuki)
When we send a replace text event to Gecko, the actual range that's altered might be larger than the range in the original request. As a result, we want to preserve any spans (selection, compositions, etc.) outside of the originally requested range.
Attachment #687150 - Flags: review?(cpeterson)
When an event such as keypress changes the text, we want to sync the Java-side selection right away instead of going through the normal post-to-UI mechanism.
Attachment #687153 - Flags: review?(cpeterson)
Comment on attachment 687140 [details] [diff] [review]
Discard stale selection notifications after text change (v1)

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

LGTM
Attachment #687140 - Flags: review?(cpeterson) → review+
Because OnIMETextChange is asynchronous, we cannot request the text content from inside it. If another text change happened in the mean time, any text that we request will be wrong. This patch collects all OnIMETextChange calls, and waits until a good time to request text content and notify Java. Similarly, selection changes are also collected and combined.
Attachment #687159 - Flags: review?(cpeterson)
Comment on attachment 687159 [details] [diff] [review]
Buffer and coalesce IME selection and text changes (v1)

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

Looking good, but I have a few questions:

::: widget/android/nsWindow.cpp
@@ +2155,5 @@
> +
> +    while (--dstIndex >= 0) {
> +        IMEChange &src = mIMETextChanges[srcIndex];
> +        IMEChange &dst = mIMETextChanges[dstIndex];
> +        if (src.mOldEnd < dst.mStart || dst.mNewEnd < src.mStart)

Are the mIMETextChanges ordered? Do we need to check for merging ranges that are not adjacent in the mIMETextChanges array?

@@ +2161,5 @@
> +            continue;
> +        if (src.mOldEnd < dst.mNewEnd) {
> +            // New range overlaps or is within previous range; merge
> +            dst.mStart = std::min(dst.mStart, src.mStart);
> +            dst.mNewEnd += src.mNewEnd - src.mOldEnd;

Can you add a comment explaining why we need to adjacent dst.mNewEnd by a delta but dst.mStart can just be assigned min(dst.mStart, src.mStart)?

@@ +2164,5 @@
> +            dst.mStart = std::min(dst.mStart, src.mStart);
> +            dst.mNewEnd += src.mNewEnd - src.mOldEnd;
> +        } else { // src.mOldEnd >= dst.mNewEnd
> +            // New range overlaps previous range; merge
> +            dst.mStart = std::min(dst.mStart, src.mStart);

You can consolidate your duplicated `dst.mStart = std::min(...)` code.

::: widget/android/nsWindow.h
@@ +213,5 @@
> +        {
> +            return mNewEnd >= 0;
> +        }
> +    };
> +    nsAutoTArray<IMEChange, 4> mIMETextChanges;

Where does the number 4 come from?
Attachment #687159 - Flags: review?(cpeterson) → feedback+
(In reply to Chris Peterson (:cpeterson) from comment #11)
> Comment on attachment 687159 [details] [diff] [review]
> Buffer and coalesce IME selection and text changes (v1)
> 
> Review of attachment 687159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good, but I have a few questions:
> 
> ::: widget/android/nsWindow.cpp
> @@ +2155,5 @@
> > +
> > +    while (--dstIndex >= 0) {
> > +        IMEChange &src = mIMETextChanges[srcIndex];
> > +        IMEChange &dst = mIMETextChanges[dstIndex];
> > +        if (src.mOldEnd < dst.mStart || dst.mNewEnd < src.mStart)
> 
> Are the mIMETextChanges ordered? Do we need to check for merging ranges that
> are not adjacent in the mIMETextChanges array?

If the adjacent one doesn't match, the loop will continue, dstIndex decrements, and the previous one is checked. This continues until all elements in the array are checked against src.

> 
> @@ +2161,5 @@
> > +            continue;
> > +        if (src.mOldEnd < dst.mNewEnd) {
> > +            // New range overlaps or is within previous range; merge
> > +            dst.mStart = std::min(dst.mStart, src.mStart);
> > +            dst.mNewEnd += src.mNewEnd - src.mOldEnd;
> 
> Can you add a comment explaining why we need to adjacent dst.mNewEnd by a
> delta but dst.mStart can just be assigned min(dst.mStart, src.mStart)?

Okay

> 
> @@ +2164,5 @@
> > +            dst.mStart = std::min(dst.mStart, src.mStart);
> > +            dst.mNewEnd += src.mNewEnd - src.mOldEnd;
> > +        } else { // src.mOldEnd >= dst.mNewEnd
> > +            // New range overlaps previous range; merge
> > +            dst.mStart = std::min(dst.mStart, src.mStart);
> 
> You can consolidate your duplicated `dst.mStart = std::min(...)` code.

Okay

> 
> ::: widget/android/nsWindow.h
> @@ +213,5 @@
> > +        {
> > +            return mNewEnd >= 0;
> > +        }
> > +    };
> > +    nsAutoTArray<IMEChange, 4> mIMETextChanges;
> 
> Where does the number 4 come from?

From my testing, the most number of simultaneous changes is 3 or 4, so I just picked 4 as the starting buffer size.
Status: NEW → ASSIGNED
Attachment #687159 - Attachment is obsolete: true
Attachment #687773 - Flags: review?(cpeterson)
Comment on attachment 687153 [details] [diff] [review]
Synchronize selection when selection changes due to events (v1)

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

LGTM
Attachment #687153 - Flags: review?(cpeterson) → review+
Comment on attachment 687150 [details] [diff] [review]
Preserve selection and spans during replacement actions (v1)

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

LGTM with one suggestion:

::: mobile/android/base/GeckoEditable.java
@@ +624,5 @@
>          } else {
> +            // Don't use replace() because Gingerbread has a bug where if the replaced text
> +            // has the same spans as the original text, the spans will end up being deleted
> +            mText.delete(start, oldEnd);
> +            mText.insert(start, mChangedText);

This delete/insert code and comment is repeated three times in this patch. Perhaps you should consolidate the code into a private helper method? This method would effectively be our own Gingerbread-safe replace().
Attachment #687150 - Flags: review?(cpeterson) → review+
Duplicate of this bug: 786450
Duplicate of this bug: 817773
Comment on attachment 687773 [details] [diff] [review]
Buffer and coalesce IME selection and text changes (v2)

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

LGTM with some nits:

::: widget/android/nsWindow.cpp
@@ +2077,5 @@
> +nsWindow::PostFlushIMEChanges()
> +{
> +    if (!mIMETextChanges.IsEmpty() || !mIMESelectionChange.IsEmpty())
> +        // Already posted
> +        return;

I think we should put braces} around this `if` statement's code block. Even though there is only one line of code, the comment line makes the formatting a little misleading.

@@ +2156,5 @@
> +    while (--dstIndex >= 0) {
> +        IMEChange &src = mIMETextChanges[srcIndex];
> +        IMEChange &dst = mIMETextChanges[dstIndex];
> +        // When merging a more recent change into an older
> +        // chenge, we need to compare recent change's (start, oldEnd)

s/chenge/change/

@@ +2160,5 @@
> +        // chenge, we need to compare recent change's (start, oldEnd)
> +        // range to the older change's (start, newEnd)
> +        if (src.mOldEnd < dst.mStart || dst.mNewEnd < src.mStart)
> +            // No overlap between ranges
> +            continue;

I think we should put braces} around this `if` statement's code block. Even though there is only one line of code, the comment line makes the formatting a little misleading.
Attachment #687773 - Flags: review?(cpeterson) → review+
Comment on attachment 687148 [details] [diff] [review]
Send notification when BR nodes become content/non-content (v1)

>@@ -74,16 +76,17 @@ public:
>   nsCOMPtr<nsIContent>           mRootContent;
>   nsCOMPtr<nsINode>              mEditableNode;
> 
> private:
>   void NotifyContentAdded(nsINode* aContainer, int32_t aStart, int32_t aEnd);
>   void ObserveEditableNode();
> 
>   bool mObserving;
>+  uint32_t mPreAttrChangeLength;
> };
> 
> /******************************************************************/
> /* nsIMEStateManager                                              */
> /******************************************************************/
> 
> nsIContent*    nsIMEStateManager::sContent      = nullptr;
> nsPresContext* nsIMEStateManager::sPresContext  = nullptr;
>@@ -971,16 +974,74 @@ nsTextStateManager::ContentRemoved(nsIDocument* aDocument,
>     return;
> 
>   // fire notification
>   if (childOffset)
>     nsContentUtils::AddScriptRunner(
>         new TextChangeEvent(this, offset, offset + childOffset, offset));
> }
> 
>+static nsIContent*
>+GetContentBR(mozilla::dom::Element *aElement) {
>+  if (!aElement->IsNodeOfType(nsINode::eCONTENT))
>+    return nullptr;

Please use {} even if it has only one line.

>+  nsIContent *content = static_cast<nsIContent*>(aElement);
>+  if (!content->IsHTML(nsGkAtoms::br))
>+    return nullptr;
>+  return content;

return content->IsHTML(nsGkAtoms::br) ? content : nullptr;

>+}
>+
>+static uint32_t
>+GetContentBRLength(nsIContent *aContent)
>+{
>+  nsIContent *parent = aContent->GetParent();
>+  if (!parent)
>+    return 0;

Use {}.

>+
>+  uint32_t length;
>+  // Measure the length from start of node to start of next sibling
>+  if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>+      aContent, parent, parent->IndexOf(aContent) + 1, &length)))
>+    return 0;

Use {}.

>+  return length;
>+}

But I wonder nsContentEventHandler::GetFlatTextOffsetOfRange() could fail in this situation? If it's not, I'd like:

nsresult rv =
  nsContentEventHandler::GetFlatTextOffsetOfRange(
    aContent, parent, parent->IndexOf(aContent) + 1, &length));
NS_ENSURE_SUCCESS(rv, 0);
return length;

Or, perhaps, there is another simple approach. That is, I think that it's OK to make IsContentBR() public. I mean it should be nsContentEventHander::IsContentBR(). Then, we can skip a lot of work which is performed in GetFlatTextOffsetOfRange(). Because your patch is really focusing on the change. So, typically, your approach is safer but I think checking IsContentBR() directly is OK in this situation.

>+
>+void
>+nsTextStateManager::AttributeWillChange(nsIDocument* aDocument,
>+                                        mozilla::dom::Element* aElement,
>+                                        int32_t      aNameSpaceID,
>+                                        nsIAtom*     aAttribute,
>+                                        int32_t      aModType)
>+{
>+  nsIContent *content = GetContentBR(aElement);
>+  if (!content)
>+    return;

Use {}.

>+  mPreAttrChangeLength = GetContentBRLength(content);
>+}
>+
>+void
>+nsTextStateManager::AttributeChanged(nsIDocument* aDocument,
>+                                     mozilla::dom::Element* aElement,
>+                                     int32_t aNameSpaceID,
>+                                     nsIAtom* aAttribute,
>+                                     int32_t aModType)
>+{
>+  nsIContent *content = GetContentBR(aElement);
>+  if (!content)
>+    return;

Use {}.
Addressed comments. Carried over r+.
Attachment #687773 - Attachment is obsolete: true
Attachment #688310 - Flags: review+
Addressed review comments. Carried over r+.
Attachment #687150 - Attachment is obsolete: true
Attachment #688311 - Flags: review+
How about this? I exposed nsContentEventHandler::GetNativeTextLength, which calls IsContentBR and fits into the code nicely.
Attachment #687148 - Attachment is obsolete: true
Attachment #687148 - Flags: review?(masayuki)
Attachment #688316 - Flags: review?(masayuki)
Comment on attachment 688316 [details] [diff] [review]
Send notification when BR nodes become content/non-content (v2)

Yeah, looks OK.

>+void
>+nsTextStateManager::AttributeWillChange(nsIDocument* aDocument,
>+                                        mozilla::dom::Element* aElement,
>+                                        int32_t      aNameSpaceID,
>+                                        nsIAtom*     aAttribute,
>+                                        int32_t      aModType)
>+{
>+  nsIContent *content = GetContentBR(aElement);
>+  mPreAttrChangeLength = content ?
>+                         nsContentEventHandler::GetNativeTextLength(content) :
>+                         0;
>+}

Following style can reduce the last line:

mPreAttrChangeLength = content ?
  nsContentEventHandler::GetNativeTextLength(content) : 0;

>+  uint32_t postAttrChangeLength =
>+      nsContentEventHandler::GetNativeTextLength(content);

remove 2 white spaces at the second line.

>+  if (postAttrChangeLength != mPreAttrChangeLength) {
>+    uint32_t start;
>+    if (NS_SUCCEEDED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>+        mRootContent, content, 0, &start))) {
>+      nsContentUtils::AddScriptRunner(new TextChangeEvent(this, start,
>+          start + mPreAttrChangeLength, start + postAttrChangeLength));

Same here at the last line.

Otherwise, looks great. Thank you.
Attachment #688316 - Flags: review?(masayuki) → review+
And if you can make a simple test for attachment 687146 [details] [diff] [review], please add it to window_composition_text_querycontent.xul. I guess that we can reproduce the same bug in contenteditable editor too. So, either editor is okay for testing it.
Sorry; spotted a mistake.
Attachment #689394 - Attachment is obsolete: true
Attachment #689394 - Flags: review?(masayuki)
Attachment #689399 - Flags: review?(masayuki)
Comment on attachment 689399 [details] [diff] [review]
Add test for PreContentIterator in nsContentEventHandler (v2)

Thank you very much!
Attachment #689399 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/0c0bc1f99130
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 821229
No longer depends on: 821229
Unable to reproduce the issue on Aurora 20.0a2 2013-01-10 using the HTC Desire Z (Android 2.3.3)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.