Closed Bug 847983 Opened 8 years ago Closed 2 years ago

IME in designmode doesn't seem to work very well

Categories

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

Product:
Firefox for Android
Component:
Keyboards and IME
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INACTIVE

People

(Reporter: martijn.martijn, Assigned: jchen)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: testcase, Whiteboard: [leave open])

Attachments

(7 files, 3 obsolete files)

catlog
36.50 KB, text/plain
Details
Catlog2
60.51 KB, text/plain
Details
Skip script text nodes for content events (v1)
1.97 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Try harder to get a valid frame (v1)
909 bytes, patch
masayuki
: review+
Details | Diff | Splinter Review
Don't include empty elements at end of range (v1.1)
3.07 KB, patch
jchen
: review+
Details | Diff | Splinter Review
Skip script text node for range-to-index conversion (v1)
1.16 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Flush layout and update offsets when text change events are run (v3)
7.50 KB, patch
Details | Diff | Splinter Review 
Tested on the Galaxy Nexus, using current trunk build.

Go to http://people.mozilla.org/~mwargers/tests/contenteditable/designmode.htm

The vkb comes up, but when typing something, no characters appear most of the time. Sometimes they do appear.
Which keyboard are you using? I tested using stock and Swiftkey on Galaxy Nexus. There was one bug but the characters all appear.
I've used the regular Android-keyboard, the language is dutch.
If you can see the characters appearing, btw, try hitting the backspace key a couple of times. Multiple characters disappear sometimes after 1 stroke of the backspace key.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Can you test again using this Nightly? http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/nchen@mozilla.com-bc8e46c78366/try-android/fennec-22.0a1.en-US.android-arm.apk

If it still has issues, can you post a copy of the logcat from start to end? Thanks!
Flags: needinfo?(martijn.martijn)
Attached file catlog — 
I still see various issues with this build.
In this catlog, I did the following.
I typed in 'aaaa', then hit the reload button, then typed 'a' again a couple of times, but those characters don't show up.
Flags: needinfo?(martijn.martijn)
Attached file Catlog2 — 
Wit this catlog, I did the following
- Typed 'a', then pressed enter
- Type 'a', then pressed enter
- Typed 'a', the pressed enter
- The pressed the backspace key 4 times or so

Issues that I'm seeing: 
- The 1st enter key doesn't seem to result in a line break
- The backspace key doesn't seem to be able to delete the first 2 characters that were typed
Sorry, I didn't mean to interrupt your vacation :)

When you get a chance, here's a new build http://people.mozilla.org/~nchen/builds/bug847983-1-fennec-22.0a1.apk.

With this build if I press enter, I see the cursor on the first line, but when you press a key, the character is shown on the second line.
Yeah, much better! This seems to fix the issues I mentioned in comment 5 and 6.
But indeed, the caret seems to stall on the previous line after pressing enter and then it disappears completely.
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22)(vacation till april 2nd) from comment #8)
> Yeah, much better! This seems to fix the issues I mentioned in comment 5 and
> 6.
> But indeed, the caret seems to stall on the previous line after pressing
> enter and then it disappears completely.

Hmm I see the same behavior on desktop, so maybe it's not a Fennec bug. I'm going to post the patches that I already have.
When there are script elements inside a design-mode document, we don't want to include the text nodes inside the script elements. These text nodes don't have frames, and that's how the patch detects them.

I think I will also write a test for it.
Attachment #730802 - Flags: review?(masayuki)
If offsets are out-of-bounds, this patch places the start/end of the range after the last "valid" element, instead of at the end of the root element.
Attachment #730804 - Flags: review?(masayuki)
Sometimes we cannot get a frame from the caret (maybe when the caret is hidden?). This patch also tries to get the frame from the root element.
Attachment #730806 - Flags: review?(masayuki)
Comment on attachment 730802 [details] [diff] [review]
Skip script text nodes for content events (v1)

Please don't omit {} at writing new code in content/. Otherwise, look good.
Attachment #730802 - Flags: review?(masayuki) → review+
Comment on attachment 730804 [details] [diff] [review]
Don't include empty elements at end of range (v1)

