Closed Bug 173046 Opened 22 years ago Closed 22 years ago

SpellCheck in Mail should skip blockquote

Categories

(SeaMonkey :: Composer, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.3beta

People

(Reporter: rods, Assigned: bugzilla)

References

Details

Attachments

(2 files, 8 obsolete files)

When blockquotes are of type "cite" spellcheck needs to skip them.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.2beta
Summary: SpellCheck in Mail shoild skip blockquote → SpellCheck in Mail should skip blockquote
I was going to move a bugscape bug over, but I have decided against it
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → INVALID
Ok, I do want this bug open
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
This is Bugscape 8868.
Status: REOPENED → ASSIGNED
Attached patch patch v1 (obsolete) (deleted) — Splinter Review
Basically this patch wraps the ContentIterator with a class that can have a filter. The filter can then be used to "skip" certain content nodes like a blockquote. When composing in mail and doing spell check we ask the mail iface if there is a filter to be installed for filtering the mail content.
Comment on attachment 101999 [details] [diff] [review] patch v1 ==== I think we may want to have a filter that is shared by both Composer and MailNews, the only difference would be that mail would conditionally tell the filter to also skip over the blockquote[cite=cite]. Having said that, this would probably have to be moved out from nsIEditorMailSupport.idl: + void getContentIteratorFilter(out nsIContentIteratorFilter aFilter); ==== This filter doesn't belong at the editor core level, since it is an app specific thing. Editor core should not know anything about spellchecking: + nsMailSpellCheckFilter.cpp \ ==== I think this needs to be restructured, otherwise we'll never check for anything but blockquotes: + if (tag) { + if (tag == mBlockQuoteAtom) { + nsAutoString cite; + if (NS_SUCCEEDED(aNode->GetAttr(kNameSpaceID_None, mTypeAtom, cite))) { + return cite.Equals(NS_LITERAL_STRING("cite").get()); + } + } + } else if (tag == mScriptAtom) { + return PR_TRUE; + } else if (tag == mTextAreaAtom) { + return PR_TRUE; + } else if (tag == mSelectAreaAtom) { + return PR_TRUE; + } else if (tag == mMapAtom) { + return PR_TRUE; + } ==== The filter interface should be implemented in IDL so that people can have a filter written in JS if necessary. I'd also rename it to something like nsTextServicesFilter.idl instead of nsIContentIteratorFilter.h. ==== InitWithDocument() needs to be modified too. Are you sure you want to tie the filter to the Init methods? Doing so means we won't be able to dynamically add filters (imagine checkboxes on a dialog). - NS_IMETHOD InitWithEditor(nsIEditor *aEditor) = 0; + NS_IMETHOD InitWithEditor(nsIEditor *aEditor, nsIContentIteratorFilter* aFilter) = 0; ==== I really think the skipping needs to be done in FilteredContentIterator::Next() and Prev() instead of CurrentNode(), so that the direction of iteration can be taken into account. Also, you have to keep in mind that the next or previous sibling can also be a node to skip, so there needs to be some sort of loop. Also if there is no next or previous sibling it may be necessary to go to the parent's next or previous sibling. ==== Also the |IsBlock()| should be |DidCrossBlockBoundary()| or something of that nature, and that should get set to true whenever we skip something.
Attachment #101999 - Flags: needs-work+
*** Bug 145706 has been marked as a duplicate of this bug. ***
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
A bit different from the first patch: 1) Created a new iface nsITextServicesFilter 2) The impl of this filter is craeted by the EditorShell and set into the TextService obj 3) A new obj FilteredContentIterator "wraps" a content iterator obj and has nsITextServicesFilter to figure out what nodes need to be skipped. 4) As the TexSvc processes node it uses the filtered content iterator and can then skip nodes going forward or backward.
Attachment #101999 - Attachment is obsolete: true
Cc jfrancis ... we talked long ago about filtered content iterators in the context of this bug, and I remember him saying that he was thinking of or had some other uses for such a thing. I'm wondering if we should have a generic iterator filter mechanism outside of the textservices module that could be used by all.
Comment on attachment 102808 [details] [diff] [review] patch v2 In the context of the TextServices, this all looks good. It may need more work if we wanted a general content filtering mechanism to share between mozilla/content and mozilla/editor/txtsvc. ==== I'd probably rename this file and it's class to nsComposerTSFilter or nsComposerTxtsvcFilter: + nsComposeFilter.cpp \ ==== FYI, all spellchecker related code that lived in |nsEditorShell| has now moved to a different class |nsEditorSpellCheck|. So some of the nsEditorShell code in this patch will need updating/changing. ==== Right now the skipping of quotes is tied to the |mMailCompose| editor isn't there supposed to be some sort of pref that we need to check in addition? That is I thought we were supposed to be able to dynamically turn on/off the skipping of quotes in mail? + if (NS_SUCCEEDED(CreateComposeFilter(getter_AddRefs(txtSvcFilter), mMailCompose))) { ==== I see a couple of references to |nsIContentIteratorFilter| which is a class/interface that doesn't exist. +interface nsIContentIteratorFilter; +class nsIContentIteratorFilter; ==== The comment shouldn't reference spellchecking, instead it should say something generic like "Sets the filter to be used while iterating over content. @param aFilter filter to be used while iterating over content." Also, the current implementation isn't very dynamic since it requires that any filter used, must be set prior to calling the |InitWithDocument()| or |InitWithEditor()| methods which create the iterator. That should be mentioned in the comment too. + + /** + * Sets a filter + * @param aFilter filter to be used to filter content for spellcheck + */ + NS_IMETHOD SetFilter(nsITextServicesFilter *aFilter) = 0; ==== |nsIContent| isn't scriptable so this might have to be changed to |nsIDOMNode|. What we want to do is eventually allow the mail guys or embedders to add filters, in js or C++. + boolean skip(in nsIContent aNode); ==== FilteredContentIterator's |First()| and |Last()| may also need to do some skipping/advancing. A real world example of this would be the "Start after quoted reply" pref in mail, since that would mean the content we want to be skipped would be the first thing the iterator would iterate over. ==== The textservices does a fair amount of calls to PositionAt(), you may need to clear/reset some of FilteredContentIterator's state flags in that method? ==== You may want to track |mDirection| in FilteredContentIterator's |MakePre()| and |MakePost()| just in case the caller manually switches things up! Right now the TextServicesCode doesn't switch the modes on the iterator, but if this code were ever going to be made generic it brings some interesting issues of how to advance to the non-filtered node without interfering with the user's specified traversal. ==== I don't like the fact that the |IsBlock()| now takes an iterator. It's meant to be a generic method to get info about a node. You are tying the node passed in to the current state of an iterator. It's probably best to move that code into it's own utility method and call it from the places that use the iterator. Are you sure that all |IsBlock()| calls need to know boundary crossings? It's been a while, so I don't remember off the top of my head. -nsTextServicesDocument::IsBlockNode(nsIContent *aContent) +nsTextServicesDocument::IsBlockNode(nsIContent *aContent, nsIContentIterator* aFilteredIter) { + // We can assume here that the Iterator is a FilteredContentIterator because + // all the iterator are created in CreateContentIterator which create a + // FilteredContentIterator + // So if the iterator bailed on one of the "filtered" content nodes then we + // consider that to be a block and bail with PR_TRUE + if (aFilteredIter) { + FilteredContentIterator* filter = NS_STATIC_CAST(FilteredContentIterator *, aFilteredIter); + if (filter) { + if (filter->DidCrossBlockBoundary()) { + return PR_TRUE; + } + } + } +
Comment on attachment 102808 [details] [diff] [review] patch v2 ==== These enums are kind of the same, can't we share their use? + typedef enum {eDirNotSet, eForward, eBackward} eDirectionType; + typedef enum {eAdvNext, eAdvPrev} eAdvNodeDirType;
Attached patch patch v3 (obsolete) (deleted) — Splinter Review
This patch: 1)Does not handle First and Last, in otherwaords, if the very first node is a blockquote it won't skip over it. Fixing that case will require reworking the ContentIterator 2) It now creates the filter from a factory. The filter can be created for mail or for composer. 3) The FilteredContentIterator and the nsITextServicesFilter use nsIDOMNode so filters can be scriptable.
Attachment #102808 - Attachment is obsolete: true
Attached file htmldiff of patch (obsolete) (deleted) —
For your reviewing/super-reviewing convenience
Target Milestone: mozilla1.2beta → mozilla1.3alpha
could this bug be super-reviewed?
We are waiting one some changes to the content iterators before this can be checked in. This bug should have a dependency...
Hi All, A fresh build that works with 1.2b (1.2.0.2002111508) was posted at http://bugzilla.mozilla.org/attachment.cgi?id=106306&action=view Unfortunately, it still checks block quoted text (which is sad, as it makes pathetic an otherwise excellent piece of writing). We may all have to wait for Netscape's spell checker to catch up with 1.2. --Tony
Depends on: 176251
rod, can we get an updated diff to review? The last set of changes in this bug don't have the modifications I gave you that call ClearDidCrossBoundary() which fixed some of your test cases.
Attached patch patch v4 (obsolete) (deleted) — Splinter Review
Same as Patch v3 except the FilteredIterator now used Kin's new pre/post iterators
Attachment #103311 - Attachment is obsolete: true
Attachment #103312 - Attachment is obsolete: true
Attachment #106919 - Flags: superreview?(kin)
Attachment #106919 - Flags: review?(jfrancis)
Just wanted to mention a few related issues: http://mozdev.org/bugs/show_bug.cgi?id=751 : "<name> wrote:" quotation line should not be checked http://mozdev.org/bugs/show_bug.cgi?id=752 : Quoted text should not be checked. http://mozdev.org/bugs/show_bug.cgi?id=788 : Don't spellcheck sigs http://mozdev.org/bugs/show_bug.cgi?id=971 : Spellchecker should ignore URLs
OS: Windows 2000 → All
Hardware: PC → All
Aleksey, These other things need to be "wrapped" in special kind of tags that we can then skipped. The mail team is aware.
Blocks: 119232
Comment on attachment 106919 [details] [diff] [review] patch v4 ==== Change "node" to "nodes": + // For node that are blockquotes, we must make sure ==== We'll want to remove this debugging ifdef code before checkin: + if (tag == mBlockQuoteAtom) { +#if 1 + *_retval = PR_TRUE; + return NS_OK; +#else ==== Do we want to do a case-insensitive compare? + *_retval = cite.Equals(NS_LITERAL_STRING("cite").get()); ==== Were you going to fill in these comments? :-) +/** + * + */ +class nsComposeTxtSrvFilter : public nsITextServicesFilter ==== Perhaps we should define the Contract/ProgID here too so that it's easier for folks find. The define could also be used in place of the hardcoded string in nsComposerRegistration.cpp. +#define NS_TXTSRVFILTER_CID \ +{/* {171E72DB-0F8A-412a-8461-E4C927A3A2AC}*/ \ +0x171e72db, 0xf8a, 0x412a, \ +{ 0x84, 0x61, 0xe4, 0xc9, 0x27, 0xa3, 0xa2, 0xac} } + ==== Make the indentation for the code in |nsComposeTxtSrvFilterConstructor()| match the rest of the file. ==== What's this comment for? +/* void setFilter (in nsITextServicesFilter filter); */ +NS_IMETHODIMP +nsEditorSpellCheck::SetFilter(nsITextServicesFilter *filter) ==== We shouldn't be depending on txtsvc in core editor code, specifically, editor/libeditor/base/Makefile.in or editor/libeditor/text/Makefile.in layout \ content \ txmgr \ + txtsvc \ htmlparser \ necko \ pref \ ==== So why does the constructor for |nsFilteredContentIterator()| take a range arg? |mRange| should be set in the range version of |Init()| so that we are totally in sync with the range that get's used when |mIter->Init(aRange)| is called. ==== In |nsFilteredContentIterator::Init(nsIContent* aRoot)| we should probably create an |mRange| that selects the root node. This can be done easily with the |SelectNode()| method on nsIRange. ==== Like I said above, |nsFilteredContentIterator::Init(nsIDOMRange* aRange)| should be setting mRange. Do we need to create a copy of |aRange| and set |mRange| to the copy? Maybe not if the only code that creates this iterator doesn't modify the range after it gives it to us. Note that the underlying content iterator doesn't store a range so this isn't an issue for it. ==== You can no longer dynamically switch directions so this comment |Next()| and |Prev()| isn't necessary anymore. + // ASSUMPTION: We are assuming here that no one will + // call MakePre or MakePost while iterating ==== Shouldn't this be doing some initialization? + nsFilteredContentIterator() { } ==== This debugging code needs to be commented out or removed ... as well as the KDEBUG code you enabled. +int cnt = 0; NS_IMETHODIMP nsTextServicesDocument::DeleteSelection() { ==== You set |result| based on |filter|, but if |filter| were ever null you would've crashed because of the AddRef. + nsFilteredContentIterator* filter = new nsFilteredContentIterator(mTxtSvcFi lter, aRange); + *aIterator = NS_STATIC_CAST(nsIContentIterator *, filter); + NS_ADDREF(*aIterator); + result = filter?NS_OK:NS_ERROR_FAILURE;
I purposely didnt read kin's review comments, so there is probably overlap. Makefile.in: + nsComposeTxtSrvFilter.cpp You also need to add this to the mac build, via editor.xml. Similarly include your other new source files in the appropriate project.xml --------------------- in nsComposeTxtSrvFilter.cpp: +nsComposeTxtSrvFilter::nsComposeTxtSrvFilter() : + mIsForMail(PR_FALSE) +{ + NS_INIT_ISUPPORTS(); + + mBlockQuoteAtom = getter_AddRefs(NS_NewAtom("blockquote")); + mTypeAtom = getter_AddRefs(NS_NewAtom("type")); + mScriptAtom = getter_AddRefs(NS_NewAtom("script")); + mTextAreaAtom = getter_AddRefs(NS_NewAtom("textarea")); + mSelectAreaAtom = getter_AddRefs(NS_NewAtom("select")); + mMapAtom = getter_AddRefs(NS_NewAtom("mMapAtom")); +} Is this an exhaustive list of the tags you want to skip? Do we need <head>, <object>, <applet>, & <embed>? +#if 1 + *_retval = PR_TRUE; + return NS_OK; +#else should be removed --------------------- in nsComposeTxtSrvFilter.cpp: +#define NS_TXTSRVFILTER_CID \ CID should have a name closer to the implementation (nsComposeTxtSrvFilter) to avoid confusion with other (future) Text Services Filters. --------------------- in nsComposerRegistration.cpp: throughout, and in the header, mDidCross is described as indicating whether you crossed a block boundary, when in fact it means did you skip a node. Fix discriptions. In fact, I suggest mDidSkip. Similarly, rename: + PRBool DidCrossBlockBoundary() { return mDidCross; } + void ClearDidCrossBlockBoundary() { mDidCross = PR_FALSE; } fix typo: +// There are no macros that enable use to have 2 constructors +static NS_IMETHODIMP There are two static function that are NS_IMETHODIMP. Should be static nsresult. --------------------- in nsFilteredContentIterator.cpp: +nsFilteredContentIterator::~nsFilteredContentIterator() +{ + mIterator = nsnull; +} mIterator is an nsCOMPtr. nulling it is unneccessary. +nsFilteredContentIterator::Init(nsIContent* aRoot) +{ + NS_ENSURE_TRUE(mIterator, NS_ERROR_FAILURE); + mIsOutOfRange = PR_FALSE; + mDirection = eForward; + mCurrentIterator = mPreIterator; + return mCurrentIterator->Init(aRoot); +} + +NS_IMETHODIMP +nsFilteredContentIterator::Init(nsIDOMRange* aRange) +{ + NS_ENSURE_TRUE(mIterator, NS_ERROR_FAILURE); + mIsOutOfRange = PR_FALSE; + mDirection = eForward; + mCurrentIterator = mPreIterator; + return mCurrentIterator->Init(aRange); +} Add NS_ENSURE_TRUE(mPreIterator, NS_ERROR_FAILURE); to these. +nsresult +nsFilteredContentIterator::AdvanceNode(nsIDOMNode* aNode, nsIDOMNode*& aNewNode, eDirectionType aDir) ... + aNode->GetParentNode(getter_AddRefs(parent)); + NS_ASSERTION(parent, "parent can't be NULL"); + + // Make sure the parent is in the DOMRange before going further + PRBool intersects; + nsresult rv = mRange->IntersectsNode(parent, &intersects); + if (NS_SUCCEEDED(rv) && intersects) { How can parent not intersect range? Presumably aNode intersected range, right? +void +nsFilteredContentIterator::CheckAdvNode(nsIDOMNode* aNode, PRBool& aDidCross, eDirectionType aDir) +{ ... + } else { + return; // fell out of range CheckAdvNode() only skips over nodes if it finds a later node not to skip. This has some ramifications for the rest of the nsFilteredContentIterator class. For instance, if a range has only a mailcite in it, First() will be set to the mailcite (and mIsOutOfRange will be set). But since CurrentNode() does not check mIsOutOfRange, you will spell check the mailcite. In Next() & Prev(), delete the lines: + // ASSUMPTION: We are assuming here that no one will + // call MakePre or MakePost while iterating In Next(), Prev(), and CurrentNode(): check mIsOutOfRange and do the right thing. --------------------- nsTextServicesDocument.cpp: +int cnt = 0; ... + printf("\n---- Before Delete %d\n", cnt++); + printf("Sel: (%2d, %4d) (%2d, %4d)\n", mSelStartIndex, mSelStartOffset, mSelEndIndex, mSelEndOffset); + PrintOffsetTable(); remove/ifdef/comment-out debugging cruft *aIterator = 0; + // Create a nsFilteredContentIterator + // This class wraps the ContentIterator in order to give itself a chance + // to filter out certain content nodes + if (!*aIterator) { Why the if statement? We know *aIterator = 0 from line above. + result = filter?NS_OK:NS_ERROR_FAILURE; some whitespace in there would be nice. I would prefer if (!filter) return NS_ERROR_FAILURE; which should do the job given what happens later. +nsTextServicesDocument::ClearDidCrossBoundary(nsIContentIterator* aFilteredIter) +{ + // We can assume here that the Iterator is a nsFilteredContentIterator because + // all the iterator are created in CreateContentIterator which create a + // nsFilteredContentIterator + // So if the iterator bailed on one of the "filtered" content nodes then we + // consider that to be a block and bail with PR_TRUE fix comment. Looks copied from nsTextServicesDocument::DidCrossBoundary() in FirstTextNodeInCurrentBlock(): + + if (DidCrossBoundary(iter)) + break; This terminates the loop, possibly before we find the first text node in block. I guess you are relying on skipped nodes always being block boundaries in their own right. Do you mean to make that assumtion? Filters that looked for spans that had a style attribute with visibilty : hidden, for instance, could not work with this. Similar comments apply to FirstTextNodeInNextBlock(). And though not touched by the patch, FirstTextNodeInPrevBlock() is also vulnerable since it uses FirstTextNodeInCurrentBlock() under the hood.
Rod, ignore my last comment in the review. I am unfamiliar with Text Services and didnt realize that it's "blocks" were actually just portions of what I ordinarily think of when I hear "block".
Comment on attachment 106919 [details] [diff] [review] patch v4 Is there a tab on this line: + nsComposeTxtSrvFilter.cpp \ In nsComposeTxtSrvFilter.cpp, is the copyright year correct (same question for nsComposeTxtSrvFilter.h, nsITextServicesFilter.idl, nsFilteredContentIterator.cpp, nsFilteredContentIterator.h): + * Portions created by the Initial Developer are Copyright (C) 1998 This block doesn't deal with null content; perhaps you intended to do the if check after the QI? + if (aNode) { + nsCOMPtr<nsIContent> content(do_QueryInterface(aNode)); + nsCOMPtr<nsIAtom> tag; + content->GetTag(*getter_AddRefs(tag)); spell out "nxt" in nsFilteredContentIterator.cpp: + nsCOMPtr<nsIDOMNode> nxtNode; I'm not sure about others, but I prefer when local variables are named things like "ignore" or "ignoreDidCross" such as here: + PRBool didCross; + CheckAdvNode(node, didCross, eBackward); In nsFilteredContentIterator.h, use PRPackedBool instead of PRBool? (Is there any chance any other booleans might be added some day?) I don't see any mac project file changes.
Attachment #106919 - Flags: superreview?(kin) → superreview-
doh! I forgot to mention for this block: + mBlockQuoteAtom = getter_AddRefs(NS_NewAtom("blockquote")); + mTypeAtom = getter_AddRefs(NS_NewAtom("type")); + mScriptAtom = getter_AddRefs(NS_NewAtom("script")); + mTextAreaAtom = getter_AddRefs(NS_NewAtom("textarea")); + mSelectAreaAtom = getter_AddRefs(NS_NewAtom("select")); + mMapAtom = getter_AddRefs(NS_NewAtom("mMapAtom")); I think we want to use do_getAtom() instead of NS_NewAtom
Attached patch patch with all reviewer's comments (obsolete) (deleted) — Splinter Review
Attachment #106919 - Attachment is obsolete: true
Comment on attachment 108025 [details] [diff] [review] patch with all reviewer's comments bringing joe's r= forward
Attachment #108025 - Flags: superreview?(kin)
Attachment #108025 - Flags: review?(jfrancis)
Rod, the latest patch does not have nsFilteredContentIterator.cpp, so I was unable to confirm that the fixes were in. Can you attach a complete patch?
Comment on attachment 108025 [details] [diff] [review] patch with all reviewer's comments r=brade with a few tweaks we discussed on AIM
Attachment #108025 - Flags: review?(jfrancis) → review+
Attached patch a final,final patch (obsolete) (deleted) — Splinter Review
Attachment #108025 - Attachment is obsolete: true
Attachment #108027 - Flags: superreview?(kin)
Attachment #108027 - Flags: review?(jfrancis)
Attached patch final patch (obsolete) (deleted) — Splinter Review
forgot the "-N" option
Attachment #108027 - Attachment is obsolete: true
Attachment #108028 - Flags: superreview?(kin)
Attachment #108028 - Flags: review?(jfrancis)
Rod, Next(), Prev(), and CurrentNode() still don't check mIsOutOfRange. Is this intentional?
Attached patch final patch? (deleted) — Splinter Review
Attachment #108028 - Attachment is obsolete: true
Attachment #108071 - Flags: superreview?(kin)
Attachment #108071 - Flags: review?(jfrancis)
Attachment #108025 - Flags: superreview?(kin) → superreview-
Attachment #108028 - Flags: superreview?(kin) → superreview-
Attachment #108027 - Flags: superreview?(kin) → superreview-
Comment on attachment 108071 [details] [diff] [review] final patch? You may want to make the mIsOutOfRange check in CurrentNode() casue an error return value, in order to sta consistent with the underlying iterator (which returns an error if it cannot find a current node).
Attachment #108071 - Flags: review?(jfrancis) → review+
Comment on attachment 108071 [details] [diff] [review] final patch? sr=kin@netscape.com ==== Fix the "usnig" and "ski;p" typos: + * This class implements a filter interface, that enables + * those usnig it to ski;p over certain nodes when traversing content ==== I would change "editor" to "composer" since this lives in the composer dll, but it's up to you: +// Generic for the editor +#define COMPOSER_TXTSRVFILTER_CONTRACTID "@mozilla.org/editor/txtsrvfilter;1" + +// This is the same but includes "cite" typed blocked quotes +#define COMPOSER_TXTSRVFILTERMAIL_CONTRACTID "@mozilla.org/editor/txtsrvfiltermail;1" ==== DO we need to check the return status of the |SelectNode()| call? + if (domRange && domNode) { + domRange->SelectNode(domNode); + } + + rv = mPreIterator->Init(domRange); ==== Get rid of the extra spaces in the arg list: + rv = nsTextServicesDocument::ComparePoints(aEndNode, aEndOffset, parentNode, indx, &endRes); ==== Shouldn't these be using the COMPOSER_TXTSRVFILTER_CONTRACTID and COMPOSER_TXTSRVFILTERMAIL_CONTRACTID defines? + filterContractId = "@mozilla.org/editor/txtsrvfiltermail;1"; + else + filterContractId = "@mozilla.org/editor/txtsrvfilter;1";
Attachment #108071 - Flags: superreview?(kin) → superreview+
Attachment #106919 - Flags: review?(jfrancis)
Attachment #108028 - Flags: review?(jfrancis)
Attachment #108027 - Flags: review?(jfrancis)
Requesting for inclusion in 1.3a
Flags: blocking1.3a?
Flags: blocking1.3a?
Comment on attachment 108071 [details] [diff] [review] final patch? I assume the "blocking1.3a" was intended as a request to land this patch (which should be done with the "approval1.3a" flag in the patch manager). Removed blocking request and a=asa for checkin to 1.3a.
Attachment #108071 - Flags: approval1.3a+
fixed
Status: ASSIGNED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
Note that Mac is still broken; see bug 184935.
Tried to read most comments, and I can't read code, so sorry if this was already addressed: Assuming this bug is to make spelllchecker skip quoted text ("> bla bla"), perhaps there should be some UI checkbox to let the user decide if he *does* want the quoted text to be checked (the default should be not to check quoted text). | | | Language: [x] Spellcheck quoted text | | [ English/US ] [Close ] | +--------------------------------------------+
Hi All, If this is fixed, is there somewhere we can download it from to give it a try out? Many thanks, --Tony aewell@bbis.com
> If this is fixed, is there somewhere we can download it from to give > it a try out? Download (or compile) the latest nightly builds and download (or compile) the spellchecker as described on http://spellchecker.mozdev.org/
Hi Aleksey, I downloaded http://mozillacafe.org/spellchecker_1.3.xpi from Spellchecker for Mozilla 1.3a and Nightly builds Windows install download (2002-12-15)Bug 2678 has been fixed. Linux x86 install download (2002-12-15) It still is not working. What did I miss? --Tony aewell@gbis.com
This fix landed *after* 1.3a. It also "broke" binary compatibility. So you'll need to find (or compile yourself) a binary that specifically targets post-1.3a nightlies.
this is working only when the user press the spell check button on the toolbar. It does not work when the spellchecker is automatically invoked while sending a message. I have a fix coming...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
reassign to myself...
Assignee: rods → ducarroz
Status: REOPENED → NEW
Attachment #109554 - Flags: review?(rods)
Also, if I correctly understand the implementation, this feature works only when composing message using an HTML compose window. More works is needed for plaintext compose, I'll open another bug...
Status: NEW → ASSIGNED
Flags: blocking1.3b?
Target Milestone: mozilla1.3alpha → mozilla1.3beta
Comment on attachment 109554 [details] [diff] [review] Propsed fix for remaining problem, v1 r=rods
Attachment #109554 - Flags: review?(rods) → review+
Attachment #109554 - Flags: superreview?(sspitzer)
Should i forget about comment #39?
Peter, your best change would be to file a new bug...
Comment on attachment 109554 [details] [diff] [review] Propsed fix for remaining problem, v1 sr=sspitzer
Attachment #109554 - Flags: superreview?(sspitzer) → superreview+
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
Depends on: 184858
Comment on attachment 109554 [details] [diff] [review] Propsed fix for remaining problem, v1 >+ var spellCheckMail = (window.document.firstChild.getAttribute("windowtype") == "msgcompose"); Actually editor.js has a function IsWebComposer() which might do what you want (except in reverse, i.e. var spellCheckMail = !IsWebComposer();).
Flags: blocking1.3b? → blocking1.3b-
Blocks: 186825
OK, i just filed bug 186825 to address the suggestion I made in comment #39.
JF: in comment #47 you said you would spin another bug for skipping spellchecking quoted portions in plain text compose messages. Could you post that bug number here please?
I haven't yet...
The changes in the latest patch makes the single 'Close' button into two buttons a 'Send' one and a 'Stop' one. Is there code attached to make the 'Send' button send the email yet? Logged bug 190734 about 'Send' button not working.
Last checkin caused another regression; spell checker does not appear if there are no misspelled words.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030210 Quoted text is still being spell checked if the original message contains either an email address or URL. Curiously, editing the quoted text in the new email to include either an email address or URL does not result in the quoted text getting spell checked, which is good. An email address or URL has to be present in the original email, prior to quoting it in the email being composed, in order for this spellchecker bug to show itself again. If neither email address nor URL are present, all works as advertised and the quoted text is not spell checked. Time to reopen the bug.
*** Bug 181391 has been marked as a duplicate of this bug. ***
*** Bug 198032 has been marked as a duplicate of this bug. ***
Re comment 55, I have logged bug 201866 for the plain text compose case.
Product: Browser → Seamonkey
I just tested 1.8b1 and this version doesn't skip the quotes. So I tested the oldest available nightly of 15th February and the newest nightly of 1st March. In both versions the spellchecker doesn't skip the quotes. Then I tested 1.8a6. This version does still skip the quotes. Therefore the bug mus came back between the release of 1.8a6 and 15th February. Can somebody verify this and reopen the bug?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: