Last Comment Bug 543160 - Search Messages dialog closes when Enter is hit, rather than starting search
: Search Messages dialog closes when Enter is hit, rather than starting search
Status: VERIFIED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks: 515228
  Show dependency treegraph
 
Reported: 2010-01-29 20:54 PST by rsx11m
Modified: 2010-05-04 06:48 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix error and problem (2.27 KB, patch)
2010-04-02 07:54 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (2.28 KB, patch)
2010-04-02 14:15 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
mnyromyr: superreview+
Details | Diff | Splinter Review
patch v3 [Checkin: comment 9] (1.04 KB, patch)
2010-05-03 15:32 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
jh: superreview+
Details | Diff | Splinter Review

Description rsx11m 2010-01-29 20:54:30 PST
I'm usually concluding a search term with the Enter key to get the search started, to save a mouse click. This works in SM 2.0.2 but not in the 2.1a1pre nightlies, where the search dialog is just aborted. This happens on both IMAP and Local Folders, but the error console only seems to report an error on an IMAP folder search, and only if it is currently open. [Mozilla/5.0 (Windows;
U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20100129 SeaMonkey/2.1a1pre]
 
Steps to reproduce:
1) Right-click on a folder and select "Search Messages";
2) Enter any search string and finish with the Enter key;
3) Search window unexpectedly closes.

Expected behavior: Should start the search.

Error Console says (not always): Error: document is null
Source File: chrome://messenger/content/mailWindow.js
Line: 364
Comment 1 rsx11m 2010-02-01 13:52:42 PST
Identical symptoms for Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a1pre) Gecko/20100201 SeaMonkey/2.1a1pre, using a fresh profile.
Comment 2 Hartmut Figge 2010-04-02 05:08:06 PDT
Same bug on Mozilla/5.0 (X11; U; Linux i686; en; rv:1.9.3a4pre) Gecko/20100402 Mnenhy/0.8.0pre15 SeaMonkey/2.1a1pre.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2010-04-02 07:54:31 PDT
Created attachment 436682 [details] [diff] [review]
fix error and problem

The actual problem was caused by bug 515228 (conversion from window to dialog).
The document is null error is unrelated but let's fix it while we're here.
Comment 4 Hartmut Figge 2010-04-02 08:19:58 PDT
(In reply to comment #3)

> Created an attachment (id=436682) [details]

WFM
Comment 5 neil@parkwaycc.co.uk 2010-04-02 09:35:33 PDT
Comment on attachment 436682 [details] [diff] [review]
fix error and problem

>+      if (!document) return;
This is just wallpaper, and it's unclear that it's necessary once the underlying bug is fixed.

>+        defaultButton="search-button"
defaultButton is a btntype, not an id.
Comment 6 Jens Hatlak (:InvisibleSmiley) 2010-04-02 14:15:36 PDT
Created attachment 436769 [details] [diff] [review]
patch v2

(In reply to comment #5)
> (From update of attachment 436682 [details] [diff] [review])
> >+      if (!document) return;
> This is just wallpaper, and it's unclear that it's necessary once the
> underlying bug is fixed.

Well, it won't hurt either, right?

> >+        defaultButton="search-button"
> defaultButton is a btntype, not an id.

Hmm, right (dlgtype, actually). I'm a bit confused that it still worked in a way but I agree that my approach was wrong. The new one should be cleaner since it disables the window.close() code path in dialog.xml's _doButtonCommand method that triggered the problem (_fireButtonEvent calls onbuttonaccept).
Comment 7 Karsten Düsterloh 2010-05-03 14:54:08 PDT
Comment on attachment 436769 [details] [diff] [review]
patch v2

>diff --git a/suite/mailnews/mailWindow.js b/suite/mailnews/mailWindow.js
>   ensureStatusFields : function()
>     {
>-      if (!this.statusTextFld ) this.statusTextFld = document.getElementById("statusText");
>+      if (!document) return;

This is (a) unneeded with your fix below and (b) not really a fix anyway, just like Neil already said - please drop it.
No need to touch the whitespace in that vicinity then either.

I do get another warning, though, when searching (IMAP):
  JavaScript strict warning: chrome://messenger/content/mailWindow.js, line 487: reference to undefined property window.MsgStatusFeedback
=> Seems to be old, filed bug 563484 on that.

>diff --git a/suite/mailnews/search/SearchDialog.xul b/suite/mailnews/search/SearchDialog.xul
>         ondialoghelp="return openHelp('search_messages');"
>+        ondialogaccept="return false;"

Maybe it's worth having a short inline comment here, like /* allow Search on Enter */.

r/moa=me with that.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2010-05-03 15:32:52 PDT
Created attachment 443215 [details] [diff] [review]
patch v3 [Checkin: comment 9]
Comment 9 Jens Hatlak (:InvisibleSmiley) 2010-05-03 15:35:23 PDT
Comment on attachment 443215 [details] [diff] [review]
patch v3 [Checkin: comment 9]

http://hg.mozilla.org/comm-central/rev/ed24c9dfc66b
Comment 10 rsx11m 2010-05-04 06:48:32 PDT
Verified fixed Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100504 SeaMonkey/2.1a1pre nightly build, thanks.

Note You need to log in before you can comment on or make changes to this bug.