>@@ -382,30 +382,34 @@ nsContentEventHandler::SetRangeFromFlatTextOffset(
>                               bool aExpandToClusterBoundaries)
> {
>   nsCOMPtr<nsIContentIterator> iter = NS_NewPreContentIterator();
>   nsresult rv = iter->Init(mRootContent);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   uint32_t nativeOffset = 0;
>   uint32_t nativeEndOffset = aNativeOffset + aNativeLength;
>+  nsINode *lastValidNode = mRootContent;
>+  uint32_t lastValidOffset = 0;


Please add some comments before them why they are necessary.

>@@ -449,27 +453,24 @@ nsContentEventHandler::SetRangeFromFlatTextOffset(
>       rv = aRange->SetEnd(domNode, int32_t(xpOffset));
>       NS_ENSURE_SUCCESS(rv, rv);
>       return NS_OK;
>     }
> 
>     nativeOffset += nativeTextLength;
>   }
> 
>-  if (nativeOffset < aNativeOffset)
>-    return NS_ERROR_FAILURE;

Why do you need to remove this?
Comment on attachment 730806 [details] [diff] [review]
Try harder to get a valid frame (v1)

Your comment make me worry. I think that the approach isn't bad. However, could you investigate the cause more?

Or, roc, do you have some idea of such case?
Attachment #730806 - Flags: feedback?(roc)
Yes I think we need more data about why we're not finding a frame here. Layout has been flushed, and that suggests we actually have no frame for the caret. Is the current node perhaps a text node at the start or end of a block, for which we optimize away the frame?
Depends on: 856632
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> Comment on attachment 730806 [details] [diff] [review]
> Try harder to get a valid frame (v1)
> 
> Your comment make me worry. I think that the approach isn't bad. However,
> could you investigate the cause more?
> 
> Or, roc, do you have some idea of such case?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Yes I think we need more data about why we're not finding a frame here.
> Layout has been flushed, and that suggests we actually have no frame for the
> caret. Is the current node perhaps a text node at the start or end of a
> block, for which we optimize away the frame?

I found that if there is only a script element inside the design-mode document, the caret is initially placed inside the script element's text node, and because that text node doesn't have a frame, there is no frame for the caret either.

I filed bug 856632 for this.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> Comment on attachment 730804 [details] [diff] [review]
> Don't include empty elements at end of range (v1)
> 
> >@@ -449,27 +453,24 @@ nsContentEventHandler::SetRangeFromFlatTextOffset(
> >       rv = aRange->SetEnd(domNode, int32_t(xpOffset));
> >       NS_ENSURE_SUCCESS(rv, rv);
> >       return NS_OK;
> >     }
> > 
> >     nativeOffset += nativeTextLength;
> >   }
> > 
> >-  if (nativeOffset < aNativeOffset)
> >-    return NS_ERROR_FAILURE;
> 
> Why do you need to remove this?

We already clip the end offset to the root element, so I thought we should clip the start offset to the root element as well, instead of failing. But I can take this out if you don't think we want that.
Comment on attachment 730806 [details] [diff] [review]
Try harder to get a valid frame (v1)

Thank you for the investigation.

That sounds a bug of nsHTMLEditor::BeginingOfDocument(). Then, this must work fine for that.
Attachment #730806 - Flags: review?(masayuki)
Attachment #730806 - Flags: review+
Attachment #730806 - Flags: feedback?(roc)
Comment on attachment 730804 [details] [diff] [review]
Don't include empty elements at end of range (v1)

Thanks. Please don't remove it. Returning error is, that is a litmus paper　for the above logic. So, for clearing the purpose, could you change it as NS_ENSURE_TRUE()?
Attachment #730804 - Flags: review?(masayuki) → review+
Funny story, debug builds don't compile. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/88d40212e319

https://tbpl.mozilla.org/php/getParsedLog.php?id=21409674&tree=Mozilla-Inbound

../../../../content/events/src/nsContentEventHandler.cpp:473:26: error: no member named 'IsNodeOfType' in 'nsIDOMNode'
    MOZ_ASSERT(!domNode->IsNodeOfType(nsINode::eTEXT));
                ~~~~~~~  ^
../../../dist/include/mozilla/Assertions.h:287:23: note: expanded from macro 'MOZ_ASSERT'
                     (__VA_ARGS__))
                      ^
../../../dist/include/mozilla/Assertions.h:284:35: note: expanded from macro 'MOZ_ASSERT_GLUE'
#  define MOZ_ASSERT_GLUE(x, y) x y
                                  ^
