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)
Core
Spelling checker
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)
4.73 KB,
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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; }
Updated•16 years ago
|
Assignee: mscott → nobody
Comment 1•16 years ago
|
||
See also bug 432225 comment 17
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All
Assignee | ||
Comment 2•16 years ago
|
||
Comment 3•16 years ago
|
||
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?
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #347832 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
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 ?
Assignee | ||
Comment 6•16 years ago
|
||
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)
Attachment #348547 -
Flags: superreview+
Attachment #348547 -
Flags: review?(roc)
Attachment #348547 -
Flags: review+
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 9•16 years ago
|
||
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+
Keywords: checkin-needed
Comment 10•16 years ago
|
||
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]
Updated•16 years ago
|
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
Comment 11•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ca39294038bc
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]
Comment 12•15 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•