348341 - "Reverse conversion" doesn't work by Kotoeri(Japanese input).

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Hiro]

In Firefox and Thunderbird, there are no problems. 
As for Camino alone, this function doesn't work. 

Firefox and Thunderbird will be influenced in the future if it is a problem of Cocoa widget. 

Reproducible: Always

Steps to Reproduce:
1. Open URL
2. Click search(input) field
3. IME is turned on. 
4. Input japanese
     e.g. kotoeri (It is displayed by the hiragana as "ことえり".) 
5. The input is fixed pressing return. 
6. Input "ことえり" is selected. 
7. "Reverse conversion" is selected from the menu of IME. 
     ( or push Contol+Shift+r)

It is explanation Movie of this function. 
http://www.apple.com/jp/articles/panther/20031107/function04.html

Mac OS X 10.3.9
2006081107 (v1.2+)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Is this also an issue in Cocoafox builds?  http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/experimental/cocoa/latest-trunk/

Comment 2

[Hiro]

Cocoa-fox was tried. 
Yes, it has this problem as well as Camino. 

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060902 Minefield/3.0a1

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Is this different from bug 237886?  Or is that one about adding support for additional conversion functions?

Comment 4

[Hiro]

(In reply to comment #3)
> Is this different from bug 237886?  Or is that one about adding support for
> additional conversion functions?
> 
It differs from bug237886. 
Bug237886 is a demand said that it wants to use the new feature supported from Kotoeri4(Mac OS X 10.3). 

bug348341 is a bug that "Reverse conversion" doesn't function by Cocoa Widget. 
And in Firefox that moved to Cocoa widget, this is regression. 

Comment 5

[Hiro]

(In reply to comment #4)
> bug348341 is a bug that "Reverse conversion" doesn't function by Cocoa Widget. 
> And in Firefox that moved to Cocoa widget, this is regression. 

In addition, the thing that "Reverse conversion" doesn't work is fatal for Japanese input. 

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Thanks for the clarification.  This should probably be blocking1.9? then.

Comment 7

[Josh Aas]

Masayuki - do you think you could take a look at this? How much work do you think this will take to fix?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I still don't understand what is a cause of this bug...

# I'm learning the TSM/IME specs at writing the patch for Mac...

I cannot promise I will fix this bug now, but I think that this should be fixed before 1.9 final. And I should fix this.

Comment 9

[Mike Schroepfer]

Masayuki have you had any time to look into this?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

No, I'll start to work on mac IME bugs in beta stage.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

hmm... I'm working on this, and I have a trouble...

I think that this bug needs very large change. Because as reconverting, IME queries the several information to us. Widget code cannot reply it, we need XP level code (e.g., content?) for it. And the current reconversion event depends on windows behavior, I think that we need to remove it, and we need to create some query event and remove selected text event, and they should be used on Windows too.

1. we create some query events on this bug.
2. we need to recreate the reconversion of windows with new query events on another bug.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.1 (work in progress)

this patch works fine for me, but the patch is pretty ugly, I'll clean up it and test it.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.2 (work in progress)

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that there are more needed works. Because the some methods have selected range in their param. But we don't support it. The current patch assumes that it is always in the selected range.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.3 (work in progress)

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv0.5 (work in progress)

I changed the implementation. This way will fix this bug strictly.

Note that this patch cannot be built on Mac.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

First, I need roc's review to the structure of the patch.

For the reconversion, we need to implement |firstRectForCharacterRange| and |selectedRange| and |attributedSubstringFromRange|. They refers the selection range in the text contents. This patch implements them strictly. Note that the offset of selection is not needed now, but it will be needed in bug 237886 (we'll need more works for it...). And this patch can remove the old QUERYCARET event and RECONVERTION event. But it will be done after this bug.

This patch creates new 3 events.
1. NS_QUERY_SELECTED_TEXT
  Query for the selected text information, it returns the selection offset, selection length and selection text.
2. NS_QUERY_TEXT_CONTENT
  Query for the text content of specified range, it returns actual length and the text of the specified range.
3. NS_QUERY_CHARACTER_RECT
  Query for the character rect of the specified offset, it returns the character rect.(or caret rect if the text content is empty.) However, the width is not needed Mac and Win, therefore, it now returns 1px.

These events assume that the content is text only content. I.e., the non-text contents are ignored. The offset conversion between the flat text and DOM tree is in nsRange (via nsIRange).

Comment 18

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

+  // The result must be same length as the result of nsIDOMRange::ToString.
+  NS_IMETHOD TextLength(PRUint32& aLength) = 0;
+
+  NS_IMETHOD SetRangeFromTextOffset(nsIContent* aRootContent,
+                                    PRUint32 aOffset, PRUint32 aLength) = 0;

Why do you need these? These can be done using existing Range methods.

+          nsRect *r = nsnull;
+          if (aEvent->message == NS_TEXT_TEXT) {
+            r = &((nsTextEvent*)aEvent)->theReply.mCursorPosition;
+          } else if ((aEvent->message == NS_COMPOSITION_START) ||
+                     (aEvent->message == NS_COMPOSITION_QUERY)) {
+            r = &((nsCompositionEvent*)aEvent)->theReply.mCursorPosition;
+          } else if (aEvent->message == NS_QUERYCARETRECT) {
+            r = &((nsQueryCaretRectEvent*)aEvent)->theReply.mCaretRect;
+          } else if (aEvent->message == NS_QUERY_CHARACTER_RECT) {
+            r = ((nsQueryContentEvent*)aEvent)->mReply.mRect;
           }
+          if (r)
+            ConvertRectAppUnitsToIntPixels(*r, p2a);

Just put in four calls to ConvertRectAppUnitsToIntPixels.

+  struct input {
+    PRBool mAllRange;
+    PRUint32 mOffset;
+    PRUint32 mLength;
+  }mInput;
+  struct reply {

If you need these struct names, make them start with Uppercase, otherwise just leave them out.

+// Selection events
+#define NS_QUERY_CONTENT_EVENT_START    3200
+#define NS_QUERY_SELECTED_TEXT          (NS_QUERY_CONTENT_EVENT_START)
+#define NS_QUERY_TEXT_CONTENT           (NS_QUERY_CONTENT_EVENT_START + 1)
+#define NS_QUERY_CHARACTER_RECT         (NS_QUERY_CONTENT_EVENT_START + 2)

Please document what these mean. Your comments in comment #17 would be a good start, although you don't say what NS_QUERY_CONTENT_EVENT_START is.

+  if (frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION) {
+    for (nsINode* node = startNode; node;
+         node = node->GetNodeParent()) {
+      nsCOMPtr<nsIDOMNSEditableElement>
+        editableElement(do_QueryInterface(node));
+      if (!editableElement)
+        continue;
+      nsCOMPtr<nsIEditor> editor;
+      rv = editableElement->GetEditor(getter_AddRefs(editor));
+      NS_ENSURE_SUCCESS(rv, rv);
+      NS_ENSURE_TRUE(editor, NS_ERROR_FAILURE);
+      nsCOMPtr<nsIDOMElement> rootElement;
+      rv = editor->GetRootElement(getter_AddRefs(rootElement));
+      NS_ENSURE_SUCCESS(rv, rv);
+      mRootContent = do_QueryInterface(rootElement);
+      NS_ASSERTION(mRootContent, "nsIDOMElement doesn't have nsIContent!");
+      return NS_OK;

Surely there's code in editor that does the same thing? We should reuse it. Chris Pearce might know, CCing.

+  nsCOMPtr<nsISelection> mSelection;
+  nsCOMPtr<nsIRange> mFirstSelectedRange;
+  nsCOMPtr<nsIContent> mRootContent;

I wish these could be regular pointers, but I guess it's easier to make them nsCOMPtrs so we can work with the editor interfaces. editor really needs major deCOMtamination...

Except that mFirstSelectedRange could just be an nsRefPtr<nsRange>, I think. Then we can use nsRange methods that are deCOMtaminated, and we can do "new nsRange" instead of NS_NewRange().

I think OnQuerySelectedText should just build a string containing the text and then either return its length or return the text. Then we don't need TextLength and I think OnQuerySelectedText will be less code too. The performance here doesn't matter.

In OnQueryCharacterRect, it looks like if the queried range is a text node, then we return the requested offset in the requested text node, otherwise we ignore what was requested and return the current caret position? That doesn't make much sense to me.

+    // XXX currently, we don't resolve actual width of the character,
+    // if you need it, you need to hack here and nsIFrame.
+    aEvent->mReply.mRect->width = 1;

No you don't, you can do GetPointFromOffset for the next character and the width is the difference in the x coordinates. But watch out for RTL and direction changes, I think you're returning the wrong rect for RTL.

+                 "Faild to get the current insertion point!");

Failed

Don't you need some Windows changes here too?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Roc: Thank you for the long review.

(In reply to comment #18)
> +  // The result must be same length as the result of nsIDOMRange::ToString.
> +  NS_IMETHOD TextLength(PRUint32& aLength) = 0;
> +
> +  NS_IMETHOD SetRangeFromTextOffset(nsIContent* aRootContent,
> +                                    PRUint32 aOffset, PRUint32 aLength) = 0;
> 
> Why do you need these? These can be done using existing Range methods.

What can I use? I cannot find it in nsRange...

> +  if (frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION) {
> +    for (nsINode* node = startNode; node;
> +         node = node->GetNodeParent()) {
> +      nsCOMPtr<nsIDOMNSEditableElement>
> +        editableElement(do_QueryInterface(node));
> +      if (!editableElement)
> +        continue;
> +      nsCOMPtr<nsIEditor> editor;
> +      rv = editableElement->GetEditor(getter_AddRefs(editor));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      NS_ENSURE_TRUE(editor, NS_ERROR_FAILURE);
> +      nsCOMPtr<nsIDOMElement> rootElement;
> +      rv = editor->GetRootElement(getter_AddRefs(rootElement));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      mRootContent = do_QueryInterface(rootElement);
> +      NS_ASSERTION(mRootContent, "nsIDOMElement doesn't have nsIContent!");
> +      return NS_OK;
> 
> Surely there's code in editor that does the same thing? We should reuse it.
> Chris Pearce might know, CCing.

I found same logic in FAYT of toolkit. But I'm not sure which module should have this code for this and FAYT.

> I think OnQuerySelectedText should just build a string containing the text and
> then either return its length or return the text. Then we don't need TextLength
> and I think OnQuerySelectedText will be less code too. The performance here
> doesn't matter.

If the editor is HTML editor, can it make performance issue? And I have a worry, doesn't it become the cause of memory fragmentation?

> In OnQueryCharacterRect, it looks like if the queried range is a text node,
> then we return the requested offset in the requested text node, otherwise we
> ignore what was requested and return the current caret position? That doesn't
> make much sense to me.

I don't have better idea now...

> Don't you need some Windows changes here too?

If you want it to merge here, I'll add the code and removing the old codes. However, I hope that it is separated to new bug.

Comment 20

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

(In reply to comment #19)
> (In reply to comment #18)
> > +  // The result must be same length as the result of nsIDOMRange::ToString.
> > +  NS_IMETHOD TextLength(PRUint32& aLength) = 0;
> > +
> > +  NS_IMETHOD SetRangeFromTextOffset(nsIContent* aRootContent,
> > +                                    PRUint32 aOffset, PRUint32 aLength) = 0;
> > 
> > Why do you need these? These can be done using existing Range methods.
> 
> What can I use? I cannot find it in nsRange...

Call ToString() and then measure the length instead of TextLength.

Call nsIDOMRange::SetStart/SetEnd instead of SetRangeFromTextOffset.

> > +  if (frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION) {
> > +    for (nsINode* node = startNode; node;
> > +         node = node->GetNodeParent()) {
> > +      nsCOMPtr<nsIDOMNSEditableElement>
> > +        editableElement(do_QueryInterface(node));
> > +      if (!editableElement)
> > +        continue;
> > +      nsCOMPtr<nsIEditor> editor;
> > +      rv = editableElement->GetEditor(getter_AddRefs(editor));
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +      NS_ENSURE_TRUE(editor, NS_ERROR_FAILURE);
> > +      nsCOMPtr<nsIDOMElement> rootElement;
> > +      rv = editor->GetRootElement(getter_AddRefs(rootElement));
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +      mRootContent = do_QueryInterface(rootElement);
> > +      NS_ASSERTION(mRootContent, "nsIDOMElement doesn't have nsIContent!");
> > +      return NS_OK;
> > 
> > Surely there's code in editor that does the same thing? We should reuse it.
> > Chris Pearce might know, CCing.
> 
> I found same logic in FAYT of toolkit. But I'm not sure which module should
> have this code for this and FAYT.

nsINode::FindRootEditableElement maybe? But editor *must* have code like this.

> > I think OnQuerySelectedText should just build a string containing the text and
> > then either return its length or return the text. Then we don't need TextLength
> > and I think OnQuerySelectedText will be less code too. The performance here
> > doesn't matter.
> 
> If the editor is HTML editor, can it make performance issue? And I have a
> worry, doesn't it become the cause of memory fragmentation?

What memory fragmentation? You're just building a string and freeing it. This can only happen once per input event, right? So we can afford to do something fairly expensive. We should certainly start with the simple code and optimize it only if we can show it's a problem.

> > In OnQueryCharacterRect, it looks like if the queried range is a text node,
> > then we return the requested offset in the requested text node, otherwise we
> > ignore what was requested and return the current caret position? That doesn't
> > make much sense to me.
> 
> I don't have better idea now...

You mean, you don't know how to implement the non-text-node case?

> > Don't you need some Windows changes here too?
> 
> If you want it to merge here, I'll add the code and removing the old codes.
> However, I hope that it is separated to new bug.

Will this patch break Windows?

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > +  // The result must be same length as the result of nsIDOMRange::ToString.
> > > +  NS_IMETHOD TextLength(PRUint32& aLength) = 0;
> > > +
> > > +  NS_IMETHOD SetRangeFromTextOffset(nsIContent* aRootContent,
> > > +                                    PRUint32 aOffset, PRUint32 aLength) = 0;
> > > 
> > > Why do you need these? These can be done using existing Range methods.
> > 
> > What can I use? I cannot find it in nsRange...
> 
> Call ToString() and then measure the length instead of TextLength.
> 
> Call nsIDOMRange::SetStart/SetEnd instead of SetRangeFromTextOffset.

No, I cannot use SetStart/SetEnd. Because the text nodes is not often a same node. We should always compute the offset from the start of first text node for TSMDocumentAccess which is needed in near future.

> > > I think OnQuerySelectedText should just build a string containing the text and
> > > then either return its length or return the text. Then we don't need TextLength
> > > and I think OnQuerySelectedText will be less code too. The performance here
> > > doesn't matter.
> > 
> > If the editor is HTML editor, can it make performance issue? And I have a
> > worry, doesn't it become the cause of memory fragmentation?
> 
> What memory fragmentation? You're just building a string and freeing it. This
> can only happen once per input event, right? So we can afford to do something
> fairly expensive. We should certainly start with the simple code and optimize
> it only if we can show it's a problem.

If the text is over nsAutoString buffer length, isn't the string buffer made in the heap? And the three events are sometimes called 2 or more times at during one input event, it depends on the IME implementation.

> > > In OnQueryCharacterRect, it looks like if the queried range is a text node,
> > > then we return the requested offset in the requested text node, otherwise we
> > > ignore what was requested and return the current caret position? That doesn't
> > > make much sense to me.
> > 
> > I don't have better idea now...
> 
> You mean, you don't know how to implement the non-text-node case?

And if the editor doesn't have any contents, I don't know that I should refer which style for the font size. The root node style is different...

> > > Don't you need some Windows changes here too?
> > 
> > If you want it to merge here, I'll add the code and removing the old codes.
> > However, I hope that it is separated to new bug.
> 
> Will this patch break Windows?

No, this patch only adds the new events for Mac. Windows uses old events which have some minor bugs. So, this patch doesn't break Windows behavior.

Comment 22

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

> If the text is over nsAutoString buffer length, isn't the string buffer made in
> the heap?

Yes but this isn't a problem.

> And the three events are sometimes called 2 or more times at during
> one input event, it depends on the IME implementation.

I still think it's not going to be a problem.

> No, I cannot use SetStart/SetEnd. Because the text nodes is not often a same
> node.

I see, OK.

> No, this patch only adds the new events for Mac. Windows uses old events which
> have some minor bugs. So, this patch doesn't break Windows behavior.

OK

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #22)
> > If the text is over nsAutoString buffer length, isn't the string buffer made in
> > the heap?
> 
> Yes but this isn't a problem.
> 
> > And the three events are sometimes called 2 or more times at during
> > one input event, it depends on the IME implementation.
> 
> I still think it's not going to be a problem.

O.K. I'll remove nsIRange::TextLength and etc..

How do you think for this?

>> > > In OnQueryCharacterRect, it looks like if the queried range is a text node,
>> > > then we return the requested offset in the requested text node, otherwise we
>> > > ignore what was requested and return the current caret position? That doesn't
>> > > make much sense to me.
>> > 
>> > I don't have better idea now...
>> 
>> You mean, you don't know how to implement the non-text-node case?
> 
> And if the editor doesn't have any contents, I don't know that I should refer
> which style for the font size. The root node style is different...

Comment 24

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

if the editor has no contents, does it really matter what the style is? I assume the style only matters for the caret rect height? Is using the root node style bad?

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ummm.... I found some problems in current patch architecture.

I think that IME (or some other applications) wants two information which are caret rect and actual character (or selected element?) rect. On mac, it can be switched by the length of |theRange| of |firstRectForCharacterRange| being zero or not.

And I think that nsRange::ToString ignores non-text contents. However, we should not ignore them nsQueryContentEventHandler. Because the caret rect handling cannot be consistent around the ignored nodes. And br elements should not be ignored, I think. They should be LF or CR or CRLF. Probably other elements should be replaced to a white space.

Comment 26

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

> However, we should not ignore them nsQueryContentEventHandler.

So what will you return in the string for non-text content?

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #26)
> > However, we should not ignore them nsQueryContentEventHandler.
> 
> So what will you return in the string for non-text content?

I think that SPACE(U+0020) is no harm. And it might be enough for TSMDocumentAccess. E.g., When the Japanese characters around <img/>, the both side Japanese words are separated by SPACE. However, I'm thinking about "</p><p>" case. The Japanese words which are around it are combined. It's wrong. Should I insert a space at the end of every element nodes?

Comment 28

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

I'm not really sure. What about list bullets? Do you want to use the plaintext serializer?

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I cannot move the caret to before the list bullets, I think we can ignore them.

And I think that plaintext serializer is too large for nsQueryContentEventHandler. I think that the original simple serializer like nsIDOMRange::ToString is better choice for this bug.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ugh... In some cases, the spaces which are replaced from open/close tags are breaking the reconversion behavior... We should not use space for them, however, the way makes a contradiction for the caret position, umm...

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

Sorry for the delay.

I ignore the complex case for HTML editor. Because I cannot find the good way for supporting it. However, we need to replace <br/> to native LF character(s). That is really needed for reconversion.

Comment 32

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

+  /**
+   * Find the nearest ancestor content which is editable.
+   * And returns the editor if it's needed.
+   */
+  virtual nsIContent* GetRootEditableContent(nsIEditor** aEditor = nsnull);
+
+  /*
+   * Get the nearet selection root content.
+   */
+  virtual nsIContent* GetSelectionRootContent(nsIPresShell* aPresShell);

How about nsLayoutUtils for these, or at least nonvirtual functions? Also FindEditableRoot already does this, so can you check they're really the same and share the code?

It looks like GetSelectionRootContent should share code with nsHTMLEditor::SelectAll as well.

+      if (content->Tag() == nsGkAtoms::br)
+        aString += NATIVE_LF;

Why do we need NATIVE_LF? \n has to work or else preformatted newlines won't give the right results, so why not just use \n here? In other words, if \r\n or \r are needed on some platforms, this patch won't work for <pre> text with newlines. Maybe you should build up the string and then do a global replace of \n with NATIVE_LF?

Looks like SetRangeFromFlatTextOffset duplicates code with GenerateFlatTextContent. To keep things consistent, how about making GenerateFlatTextContent also fill in an nsTArray with one array element per string character, each array element being an nsIContent*,PRUint32 pair?

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #32)
> +  /**
> +   * Find the nearest ancestor content which is editable.
> +   * And returns the editor if it's needed.
> +   */
> +  virtual nsIContent* GetRootEditableContent(nsIEditor** aEditor = nsnull);
> +
> +  /*
> +   * Get the nearet selection root content.
> +   */
> +  virtual nsIContent* GetSelectionRootContent(nsIPresShell* aPresShell);
> 
> How about nsLayoutUtils for these, or at least nonvirtual functions? Also
> FindEditableRoot already does this, so can you check they're really the same
> and share the code?

If I use nsLayoutUtils, the static member cannot be called from toolkit (failed to link). And for same reason, they should be virtual method in nsINode.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #32)
> Looks like SetRangeFromFlatTextOffset duplicates code with
> GenerateFlatTextContent. To keep things consistent, how about making
> GenerateFlatTextContent also fill in an nsTArray with one array element per
> string character, each array element being an nsIContent*,PRUint32 pair?

I cannot understand what you imagined. GenerateFlatTextContent uses nsIRange, however, SetRangeFromFlatTextOffset resolves nsIRange...

Comment 35

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

(In reply to comment #33)
> (In reply to comment #32)
> > +  /**
> > +   * Find the nearest ancestor content which is editable.
> > +   * And returns the editor if it's needed.
> > +   */
> > +  virtual nsIContent* GetRootEditableContent(nsIEditor** aEditor = nsnull);
> > +
> > +  /*
> > +   * Get the nearet selection root content.
> > +   */
> > +  virtual nsIContent* GetSelectionRootContent(nsIPresShell* aPresShell);
> > 
> > How about nsLayoutUtils for these, or at least nonvirtual functions? Also
> > FindEditableRoot already does this, so can you check they're really the same
> > and share the code?
> 
> If I use nsLayoutUtils, the static member cannot be called from toolkit (failed
> to link). And for same reason, they should be virtual method in nsINode.

How about making them nonvirtual methods in nsINode so we don't have to bloat too many classes? And have nsIDocument::GetRootEditableContentExternal which is a virtual wrapper that toolkit can call? Putting it in nsIDocument means we don't have to increase the vtable size for many classes.

(In reply to comment #34)
> I cannot understand what you imagined. GenerateFlatTextContent uses nsIRange,
> however, SetRangeFromFlatTextOffset resolves nsIRange...

OK, don't worry about this one for now, although it'd still be good to share some code here if we can.

Another comment:

+      nsCOMPtr<nsIDOMText> textNode(do_QueryInterface(content));

Probably be easier to use nsIContent::GetText().

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #35)
> Another comment:
> 
> +      nsCOMPtr<nsIDOMText> textNode(do_QueryInterface(content));
> 
> Probably be easier to use nsIContent::GetText().

mmmmm..... is this right?? nsIContent::GetText() returns nsTextFragment but it has Get1b and Get2b, so, the code becomes more complex.

Comment 37

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

It also has AppendTo and CharAt.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

o.k. thanks!

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.0

> It looks like GetSelectionRootContent should share code with
> nsHTMLEditor::SelectAll as well.

No, ns*Editor has root content in its member. And it comes from the param of ns*Editor::Init().

Comment 40

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

Why can't GetRootEditableContent just do what FindEditableRoot does, plus some extra code to get the editor? They always return the same nsIContent, right?

I also don't understand why GetSelectionRootContent can't share code with the editor code. The range we create using GetSelectionRootContent is the same range we would select in nsHTMLEditor::SelectAll, right?

+#undef NATIVE_LF
+#undef NEED_TO_REPLACE
+
+#if defined(XP_MACOSX)
+#define NATIVE_LF NS_LITERAL_STRING("\r")
+#define NEED_TO_REPLACE 1
+#elif defined(XP_WIN)
+#define NATIVE_LF NS_LITERAL_STRING("\r\n")
+#define NEED_TO_REPLACE 1
+#else
+#define NATIVE_LF NS_LITERAL_STRING("\n")
+#define NEED_TO_REPLACE 0
+#endif

A simpler way to do this would be to define a function
  static void ConvertToNativeNewlines(nsString* aString)
which is defined to do the right thing on each platform. Then you don't need to #define anything and there are no #ifdefs in the main code.

+  if (NATIVE_LF.Length() == 1)
+    return aContent->TextLength();

Don't do this optimization, it's not worth it.

I wouldn't do the conversion in AppendString. I would do it at the very end before you return the overall string or compute its length. Then you don't need to use NATIVE_LF, you can just use '\n' for <br>.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #40)
> Why can't GetRootEditableContent just do what FindEditableRoot does, plus some
> extra code to get the editor? They always return the same nsIContent, right?
> 
> I also don't understand why GetSelectionRootContent can't share code with the
> editor code. The range we create using GetSelectionRootContent is the same
> range we would select in nsHTMLEditor::SelectAll, right?

er, OK. There is a difference. I think that GetSelectionRootContent should have FindEditableRoot. And GetRootEditableContent is not same as FindEditableRoot. It should be renamed GetEditorRootContent, I think.

Comment 42

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

Can you explain why they're not the same?

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #42)
> Can you explain why they're not the same?

Because GetRootEditableContent() returns the result of nsIEditor::GetRootElement(). However, FindEditableRoot() returns an editable content which parent is non-editable content. So, they might be different.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.1

Comment 45

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

In what situations would those be different?

Comment 46

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

Even if they were different, wouldn't FindEditableRoot be OK for this bug?

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #45)
> In what situations would those be different?

contentEdiable case. The editor root and selection root might be different.

So, FindEditableRoot is not same as GetTextEditorRootContent and GetHTMLEditorRootContent. That is same as GetSelectionRootContent.

Comment 48

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

What case? Please give an example ... and explain why using FindEditableRoot in that case would not work for your patch here.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #48)
> What case? Please give an example ... and explain why using FindEditableRoot in
> that case would not work for your patch here.

er, no. the latest patch's GetSelectionRootContent is same as FindEditableRoot for HTML editor. Therefore, I called GetSelectionRootContent from nsHTMLEditor instead of FindEditableRoot. (Not *GetHTMLEditorRootContent*)

Comment 50

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

Yes. Sorry.

Why are the changes to nsTypeAheadFind needed here?

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #50)
> Why are the changes to nsTypeAheadFind needed here?

Only for share the code. There are no more reasons.

Comment 52

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

+  /*
+   * Get the nearet selection root content.
+   */

You probably want to explain that this is the content that will be selected if the user does "Select All" while the focus is in the node.

+    content = do_QueryInterface(node);
+    if (content)
+      break;

Use IsNodeOfType(eCONTENT)? Then 'content' doesn't need to be an nsCOMPtr.

GetHTMLEditorRootContent seems like it will always return the document's BODY element even if the node is in a contenteditable region, which is not very useful, so might as well not expose this as an API and just inline it into nsINode::GetSelectionRootContent which is the only place you use it.

+  else if (aContent->IsNodeOfType(nsINode::eELEMENT)) {
+    if (aContent->Tag() == nsGkAtoms::br)

Just test Tag(). I don't know what text nodes return for Tag() but it can't be 'br'.

+    } else if (content->IsNodeOfType(nsINode::eELEMENT)) {
+      if (content->Tag() == nsGkAtoms::br)
+        aString += NS_LITERAL_STRING("\n");

Just test Tag(). Also use Append('\n').

+/*
+ * Query Content Event Handler
+ */

Please include comments explaining what this class is for and what each of these methods do.

+    if (!aEvent->mReply.mString && !aEvent->mReply.mLength) {
+      aEvent->mSucceeded = PR_TRUE;
+      return NS_OK;
+    }

Instead of supporting selective query of the offset, string and length, it would be simpler to always return all three.

+  if (aEvent->mReply.mString)
+    aEvent->mReply.mString->Truncate();

Just require that the caller pass an empty mString (and assert that that's true).

+        if (aEvent->mReply.mString)
+          aEvent->mReply.mString->AppendLiteral("  ");

It would be safe (and simpler) to just append "  " before the first string too, right?

Why do we support multi-range selections converted to text, but we only support querying an offset in the first selection? It would be relatively simple to support both, by moving the offset query into the loop over ranges, right?

nsQueryContentEventHandler::OnQueryCharacterRect might have problems if the character is the first surrogate of a pair or part of a cluster. Maybe we should pass length 0 to SetRangeFromFlatTextOffset and then do a bit more work to get the text frame to tell us where the cluster ends.

You might be able to share code between OnQueryCharacterRect and OnQueryCaretRect --- the code that takes a flat-text offset and returns the character rect. Then the caretrect code would just override the width with the caret width.

+    NS_WARNING("GetSelectionRootContent was failed!?");

Just "GetSelectionRootContent failed"

Comment 53

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

Note to self: I need to check the coordinate stuff.

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.2

Thanks.

* I remove the multi-range support for NS_QUERY_SELECTED_TEXT now. Because Gecko1.9 is not needed it. It will be implemented if it is needed.

* I add |nsQueryContentEvent::mReply.mContentsRoot| that is set the selection root content pointer. It is used for |NSTextInput::conversationIdentifier|.
http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Protocols/NSTextInput_Protocol/Reference/Reference.html#//apple_ref/occ/intfm/NSTextInput/conversationIdentifier
It seems that if the selection root is different, we should return different ID by this method. (But it is not called in my environment and my tests.) I think that the base of text offset is changed, IMEs should be able to know it by this method.

* I remove |nsQueryContentEvent::mReply.mLength|. Because the user can know the length from actual string.

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, I forgot to add surrogate pair checking, sorry. The new patch will come soon.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #52)
> nsQueryContentEventHandler::OnQueryCharacterRect might have problems if the
> character is the first surrogate of a pair or part of a cluster. Maybe we
> should pass length 0 to SetRangeFromFlatTextOffset and then do a bit more work
> to get the text frame to tell us where the cluster ends.

Can I use nsIFrame::PeekOffsetCharacter for this? And must the start and the end of a textframe be the caret acceptable point? Do need to pass length 0 to SetRangeFromFlatTextOffset? I think that we only need to expand the range for clusters.

Comment 57

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

(In reply to comment #56)
> Can I use nsIFrame::PeekOffsetCharacter for this?

Yes

> And must the start and the
> end of a textframe be the caret acceptable point?

In principle, no --- you can have clusters spanning textframe boundaries. But we don't handle those very well and I think for your work here we can ignore that case. Maybe with an XXX comment.

> Do need to pass length 0 to
> SetRangeFromFlatTextOffset? I think that we only need to expand the range for
> clusters.

Not sure

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.3

This expands the range for getting the char rect and caret rect.

Comment 59

[Josh Aas]

My understanding of how critical this bug is and how risky it is suggests that this should block the beta 4 release. If you feel otherwise please make a note here.

Comment 60

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

+    void* mContentsRoot;
+    PRUint32* mOffset;
+    nsString* mString; // must be empty string
+    nsRect* mRect;

Instead of these being pointers, why not just make them direct fields?

+  nsIFrame* frame = fs->GetFrameForNodeOffset(aContent, PRInt32(*aXPOffset),
+                                              fs->GetHint(), &offsetInFrame);

Don't use fs->GetHint(). I think you need to set the hint based on aForward.

+  newOffsetInFrame += aForward ? -1 : 1;

I don't think you should do this. PeekOffsetCharacter will move at least one character.

What coordinates is mReply in? That shoudl be documented. nsQueryContentEventHandler::ConvertToTopLevelWindowPosition doesn't seem to actually convert to the top level window position ... what are you trying to do there?

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #60)
> +  newOffsetInFrame += aForward ? -1 : 1;
> 
> I don't think you should do this. PeekOffsetCharacter will move at least one
> character.

Yes, "moving at least one character" is problem. I need the nearest cluster boundary. It might be in current offset.

> What coordinates is mReply in? That shoudl be documented.
> nsQueryContentEventHandler::ConvertToTopLevelWindowPosition doesn't seem to
> actually convert to the top level window position ... what are you trying to do
> there?

Really? I use same logic in |nsCaret::GetViewForRendering|.
http://mxr.mozilla.org/mozilla/source/layout/base/nsCaret.cpp#850
I need same result of |nsCaret::GetViewForRendering| with |eTopLevelWindowCoordinates|.

Comment 62

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

(In reply to comment #61)
> (In reply to comment #60)
> > +  newOffsetInFrame += aForward ? -1 : 1;
> > 
> > I don't think you should do this. PeekOffsetCharacter will move at least one
> > character.
> 
> Yes, "moving at least one character" is problem. I need the nearest cluster
> boundary. It might be in current offset.

OK.

> > What coordinates is mReply in? That shoudl be documented.
> > nsQueryContentEventHandler::ConvertToTopLevelWindowPosition doesn't seem to
> > actually convert to the top level window position ... what are you trying to do
> > there?
> 
> Really? I use same logic in |nsCaret::GetViewForRendering|.
> http://mxr.mozilla.org/mozilla/source/layout/base/nsCaret.cpp#850
> I need same result of |nsCaret::GetViewForRendering| with
> |eTopLevelWindowCoordinates|.

That gets the root view and returns a result relative to the root view, which makes sense to me. You're calling GetOffsetFromView which doesn't necessarily return the root view.

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #62)
> That gets the root view and returns a result relative to the root view, which
> makes sense to me. You're calling GetOffsetFromView which doesn't necessarily
> return the root view.

The aRect has the offset which is the aFrame relative. I need to get the offset of the frame in the nearest view.

Comment 64

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

OK, sorry, you're right.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.4

This patch has another bug fix (on nsChildView.mm). Current trunk converts the result offset with event received widget. But it should be root widget.

Comment 66

[Håkan Waara]

Comment on attachment 302213 [details] [diff] [review]
Patch rv3.4


>+  NSView<mozView>*  GetView() { return mView; }
>+

No need to add this method. Just call GetNativeData(NS_NATIVE_WIDGET) instead on the childview.

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.5

Thank you for your advice.

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 302299 [details] [diff] [review]
Patch rv3.5

Josh:

Would you check the widget/src/cocoa part?

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 302299 [details] [diff] [review]
Patch rv3.5

Peter:

Would you check the HTML editor part?

Comment 70

[Josh Aas]

Comment on attachment 302299 [details] [diff] [review]
Patch rv3.5

+  nsChildView* rootWidget =
+    static_cast<nsChildView*>(mGeckoChild->GetTopLevelWidget());

Won't the top-level widget usually be of type nsCocoaWindow, not nsChildView?

Comment 71

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.6

josh:

It seems that you're right. But the result of the method looks like nsChildView. I'm not sure the reason. But this new patch doesn't depend on the type of nsIWidget, so this is safer.

Comment 72

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Peter:

Did you read my email?
Please hurry up to review the latest patch (HTML editor part), this bug blocks my next IME work.

Comment 73

[Peter Van der Beken [:peterv]]

Comment on attachment 302559 [details] [diff] [review]
Patch rv3.6

>Index: content/base/public/nsINode.h
>===================================================================

>+  /**
>+   * Get the root content of an editor. So, this node must be a descendant of
>+   * an editor. Note that this should be only used for getting input or teatarea
>+   * editor's root content. This method doesn't support HTML editors.

textarea

>Index: content/base/src/nsGenericElement.cpp
>===================================================================

>+static nsIContent* GetEditorRootContent(nsIEditor* aEditor)
>+{
>+  NS_ASSERTION(aEditor, "aEditor must not be null");
>+  nsCOMPtr<nsIDOMElement> rootElement;
>+  nsresult rv = aEditor->GetRootElement(getter_AddRefs(rootElement));
>+  NS_ENSURE_SUCCESS(rv, nsnull);
>+  NS_ENSURE_TRUE(rootElement, nsnull);
>+
>+  nsCOMPtr<nsIContent> rootContent(do_QueryInterface(rootElement));
>+  NS_ASSERTION(rootContent, "nsIDOMElement doesn't have nsIContent!");
>+  return rootContent;
>+}

I would make this:

  nsCOMPtr<nsIDOMElement> rootElement;
  aEditor->GetRootElement(getter_AddRefs(rootElement));
  nsCOMPtr<nsIContent> rootContent = do_QueryInterface(rootElement);
  return rootContent;

>+nsIContent*
>+nsINode::GetTextEditorRootContent(nsIEditor** aEditor)
>+{

>+    nsresult rv = editableElement->GetEditor(getter_AddRefs(editor));
>+    NS_ENSURE_SUCCESS(rv, nsnull);
>+    NS_ENSURE_TRUE(editor, nsnull);

Just checking editor should be enough.

>+    if (aEditor) {
>+      *aEditor = editor;
>+      NS_IF_ADDREF(*aEditor);

use editor.swap

>+    }
>+    return GetEditorRootContent(editor);

Would GetEditorRootContent ever return something different from |node| here? Seems like a lot of work to find the same node anyway.

>+  }
>+  return nsnull;

>+static nsIContent* GetHTMLEditorRootContent(nsPresContext* aPresContext)

>+  nsresult rv = editorDocShell->GetEditor(getter_AddRefs(editor));
>+  NS_ENSURE_SUCCESS(rv, nsnull);
>+  NS_ENSURE_TRUE(editor, nsnull);

Just checking editor should be enough.

>+  return GetEditorRootContent(editor);
>+}
>+
>+nsIContent*
>+nsINode::GetSelectionRootContent(nsIPresShell* aPresShell)
>+{

>+  // First, find the nearest content node
>+  nsIContent* content = nsnull;
>+  for (nsINode* node = this; node; node = node->GetNodeParent()) {
>+    if (node->IsNodeOfType(eDOCUMENT)) {
>+      nsCOMPtr<nsIDocument> doc(do_QueryInterface(this));
>+      NS_ASSERTION(doc, "This is not document node!");
>+      return doc->GetRootContent();
>+    }
>+    if (node->IsNodeOfType(eCONTENT)) {
>+      content = static_cast<nsIContent*>(node);
>+      break;
>+    }
>+  }
>+
>+  if (!content)
>+    return nsnull;

What is this code supposed to do? AFAICT this is just a convoluted way of doing

  if (node->IsNodeOfType(eDOCUMENT)) {
    return static_cast<nsIDocument*>(node)->GetRootContent();
  }
  if (!node->IsNodeOfType(eCONTENT)) {
    return nsnull
  }

  content = static_cast<nsIContent*>(node);


>+
>+  nsIFrame* frame = aPresShell->GetPrimaryFrameFor(content);
>+  NS_ENSURE_TRUE(frame, nsnull);
>+  if (frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION) {
>+    // This node should be a descendant of input/textarea editor.
>+    content = content->GetTextEditorRootContent();
>+    if (content)
>+      return content;
>+    NS_ERROR("Editor is not found!");

So why fall through? Isn't always returning content here the right thing to do?
Why can't you just get the nsFrameSelection from frame and get its limiter instead of calling GetTextEditorRootContent?

>+  }
>+
>+  nsPresContext* presContext = aPresShell->GetPresContext();
>+  if (presContext) {
>+    nsIContent* rootContent = GetHTMLEditorRootContent(presContext);
>+    if (rootContent) {
>+      // This node is in HTML editor.
>+      nsIDocument* doc = content->GetCurrentDoc();
>+      if (!doc || doc->HasFlag(NODE_IS_EDITABLE) ||
>+          !content->HasFlag(NODE_IS_EDITABLE))
>+        return rootContent;

So you're doing unneeded work getting the root content of the editor to throw it away below. Get the editor here (make GetHTMLEditorRootContent into just GetHTMLEditor) and make it

    nsIEditor* editor = GetHTMLEditor(presContext);
    if (editor) {
      // This node is in HTML editor.
      nsIDocument* doc = content->GetCurrentDoc();
      if (!doc || doc->HasFlag(NODE_IS_EDITABLE) ||
          !content->HasFlag(NODE_IS_EDITABLE))
        return GetEditorRootContent(editor);

>+      // If the current document is not editable, but current content is
>+      // editable, we should assume that the child of the nearest non-editable
>+      // ancestor is selection root.
>+      for (nsIContent* parent = content->GetParent();
>+           parent && parent->HasFlag(NODE_IS_EDITABLE);
>+           parent = content->GetParent())
>+        content = parent;
>+      return content;
>+    }
>+  }
>+

>+  nsCOMPtr<nsFrameSelection> fs = aPresShell->FrameSelection();
>+  content = fs->GetLimiter();
>+  if (content)
>+    return content;
>+  content = fs->GetAncestorLimiter();
>+  if (content)
>+    return content;
>+  nsIDocument* doc = aPresShell->GetDocument();
>+  NS_ENSURE_TRUE(doc, nsnull);
>+  return doc->GetRootContent();

Why do you even have this block? The nodes you're returning here could be totally unrelated to this node.

Comment 74

[Peter Van der Beken [:peterv]]

I didn't really review anything outside of content/base/ and editor/ but I see you have a bunch of |aContent->Tag() == nsGkAtoms::br|, are you sure aContent is always (X)HTML?

Comment 75

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #73)
> (From update of attachment 302559 [details] [diff] [review])
> >Index: content/base/src/nsGenericElement.cpp
> >===================================================================
> >+    if (aEditor) {
> >+      *aEditor = editor;
> >+      NS_IF_ADDREF(*aEditor);
> 
> use editor.swap

I'm not sure what you meant. What is editor.swap?

>+    }
>+    return GetEditorRootContent(editor);

Would GetEditorRootContent ever return something different from |node| here?
Seems like a lot of work to find the same node anyway.

I'm not sure. But FAYT of toolkit did this. And I think API of content/ should not depend on spec/implmentation of editor, so, I think this is safe and good.

> >+nsIContent*
> >+nsINode::GetSelectionRootContent(nsIPresShell* aPresShell)
> >+{
> 
> >+  // First, find the nearest content node
> >+  nsIContent* content = nsnull;
> >+  for (nsINode* node = this; node; node = node->GetNodeParent()) {
> >+    if (node->IsNodeOfType(eDOCUMENT)) {
> >+      nsCOMPtr<nsIDocument> doc(do_QueryInterface(this));
> >+      NS_ASSERTION(doc, "This is not document node!");
> >+      return doc->GetRootContent();
> >+    }
> >+    if (node->IsNodeOfType(eCONTENT)) {
> >+      content = static_cast<nsIContent*>(node);
> >+      break;
> >+    }
> >+  }
> >+
> >+  if (!content)
> >+    return nsnull;
> 
> What is this code supposed to do? AFAICT this is just a convoluted way of doing
> 
>   if (node->IsNodeOfType(eDOCUMENT)) {
>     return static_cast<nsIDocument*>(node)->GetRootContent();
>   }
>   if (!node->IsNodeOfType(eCONTENT)) {
>     return nsnull
>   }
> 
>   content = static_cast<nsIContent*>(node);

Why if this node is not doc and content, we should return null?
I think that we should process with nearest content ancestor.

> >+
> >+  nsIFrame* frame = aPresShell->GetPrimaryFrameFor(content);
> >+  NS_ENSURE_TRUE(frame, nsnull);
> >+  if (frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION) {
> >+    // This node should be a descendant of input/textarea editor.
> >+    content = content->GetTextEditorRootContent();
> >+    if (content)
> >+      return content;
> >+    NS_ERROR("Editor is not found!");
> 
> So why fall through? Isn't always returning content here the right thing to do?
> Why can't you just get the nsFrameSelection from frame and get its limiter
> instead of calling GetTextEditorRootContent?

nsFrameSelection doesn't return the editor root, I think. In a fact, nsFrameSelection::GetLimiter doesn't work fine here. And note that this code is ported from FAYT of toolkit.

> >+  nsCOMPtr<nsFrameSelection> fs = aPresShell->FrameSelection();
> >+  content = fs->GetLimiter();
> >+  if (content)
> >+    return content;
> >+  content = fs->GetAncestorLimiter();
> >+  if (content)
> >+    return content;
> >+  nsIDocument* doc = aPresShell->GetDocument();
> >+  NS_ENSURE_TRUE(doc, nsnull);
> >+  return doc->GetRootContent();
> 
> Why do you even have this block? The nodes you're returning here could be
> totally unrelated to this node.

Same logic is used in nsFrameSelection::SelectAll. I think that the aPresShell and this node is related, the result should be related.

(In reply to comment #74)
> I didn't really review anything outside of content/base/ and editor/ but I see
> you have a bunch of |aContent->Tag() == nsGkAtoms::br|, are you sure aContent
> is always (X)HTML?

not, aContent might be non-XHTML content.

roc:

Do I need to add the namespace checking?

Comment 76

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Or checking nsINode::eHTML is enough for XHTML?

Comment 77

[Peter Van der Beken [:peterv]]

(In reply to comment #75)
> I'm not sure what you meant. What is editor.swap?

swap is a method on nsCOMPtr.

> 
> >+    }
> >+    return GetEditorRootContent(editor);
> 
> Would GetEditorRootContent ever return something different from |node| here?
> Seems like a lot of work to find the same node anyway.
> 
> I'm not sure. But FAYT of toolkit did this.

No it doesn't, if it finds an nsIDOMNSEditableElement with an editor it uses that node.

> Why if this node is not doc and content, we should return null?
> I think that we should process with nearest content ancestor.

If it's not an nsIDocument and not an nsIContent it's an nsIAttribute, in which case GetNodeParent returns null. So my code is exactly equivalent to yours, and if this called on an nsIAttribute something is wrong anyway imho, so returning null should be fine.

> nsFrameSelection doesn't return the editor root, I think.

It does for input and textarea.

> In a fact,
> nsFrameSelection::GetLimiter doesn't work fine here.

But why? The limiter is the root of the selection *by definition*, isn't that what this method is trying to return?

> Same logic is used in nsFrameSelection::SelectAll. I think that the aPresShell
> and this node is related, the result should be related.

No, the nodes here could be in a subtree that has no relation to this nsINode except that it's in the same document. If that's the case this method looks misplaced on nsINode. I think you should document that this method should be called on a node that's currently in the selection (which is the case for the callers you add), and then you can just drop that block.

Comment 78

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

maybe you should test QI to nsIDOMHTMLBRElement instead.

Comment 79

[Peter Van der Beken [:peterv]]

IsNodeOfType(nsINode::eHTML) sounds good to me (hopefully slightly faster than QI).

Comment 80

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

That way you'll have to check the tag as well, though.

It really doesn't matter either way. Performance definitely doesn't matter here. Do whatever feels simplest.

Comment 81

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #77)
> > nsFrameSelection doesn't return the editor root, I think.
> 
> It does for input and textarea.
> 
> > In a fact,
> > nsFrameSelection::GetLimiter doesn't work fine here.
> 
> But why? The limiter is the root of the selection *by definition*, isn't that
> what this method is trying to return?

The definition of this method is: It returns the common ancestor element of the selected range when "SelectAll" is executed on this node.

The presShell comes from nsPresContext for ESM when I called from nsQueryContentEventHandler, am I using wrong shell? Can we get the actual another shell for the node??

Comment 82

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The text editors have another presShell that is different from document's presShell?

Comment 83

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #77)
> > Same logic is used in nsFrameSelection::SelectAll. I think that the aPresShell
> > and this node is related, the result should be related.
> 
> No, the nodes here could be in a subtree that has no relation to this nsINode
> except that it's in the same document. If that's the case this method looks
> misplaced on nsINode. I think you should document that this method should be
> called on a node that's currently in the selection (which is the case for the
> callers you add), and then you can just drop that block.

I should say yes for the misplacing. Because I hoped that this should be in content utils. However, nsContentUtils are in dependency hell, so, I cannot use it from toolkit :-(

Comment 84

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

|nsPlainTextEditor's SelectAll| is |nsEditor::SelectAll|. It calls |nsEditor::SelectEntireDocument|. That calls |nsISelection::SelectAllChildren| with the root content of the editor.

Comment 85

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #77)
> > >+    }
> > >+    return GetEditorRootContent(editor);
> > 
> > Would GetEditorRootContent ever return something different from |node| here?
> > Seems like a lot of work to find the same node anyway.
> > 
> > I'm not sure. But FAYT of toolkit did this.
> 
> No it doesn't, if it finds an nsIDOMNSEditableElement with an editor it uses
> that node.

This is false, nsIDOMNSEditableElement is input or textarea element node. But the root node of the editor is the anonymous child div element of the editor.

Comment 86

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.7

I changed the points excepted 2 points: (1) comment 81 (2) comment 84 and comment 85.

And I don't use QI way for br checking. Because:
1. Many points use the IsNodeOfType(nsINode::eHTML) way.
2. I have no idea for I wrote the QI in one line.

Comment 87

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 303600 [details] [diff] [review]
Patch rv3.7

Oops, this patch has a crash bug, sorry.

Comment 88

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.8

Comment 89

[Peter Van der Beken [:peterv]]

Comment on attachment 303615 [details] [diff] [review]
Patch rv3.8

> Index: content/base/public/nsIDocument.h
> ===================================================================

> +   * input or teatarea editor's root content. This method doesn't support HTML

textarea

> +  /*

/**

> +   * Get the nearest selection root content. That will be selected if the user
> +   * does "Select All" while the focus is in the node.
> +   * Note that the node is not in an editor, the result comes from
> +   * nsFrameSelection that is related to aPresShell.
> +   * I.e., the result might not be the ancestor of this node in such case.

  Get the nearest selection root, ie. the node that will be selected if the user
  does "Select All" while the focus is in this node. Note that if this node is
  not in an editor, the result comes from the nsFrameSelection that is related
  to aPresShell, so the result might not be the ancestor of this node.

> Index: content/base/public/nsINode.h
> ===================================================================

> +   * Get the nearest selection root content. That will be selected if the user
> +   * does "Select All" while the focus is in this node.
> +   * Note that this node is not in an editor, the result comes from
> +   * nsFrameSelection that is related to aPresShell.
> +   * I.e., the result might not be the ancestor of the node in such case.

If you update the comment in nsIDocument update it here too.

> Index: content/base/src/nsGenericElement.cpp
> ===================================================================

> +  NS_ASSERTION(aEditor, "aEditor must not be null");

No need for the assertion, we'll crash anyway.

> +  for (nsINode* node = this; node; node = node->GetNodeParent()) {
> +    nsCOMPtr<nsIDOMNSEditableElement> editableElement(do_QueryInterface(node));
> +    if (!editableElement)
> +      continue;
> +
> +    nsCOMPtr<nsIEditor> editor;
> +    nsresult rv = editableElement->GetEditor(getter_AddRefs(editor));
> +    NS_ENSURE_TRUE(editor, nsnull);
> +    nsIContent* rootContent = GetEditorRootContent(editor);
> +    if (aEditor)
> +      editor.swap(*aEditor);
> +    return rootContent;

> > > I'm not sure. But FAYT of toolkit did this.
> > 
> > No it doesn't, if it finds an nsIDOMNSEditableElement with an editor it uses
> > that node.
> 
> This is false, nsIDOMNSEditableElement is input or textarea element node. But
> the root node of the editor is the anonymous child div element of the editor.

Just so we're clear, by 'This is false' you mean that the current FAYT toolkit code is wrong? Note that the current FAYT code looks for a node to focus, which afaict should be the nsIDOMNSEditableElement, and not the anonymous div, which would be the selection root.

> +  nsIFrame* frame =
> +    aPresShell->GetPrimaryFrameFor(static_cast<nsIContent*>(this));
> +  if (frame && frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION) {
> +    // This node should be a descendant of input/textarea editor.
> +    nsIContent* content = GetTextEditorRootContent();

I still don't understand why you can't get the limiter of the frame's nsFrameSelection, it's the root content of the editor for input and textarea.

> Index: editor/libeditor/html/nsHTMLEditor.cpp
> ===================================================================

> +  nsIContent *rootContent = anchorContent->GetSelectionRootContent(ps);
>    if (!rootContent) {
> +    NS_WARNING("GetSelectionRootContent failed");

This really shouldn't fail, assert please.

r=peterv on the content/base and editor code.

Comment 90

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #89)
> > +  for (nsINode* node = this; node; node = node->GetNodeParent()) {
> > +    nsCOMPtr<nsIDOMNSEditableElement> editableElement(do_QueryInterface(node));
> > +    if (!editableElement)
> > +      continue;
> > +
> > +    nsCOMPtr<nsIEditor> editor;
> > +    nsresult rv = editableElement->GetEditor(getter_AddRefs(editor));
> > +    NS_ENSURE_TRUE(editor, nsnull);
> > +    nsIContent* rootContent = GetEditorRootContent(editor);
> > +    if (aEditor)
> > +      editor.swap(*aEditor);
> > +    return rootContent;
> 
> > > > I'm not sure. But FAYT of toolkit did this.
> > > 
> > > No it doesn't, if it finds an nsIDOMNSEditableElement with an editor it uses
> > > that node.
> > 
> > This is false, nsIDOMNSEditableElement is input or textarea element node. But
> > the root node of the editor is the anonymous child div element of the editor.
> 
> Just so we're clear, by 'This is false' you mean that the current FAYT toolkit
> code is wrong? Note that the current FAYT code looks for a node to focus, which
> afaict should be the nsIDOMNSEditableElement, and not the anonymous div, which
> would be the selection root.

er, right. I changed FAYT logic by my mistake. I'll remove FAYT part and nsIDocument changes.

Comment 91

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.9

Thank you, Peter.

So, the previous patch changed the typeaheadfind logic. So, we should not use GetTextEditorRootContent in typeahead find. And nsIDcoument changes are not need now. Roc, would you check this changes?

Comment 92

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in, thanks a lot to everyone!