../../../dist/include/mozilla/Assertions.h:251:14: note: expanded from macro 'MOZ_ASSERT_HELPER1'
       if (!(expr)) { \
             ^
1 error generated.
make[7]: *** [nsContentEventHandler.o] Error 1
Fix bustage
Attachment #730804 - Attachment is obsolete: true
Attachment #733397 - Flags: review+
When presenting text to the IME, we want to ignore text nodes that are not displayed (e.g. text nodes under script elements). I had hoped that checking for nsIContent::GetPrimaryFrame() would work, but I didn't realize sometimes we retrieve the text before layout flushes, and we end up not having frames for normal text nodes as well.

roc, can you think of a better way to check? Or should I just check for a <script> ancestor? Thanks!
Flags: needinfo?(roc)
You shouldn't check for a <script> ancestor. <style> and other elements, even just "display:none" elements, have similar issues.

A FlushPendingNotifications(Flush_Frames) followed by a check of GetPrimaryFrame() would work to see if there's a frame. Of course text nodes can be not displayed for other reasons, e.g. CSS visibility:hidden, or in a height:0 overflow:hidden container, etc.

As I said in comment #16 it looks like we actually do flush before reaching this code, so GetPrimaryFrame() should work and I don't know why you're not finding a frame. It might be because of our optimization where collapsible whitespace-only text nodes at the start or end of a block don't get a text node. E.g. given <div>   <a>foo</a></div> the leading whitespace doesn't get a text node (it's not visible at all either, since it collapses away).
Flags: needinfo?(roc)
Calling nsFrame::DumpFrameTree(someFrame) in a debug build when the code is triggered might help diagnose the problem.
One reason the tests were failing is because I forgot to skip script nodes in GetFlatTextOffsetOfRange.
Attachment #747605 - Flags: review?(masayuki)
This is fixes another reason the tests were failing. We check script text nodes by checking the primary frame; however, during text change events, we may not have a frame yet. So this patch saves the original nodes, and when it's safe, it flushes layout and updates the offsets for the text change event.
Attachment #747609 - Flags: review?(masayuki)
Attachment #747605 - Flags: review?(masayuki) → review+
Comment on attachment 747609 [details] [diff] [review]
Flush layout and update offsets when text change events are run (v1)

>   TextChangeEvent(nsTextStateManager* aDispatcher,
>-                  uint32_t start, uint32_t oldEnd, uint32_t newEnd)
>+                  nsINode *aStartNode,
>+                  int32_t aStartOffset,
>+                  nsINode *aNewEndNode,
>+                  int32_t aNewEndOffset,
>+                  uint32_t aDeletedLength)
>     : mDispatcher(aDispatcher)
>-    , mStart(start)
>-    , mOldEnd(oldEnd)
>-    , mNewEnd(newEnd)
>+    , mStartNode(aStartNode)
>+    , mNewEndNode(aNewEndNode)
>+    , mStartOffset(aStartOffset)
>+    , mNewEndOffset(aNewEndOffset)
>+    , mDeletedLength(aDeletedLength)
>+    , mStart(0)
>+    , mNewEnd(0)
>   {
>     MOZ_ASSERT(mDispatcher);
>+    MOZ_ASSERT(mStartNode);
>+    MOZ_ASSERT(mNewEndNode);
>+    UpdateOffsets();
>   }
> 
>   NS_IMETHOD Run() {
>     if (mDispatcher->mWidget) {
>-      mDispatcher->mWidget->NotifyIMEOfTextChange(mStart, mOldEnd, mNewEnd);
>+      nsPresContext *presContext = nsIMEStateManager::GetPresContext();

I'm not sure if this is safe. I think that the constructor should have aPresContext and the instance should hold it strongly.

>+      if (presContext) {

If there is no presContext, is it worthwhile to continue the processing?

>+        nsIPresShell *presShell = presContext->PresShell();
>+        if (presShell) {

You shouldn't need to check if the result of PresShell() is null. Get*() may return nullptr, but without "Get", not so. If you meet crash by returning nullptr, then, you should use NS_ENSURE_TRUE() instead. I think that we don't need to continue the processing.

>+          presShell->FlushPendingNotifications(Flush_Layout);
>+          UpdateOffsets();
>+        }
>+      }
>+      uint32_t oldEnd = mStart + mDeletedLength;
>+      mDispatcher->mWidget->NotifyIMEOfTextChange(mStart, oldEnd, mNewEnd);
>     }
>     return NS_OK;
>   }
> 
> private:
>+  void UpdateOffsets() {

Put "{" to the next line.

>+    uint32_t start, newEnd;
>+    NS_ENSURE_TRUE_VOID(mDispatcher->mRootContent);
>+    NS_ENSURE_SUCCESS_VOID(nsContentEventHandler::GetFlatTextOffsetOfRange(
>+        mDispatcher->mRootContent, mStartNode, mStartOffset, &start));
>+    NS_ENSURE_SUCCESS_VOID(nsContentEventHandler::GetFlatTextOffsetOfRange(
>+        mDispatcher->mRootContent, mNewEndNode, mNewEndOffset, &newEnd));

If one of them fails, what happens to notify IME of text change with wrong offsets? I think that if one of them fails, we don't need to notify IME of the text change.

> void
> nsTextStateManager::ContentRemoved(nsIDocument* aDocument,
>                                    nsIContent* aContainer,
>                                    nsIContent* aChild,
>                                    int32_t aIndexInContainer,
>                                    nsIContent* aPreviousSibling)
> {
>-  uint32_t offset = 0, childOffset = 1;
>-  if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>-                    mRootContent, NODE_FROM(aContainer, aDocument),
>-                    aIndexInContainer, &offset)))
>-    return;
>-
>+  uint32_t childOffset = 1;
>   // get offset at the end of the deleted node
>   if (aChild->IsNodeOfType(nsINode::eTEXT))
>     childOffset = aChild->TextLength();
>   else if (0 < aChild->GetChildCount())
>     childOffset = aChild->GetChildCount();
> 
>   if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>                     aChild, aChild, childOffset, &childOffset)))
>     return;

