847983 - IME in designmode doesn't seem to work very well

Status: RESOLVED INACTIVE

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

Description

[Martijn Wargers (dead)]

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.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Which keyboard are you using? I tested using stock and Swiftkey on Galaxy Nexus. There was one bug but the characters all appear.

Comment 2

[Martijn Wargers (dead)]

I've used the regular Android-keyboard, the language is dutch.

Comment 3

[Martijn Wargers (dead)]

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.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Can you test again using this Nightly? http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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!

Comment 5

[Martijn Wargers (dead)]

Attachment: 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.

Comment 6

[Martijn Wargers (dead)]

Attachment: 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

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 8

[Martijn Wargers (dead)]

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.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

(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.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Skip script text nodes for content events (v1)

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.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Don't include empty elements at end of range (v1)

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.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Try harder to get a valid frame (v1)

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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

(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.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

(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 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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()?

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Addressed comments.

try: https://tbpl.mozilla.org/?tree=Try&rev=90973cdf25e6

https://hg.mozilla.org/integration/mozilla-inbound/rev/fff4886f812c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e95379bbcfd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/35ba787bc3db

Leaving open for test.

Comment 22

[(PTO June 19-July 7) Ryan VanderMeulen]

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

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Don't include empty elements at end of range (v1.1)

Fix bustage

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

New try and push:

https://tbpl.mozilla.org/?tree=Try&rev=91514db915f0

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d817fa463cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2606c5349493
https://hg.mozilla.org/integration/mozilla-inbound/rev/9967368c7c72

Comment 25

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/55f9e3e3dae7 for intermittent but very frequent crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=21444707&tree=Mozilla-Inbound in mochitest-8.

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

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!

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Calling nsFrame::DumpFrameTree(someFrame) in a debug build when the code is triggered might help diagnose the problem.

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Skip script text node for range-to-index conversion (v1)

One reason the tests were failing is because I forgot to skip script nodes in GetFlatTextOffsetOfRange.

Comment 30

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Flush layout and update offsets when text change events are run (v1)

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.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

>   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.

Comment 33

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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?

> > 
> > 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!

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

(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?

Comment 35

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Flush layout and update offsets when text change events are run (v3)

(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).

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

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 37

[(inactive) Jim Chen [:jchen] [:darchons]]

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.