Closed Bug 366682 Opened 17 years ago Closed 16 years ago

spellCheckRange : scan only the first word of a text node and stop

Categories

(Core :: Spelling checker, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: sylvain.spinelli, Assigned: sylvain.spinelli)

References

()

Details

(Keywords: testcase, verified1.9.1)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: MOZILLA_1_8_BRANCH, co 2007/01/10, --enable-application=xulrunner --enable-extensions=default,spellcheck --enable-svg --enable-canvas --disable-installer --disable-tests --disable-debug --enable-optimize --disable-static --enable-shared --disable-optimize --enable-debug

The scanning for mispelled words failed in some cases. Exemple :
source : <div>text</div> and no other textnodes in the document, and no BR tag.
range : startNode == endNode == div tag, startOffset==0, enOffset==1.




Reproducible: Always

Steps to Reproduce:
Make a xul page with :
	<script>
		var vEd;
		function makeEditable() {
			vEd = document.getElementById("editor");
			vEd.makeEditable('text',false);
		}
		
		function setSpellCheck() {			
			//Remove the BR tag added by the editor.
			var vBody = vEd.contentDocument.body;
			if(vBody.lastChild.nodeName=="BR")	vBody.removeChild(vBody.lastChild);

			vEd.	getEditor(vEd.contentWindow).QueryInterface(Components.interfaces.nsIEditor_MOZILLA_1_8_BRANCH).setSpellcheckUserOverride(true);
		}
	</script>
	<editor id="editor" src="simplePage.html" style="height:500px; width:400px;" />
	<button label="1 - make Editable" oncommand="makeEditable()"/>
	<button label="2 - activeSpellCheck" oncommand="setSpellCheck();"/>

with in simplePage.html : <html><body><div>errror and an other errror.</div></body></html>

1. Launch a xul app with 
2. Press make editable button
3. Press activeSpellCheck button 

Actual Results:  
Only the first word "errror" is analyzed.

Expected Results:  
The 2 words "errror" should be mispelled.

The problem is in the mozInlineSpellWordUtil.cpp, in FindNextTextNode static method.

When the offset is after the last child, the actual code go back to the last child for searching the next textNode. The textNode which should be scanned is returned and then considered outside the scope.

Here is a patch :

Index: extensions/spellcheck/src/mozInlineSpellWordUtil.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp,v
retrieving revision 1.4.2.8
diff -u -8 -p -r1.4.2.8 mozInlineSpellWordUtil.cpp
--- extensions/spellcheck/src/mozInlineSpellWordUtil.cpp	9 Aug 2006 16:45:17 -0000	1.4.2.8
+++ extensions/spellcheck/src/mozInlineSpellWordUtil.cpp	11 Jan 2007 13:55:04 -0000
@@ -188,24 +188,31 @@ FindNextTextNode(nsIDOMNode* aNode, PRIn
     nsCOMPtr<nsIDOMNode> next;
     child->GetNextSibling(getter_AddRefs(next));
     child.swap(next);
     --aOffset;
   }
   if (child) {
     checkNode = child;
   } else {
-    // aOffset was beyond the end of the child list. Start checking at the next
-    // node after the last child, or aNode if there are no children.
-    aNode->GetLastChild(getter_AddRefs(child));
-    if (child) {
-      checkNode = FindNextNode(child, aRoot);
-    } else {
-      checkNode = FindNextNode(aNode, aRoot);
-    }
+	// aOffset was beyond the end of the child list. 
+	// goto next node in a preorder DOM traversal.
+	nsCOMPtr<nsIDOMNode> next;
+	aNode->GetNextSibling(getter_AddRefs(next));
+	while ( !next) {
+		// Go up
+		aNode->GetParentNode(getter_AddRefs(next));
+		if (next == aRoot || ! next) {
+			return nsnull;
+		} else {
+			aNode = next;
+			aNode->GetNextSibling(getter_AddRefs(next));
+		}
+	}
+	checkNode = next;
   }
   
   while (checkNode && !IsTextNode(checkNode)) {
     checkNode = FindNextNode(checkNode, aRoot);
   }
   return checkNode;
 }
Assignee: mscott → nobody
See also bug 432225 comment 17
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All
Attached patch patch for 1.9.1 (obsolete) — Splinter Review
SylvainS, if this patch is ready for review, roc would probably be a reasonable person to request review from.  If not, what needs to happen next?
Attached patch patch for 1.9.1 (typo changes) (obsolete) — Splinter Review
Attachment #347832 - Attachment is obsolete: true
Attachment #348171 - Flags: review?(roc)
+      if (next == aRoot || !next) {
+        return nsnull;
+      } else {
+        aNode = next;
+        aNode->GetNextSibling(getter_AddRefs(next));
+      }

Don't do 'else' after 'return' --- the 'else' statements can just move up one level.

+        aNode = next;

This can be aNode.swap(next).

Otherwise looks good. It would be nice to have a mochitest for this; can you make a test like http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug432225.html?raw=1 ?
Here is the patch with a mochitest.

> +        aNode = next;
> This can be aNode.swap(next).

aNode type is nsIDOMNode*. So aNode.swap(next) failed to compile.
Attachment #348171 - Attachment is obsolete: true
Attachment #348547 - Flags: review?(roc)
Attachment #348171 - Flags: review?(roc)
Very nice thanks! Let's see if we can get this in for Firefox 3.1. (If not it would wait till Firefox 3.2.)
Comment on attachment 348547 [details] [diff] [review]
patch for 1.9.1 with mochitest
[Checkin: Comment 10]

I think we should take this for 1.9.1. Low-risk patch with tests that fixes an subtle but visible spellcheck issue.
Attachment #348547 - Flags: approval1.9.1?
Comment on attachment 348547 [details] [diff] [review]
patch for 1.9.1 with mochitest
[Checkin: Comment 10]

a191=beltzner
Attachment #348547 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 348547 [details] [diff] [review]
patch for 1.9.1 with mochitest
[Checkin: Comment 10]

http://hg.mozilla.org/mozilla-central/rev/2ee44f184ffc
Attachment #348547 - Attachment description: patch for 1.9.1 with mochitest → patch for 1.9.1 with mochitest [Checkin: Comment 10]
Assignee: nobody → sylvain.spinelli
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
Seeing as there hasn't been any discussions about this bug for 4 1/2 months and it's been in mochitest, I'm assuming there aren't any residual issues. I'm moving this to verified as a result. If anyone has any qualms, feel free to bring them up.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.