Is it okay calling this before layout flush??? Note that two or more mutation notifications may be occurred continuously. For example, remove a lot of nodes selected and press del key in an HTML editor.

> 
>   // fire notification
>-  if (childOffset)
>-    nsContentUtils::AddScriptRunner(
>-        new TextChangeEvent(this, offset, offset + childOffset, offset));
>+  if (childOffset) {

if (!childOffset) {
  return;
}

then, you can reduce following indentation.

>+    nsINode *node = NODE_FROM(aContainer, aDocument);
>+    nsContentUtils::AddScriptRunner(new TextChangeEvent(this,
>+        node, aIndexInContainer,
>+        node, aIndexInContainer,
>+        childOffset));
>+  }
> }

>@@ -1014,22 +1030,24 @@ nsTextStateManager::AttributeChanged(nsIDocument* aDocument,
> {
>   nsIContent *content = GetContentBR(aElement);
>   if (!content) {
>     return;
>   }
>   uint32_t postAttrChangeLength =
>     nsContentEventHandler::GetNativeTextLength(content);
>   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));
>-    }
>+    nsIContent *parent = content->GetParent();
>+    NS_ENSURE_TRUE_VOID(parent);
>+    int32_t index = parent->IndexOf(content);
>+    NS_ENSURE_TRUE_VOID(index >= 0);

Is this possible? If not, use MOZ_ASSERT().

>+    nsContentUtils::AddScriptRunner(new TextChangeEvent(this,
>+        parent, index,
>+        parent, index + 1,
>+        mPreAttrChangeLength));

please change to "early return style". I.e.,

if (postAttrChangeLength == mPreAttrChangeLength) {
  return;
}


Please note that I don't understand this patch perfectly. So, if I reviewed with misunderstanding, let me know.
>   TextChangeEvent(nsTextStateManager* aDispatcher,
>-                  uint32_t start, uint32_t oldEnd, uint32_t newEnd)
>+                  nsINode *aStartNode,
>+                  int32_t aStartOffset,
>+                  nsINode *aNewEndNode,
>+                  int32_t aNewEndOffset,
>+                  uint32_t aDeletedLength)

nit: |*| should be immediately after the type of arguments.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> Comment on attachment 747609 [details] [diff] [review]
> Flush layout and update offsets when text change events are run (v1)
> 
> >   NS_IMETHOD Run() {
> >     if (mDispatcher->mWidget) {
> >-      mDispatcher->mWidget->NotifyIMEOfTextChange(mStart, mOldEnd, mNewEnd);
> >+      nsPresContext *presContext = nsIMEStateManager::GetPresContext();
> 
> I'm not sure if this is safe. I think that the constructor should have
> aPresContext and the instance should hold it strongly.
> 
> >+      if (presContext) {
> 
> If there is no presContext, is it worthwhile to continue the processing?
> 
> >+        nsIPresShell *presShell = presContext->PresShell();
> >+        if (presShell) {
> 
> You shouldn't need to check if the result of PresShell() is null. Get*() may
> return nullptr, but without "Get", not so. If you meet crash by returning
> nullptr, then, you should use NS_ENSURE_TRUE() instead. I think that we
> don't need to continue the processing.

Okay. I changed to keeping a strong reference to the nsIDocument, which is available through the mutation observer. And from the nsIDocument we can get the PresShell. Is that okay?

> > 
> > private:
> >+  void UpdateOffsets() {
> 
> Put "{" to the next line.

Done

> 
> >+    uint32_t start, newEnd;
> >+    NS_ENSURE_TRUE_VOID(mDispatcher->mRootContent);
> >+    NS_ENSURE_SUCCESS_VOID(nsContentEventHandler::GetFlatTextOffsetOfRange(
> >+        mDispatcher->mRootContent, mStartNode, mStartOffset, &start));
> >+    NS_ENSURE_SUCCESS_VOID(nsContentEventHandler::GetFlatTextOffsetOfRange(
> >+        mDispatcher->mRootContent, mNewEndNode, mNewEndOffset, &newEnd));
> 
> If one of them fails, what happens to notify IME of text change with wrong
> offsets? I think that if one of them fails, we don't need to notify IME of
> the text change.

I think we should still notify text change even if one of the above fails. If a node is added and then immediately removed, during text change for adding, one of the above will fail because the node is not in the document. However, we still need to notify about the 'adding' operation, because there is a 'removing' operation in the queue.

> > 
> >   if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
> >                     aChild, aChild, childOffset, &childOffset)))
> >     return;
> 
> Is it okay calling this before layout flush??? Note that two or more
> mutation notifications may be occurred continuously. For example, remove a
> lot of nodes selected and press del key in an HTML editor.

I think it's okay; the node likely already has a frame. Also even if the node doesn't have a frame, because it's already removed from the document, I don't think layout flush will change it.

> > 
> >   // fire notification
> >-  if (childOffset)
> >-    nsContentUtils::AddScriptRunner(
> >-        new TextChangeEvent(this, offset, offset + childOffset, offset));
> >+  if (childOffset) {
> 
> if (!childOffset) {
>   return;
> }
> 
> then, you can reduce following indentation.
> 

Done

> >-    }
> >+    nsIContent *parent = content->GetParent();
> >+    NS_ENSURE_TRUE_VOID(parent);
> >+    int32_t index = parent->IndexOf(content);
> >+    NS_ENSURE_TRUE_VOID(index >= 0);
> 
> Is this possible? If not, use MOZ_ASSERT().

Done

> 
> >+    nsContentUtils::AddScriptRunner(new TextChangeEvent(this,
> >+        parent, index,
> >+        parent, index + 1,
> >+        mPreAttrChangeLength));
> 
> please change to "early return style". I.e.,

Done

Let me know if you have more concerns. Thanks!
Attachment #747609 - Attachment is obsolete: true
Attachment #747609 - Flags: review?(masayuki)
Attachment #748281 - Flags: review?(masayuki)
(In reply to Jim Chen [:jchen :nchen] from comment #33)
> Created attachment 748281 [details] [diff] [review]
> Flush layout and update offsets when text change events are run (v2)
> 
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> > Comment on attachment 747609 [details] [diff] [review]
> > Flush layout and update offsets when text change events are run (v1)
> > 
> > >   NS_IMETHOD Run() {
> > >     if (mDispatcher->mWidget) {
> > >-      mDispatcher->mWidget->NotifyIMEOfTextChange(mStart, mOldEnd, mNewEnd);
> > >+      nsPresContext *presContext = nsIMEStateManager::GetPresContext();
> > 
> > I'm not sure if this is safe. I think that the constructor should have
> > aPresContext and the instance should hold it strongly.
> > 
> > >+      if (presContext) {
> > 
> > If there is no presContext, is it worthwhile to continue the processing?
> > 
> > >+        nsIPresShell *presShell = presContext->PresShell();
> > >+        if (presShell) {
> > 
> > You shouldn't need to check if the result of PresShell() is null. Get*() may
> > return nullptr, but without "Get", not so. If you meet crash by returning
> > nullptr, then, you should use NS_ENSURE_TRUE() instead. I think that we
> > don't need to continue the processing.
> 
> Okay. I changed to keeping a strong reference to the nsIDocument, which is
> available through the mutation observer. And from the nsIDocument we can get
> the PresShell. Is that okay?

Hmm, then, cannot you get the nsIDocument from the start node or end node?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #34)
> (In reply to Jim Chen [:jchen :nchen] from comment #33)
> > Created attachment 748281 [details] [diff] [review]
> > Flush layout and update offsets when text change events are run (v2)
> > 
> > > >+        nsIPresShell *presShell = presContext->PresShell();
> > > >+        if (presShell) {
> > > 
> > > You shouldn't need to check if the result of PresShell() is null. Get*() may
> > > return nullptr, but without "Get", not so. If you meet crash by returning
> > > nullptr, then, you should use NS_ENSURE_TRUE() instead. I think that we
> > > don't need to continue the processing.
> > 
> > Okay. I changed to keeping a strong reference to the nsIDocument, which is
> > available through the mutation observer. And from the nsIDocument we can get
> > the PresShell. Is that okay?
> 
> Hmm, then, cannot you get the nsIDocument from the start node or end node?

You are right. I changed the patch to get the nsIDocument from the start node in the TextChangeEvent constructor (we have to get the doc in the constructor because the node may not be in a document later).
Attachment #748281 - Attachment is obsolete: true
Attachment #748281 - Flags: review?(masayuki)
Attachment #748618 - Flags: review?(masayuki)
Comment on attachment 748618 [details] [diff] [review]
Flush layout and update offsets when text change events are run (v3)

>-  NS_IMETHOD Run() {
>+  NS_IMETHOD Run()
>+  {
>     if (mDispatcher->mWidget) {
>-      mDispatcher->mWidget->NotifyIMEOfTextChange(mStart, mOldEnd, mNewEnd);
>+      nsIPresShell *presShell = mDocument->GetShell();
>+      NS_ENSURE_STATE(presShell);
>+      presShell->FlushPendingNotifications(Flush_Layout);
>+      UpdateOffsets();
>+
>+      uint32_t oldEnd = mStart + mDeletedLength;
>+      mDispatcher->mWidget->NotifyIMEOfTextChange(mStart, oldEnd, mNewEnd);
>     }
>     return NS_OK;
>   }

nit:

if (!mDispatcher->mWidget) {
  return NS_OK;
}

nsIPresShell* presShell = mDocument->GetShell();

# Note that "*" should be immediately after the type.

>+  void UpdateOffsets()
>+  {
>+    uint32_t start, newEnd;
>+    NS_ENSURE_TRUE_VOID(mDispatcher->mRootContent);
>+    NS_ENSURE_SUCCESS_VOID(nsContentEventHandler::GetFlatTextOffsetOfRange(
>+        mDispatcher->mRootContent, mStartNode, mStartOffset, &start));
>+    NS_ENSURE_SUCCESS_VOID(nsContentEventHandler::GetFlatTextOffsetOfRange(
>+        mDispatcher->mRootContent, mNewEndNode, mNewEndOffset, &newEnd));
>+    mStart = start;
>+    mNewEnd = newEnd;
>+  }

> > If one of them fails, what happens to notify IME of text change with wrong
> > offsets? I think that if one of them fails, we don't need to notify IME of
> > the text change.
> 
> I think we should still notify text change even if one of the above fails.
> If a node is added and then immediately removed, during text change for
> adding, one of the above will fail because the node is not in the document.
> However, we still need to notify about the 'adding' operation, because there
> is a 'removing' operation in the queue.
> 

But the offset isn't initialized at sending the notification... How do you think about this?

Should it be notified as all text changed? Or if it's always followed by another notification, cannot we omit the notification?

> nsTextStateManager::CharacterDataChanged(nsIDocument* aDocument,
>                                          nsIContent* aContent,
>                                          CharacterDataChangeInfo* aInfo)
> {
>   NS_ASSERTION(aContent->IsNodeOfType(nsINode::eTEXT),
>                "character data changed for non-text node");
> 
>-  uint32_t offset = 0;
>-  // get offsets of change and fire notification
>-  if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>-                    mRootContent, aContent, aInfo->mChangeStart, &offset)))
>-    return;
>-
>-  uint32_t oldEnd = offset + aInfo->mChangeEnd - aInfo->mChangeStart;
>-  uint32_t newEnd = offset + aInfo->mReplaceLength;
>-
>-  nsContentUtils::AddScriptRunner(
>-      new TextChangeEvent(this, offset, oldEnd, newEnd));
>+  nsContentUtils::AddScriptRunner(new TextChangeEvent(this,
>+      aContent, aInfo->mChangeStart,
>+      aContent, aInfo->mChangeStart + aInfo->mReplaceLength,
>+      aInfo->mChangeEnd - aInfo->mChangeStart));
> }

nit: The indentation should be 2 white-spaces. Indeed, it's been used 4 white-spaces here. Please correct them.

> void
> nsTextStateManager::NotifyContentAdded(nsINode* aContainer,
>                                        int32_t aStartIndex,
>                                        int32_t aEndIndex)
> {
>-  uint32_t offset = 0, newOffset = 0;
>-  if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>-                    mRootContent, aContainer, aStartIndex, &offset)))
>-    return;
>-
>-  // get offset at the end of the last added node
>-  if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>-                    aContainer->GetChildAt(aStartIndex),
>-                    aContainer, aEndIndex, &newOffset)))
>-    return;
>-
>-  // fire notification
>-  if (newOffset)
>-    nsContentUtils::AddScriptRunner(
>-        new TextChangeEvent(this, offset, offset, offset + newOffset));
>+  nsContentUtils::AddScriptRunner(new TextChangeEvent(this,
>+      aContainer, aStartIndex, aContainer, aEndIndex, 0));
> }

Same.

> void
> nsTextStateManager::ContentRemoved(nsIDocument* aDocument,
>                                    nsIContent* aContainer,
>                                    nsIContent* aChild,
>                                    int32_t aIndexInContainer,
>                                    nsIContent* aPreviousSibling)
> {
>-  uint32_t offset = 0, childOffset = 1;
>-  if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>-                    mRootContent, NODE_FROM(aContainer, aDocument),
>-                    aIndexInContainer, &offset)))
>-    return;
>-
>+  uint32_t childOffset = 1;
>   // get offset at the end of the deleted node
>   if (aChild->IsNodeOfType(nsINode::eTEXT))
>     childOffset = aChild->TextLength();
>   else if (0 < aChild->GetChildCount())
>     childOffset = aChild->GetChildCount();
> 
>   if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>                     aChild, aChild, childOffset, &childOffset)))
>     return;
> 
>   // fire notification
>-  if (childOffset)
>-    nsContentUtils::AddScriptRunner(
>-        new TextChangeEvent(this, offset, offset + childOffset, offset));
>+  if (!childOffset) {
>+    return;
>+  }
>+  nsINode *node = NODE_FROM(aContainer, aDocument);
>+  nsContentUtils::AddScriptRunner(new TextChangeEvent(this,
>+      node, aIndexInContainer,
>+      node, aIndexInContainer,
>+      childOffset));
> }

Same.

>@@ -1013,24 +1034,27 @@ nsTextStateManager::AttributeChanged(nsIDocument* aDocument,
>                                      int32_t aModType)
> {
>   nsIContent *content = GetContentBR(aElement);
>   if (!content) {
>     return;
>   }
>   uint32_t postAttrChangeLength =
>     nsContentEventHandler::GetNativeTextLength(content);
>-  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));
>-    }
>+  if (postAttrChangeLength == mPreAttrChangeLength) {
>+    return;
>   }
>+  nsIContent *parent = content->GetParent();
>+  NS_ENSURE_TRUE_VOID(parent);
>+  int32_t index = parent->IndexOf(content);
>+  MOZ_ASSERT(index >= 0);
>+  nsContentUtils::AddScriptRunner(new TextChangeEvent(this,
>+      parent, index,
>+      parent, index + 1,
>+      mPreAttrChangeLength));
> }

Same.
Comment on attachment 748618 [details] [diff] [review]
Flush layout and update offsets when text change events are run (v3)

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

Hmm I now see a bug with this patch -- if there are two insertions and the second one is before the first one. The first insertion will have the wrong offsets when we flush layout and update the offsets. I will take a look at this more when I'm back from PTO. Sorry for the spam.
Attachment #748618 - Flags: review?(masayuki)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.