Last Comment Bug 49474 - Filter editor UI: Enter key after entering criteria closes dialog.
: Filter editor UI: Enter key after entering criteria closes dialog.
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
Mentors:
: 40495 50971 52952 (view as bug list)
Depends on:
Blocks: 786436
  Show dependency treegraph
 
Reported: 2000-08-18 11:46 PDT by laurel
Modified: 2012-09-13 14:07 PDT (History)
11 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.65 KB, patch)
2012-08-27 12:33 PDT, :aceman
mconley: review+
bwinton: ui‑review+
rkent: feedback+
jh: feedback-
Details | Diff | Review
patch v2 (3.07 KB, patch)
2012-08-30 13:12 PDT, :aceman
jh: feedback+
Details | Diff | Review
patch v3 (2.98 KB, patch)
2012-09-04 12:49 PDT, :aceman
iann_bugzilla: review+
bwinton: ui‑review+
Details | Diff | Review

Description laurel 2000-08-18 11:46:53 PDT
Hitting Enter after entering search criteria closes the dialog. According to
spec (http://gooey/client/5.0/specs/mail/Search/Search.html) a new criteria line
should be created if you've entered criteria.  Otherwise I'd expect Enter to
initiate the search.  Would not expect the dialog to close.

(similar filter ui bug 40495).
Comment 1 Alec Flett 2000-08-18 14:27:41 PDT
putterman and I just noticed this yesterday. should be a one-line fix
Comment 2 laurel 2000-08-18 14:34:36 PDT
You know, if you're fixing this dialog's Enter, can you fix filters too?
Comment 3 Alec Flett 2000-08-18 14:40:25 PDT
sure, it's the same one liner in each file
Comment 4 lchiang 2000-08-21 18:47:24 PDT
+ per mail triage
Comment 5 selmer (gone) 2000-08-28 18:45:23 PDT
Elevating priority since we couldn't convince ourselves to cut this today.
Comment 6 lchiang 2000-08-29 13:46:39 PDT
second pass: - per mail triage (we convinced ourselves)
Comment 7 lchiang 2000-08-31 15:36:19 PDT
*** Bug 50971 has been marked as a duplicate of this bug. ***
Comment 8 laurel 2000-09-19 11:27:13 PDT
*** Bug 52952 has been marked as a duplicate of this bug. ***
Comment 9 selmer (gone) 2000-09-27 07:47:19 PDT
nom rtm since Phil used this as an example of stuff we should fix before rtm :-)

I think this is sort of a proxy for ALL the dialogs where enter dismisses while
focus is not on the OK button.  I'd sure like to see a general fix that catches
all dialogs.
Comment 10 selmer (gone) 2000-09-28 06:59:59 PDT
rtm-, not sufficiently nasty to hold rtm for.
Comment 11 Alec Flett 2000-11-07 15:18:51 PST
reassign all search/filter UI bugs to gayatrib, part 2
Comment 12 laurel 2000-12-19 11:39:43 PST
The search dialog doesn't have this problem anymore, but still exists in Filters.
Comment 13 laurel 2000-12-19 11:39:56 PST
*** Bug 40495 has been marked as a duplicate of this bug. ***
Comment 14 (not reading, please use seth@sspitzer.org instead) 2001-04-14 15:30:11 PDT
currently, hitting enter in a search term will initiate a search.

to change the behaviour, change onEnterInSearchTerm in SearchDialog.js
Comment 15 laurel 2001-04-16 13:43:40 PDT
Changed summary to reflect current issue is in Filter UI only.
Comment 16 scottputterman 2001-05-20 21:39:58 PDT
reassigning to naving
Comment 17 Henrik Gemal 2001-10-24 10:51:13 PDT
what is the bug about? Search or filter?
hitting Enter inthe search dialog starts searching...
Comment 18 laurel 2001-10-24 11:06:21 PDT
Read the summary or a couple comments back. It's about Filters UI.
Comment 19 (not reading, please use seth@sspitzer.org instead) 2003-05-08 09:55:27 PDT
mass re-assign.
Comment 20 Wayne Mery (:wsmwk, NI for questions) 2008-02-28 08:29:02 PST
invalid, given that filter UI was redesigned?
Comment 21 Serge Gautherie (:sgautherie) 2008-10-02 10:09:30 PDT
(In reply to comment #20)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20081002 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)

Bug still there for me:
1. "Tools > Message Filters... > New..."
2. Select a folder.
3. Type "dumbString" as the "Subject, contains" criteria.
4. Press "Enter".
4r. Dialog closes (and filter is created) :-(
Comment 22 Serge Gautherie (:sgautherie) 2008-10-11 07:38:48 PDT
(In reply to comment #21)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081011 Shredder/3.0b1pre] (nightly) (W2Ksp4)

Same behavior.
Comment 23 Wayne Mery (:wsmwk, NI for questions) 2010-03-26 05:03:07 PDT
the problem is more general.  with focus on any field other than "+" or "-", hitting enter closes the dialog
Comment 24 :aceman 2011-10-26 03:35:04 PDT
So it acts as OK.
Comment 25 :aceman 2012-08-27 06:42:51 PDT
I think I can do this.

In /mail/base/content/mailWidgets.xml:
      <handler event="keypress" keycode="VK_RETURN" action="onEnterInSearchTerm(event);" preventdefault="true"/>

In /mailnews/base/search/content/FilterEditor.js:
function onEnterInSearchTerm(event)
{
  onMore(event);
}

The SM version could be the same.
Comment 26 :aceman 2012-08-27 12:33:38 PDT
Created attachment 655706 [details] [diff] [review]
patch

This works for me. I have not tested the SM part.
I would like onMore to select the new added line and possibly to focus (move cursor) to the new textbox. But I don't know how to do that as there is a deck of elements.
Comment 27 Blake Winton (:bwinton) (:☕️) 2012-08-27 13:38:47 PDT
Comment on attachment 655706 [details] [diff] [review]
patch

I think you can be a little cleverer here…  ;)

I'ld like to see two more things to make this a perfect patch.
1) When I hit enter, focus the next field, so that I can keep on typing.
2) When I hit delete on an empty field, remove that line and focus the previous one.

Thanks,
Blake.
Comment 28 :aceman 2012-08-27 14:41:07 PDT
That would be nice but as I wrote that is hard. It seems a newly added rule does have its searchvalue.value set to null. For old rules (when a filter is edited) the .value is set and its .str property could be used (if it is a text field). I'd need help from rkent if something clever can be done here.
Comment 29 Kent James (:rkent) 2012-08-28 13:34:45 PDT
Comment on attachment 655706 [details] [diff] [review]
patch

I'm happy with the patch as is (although I did not test it, just looked at the code).

It seems to me that bwinton is expanding the scope of this beyond the original bug request. AFAICT none of the other actions in the filter dialog currently change focus when you act on them. While that may be a good overall design, coming up with a plan for focus management in this dialog seems to me not necessary to fix this small issue.
Comment 30 :aceman 2012-08-28 13:40:07 PDT
Thanks.
Bwinton?
Comment 31 Blake Winton (:bwinton) (:☕️) 2012-08-28 13:46:48 PDT
Yep, I totally am.  Unabashedly, too.  ;)

If you think that it's too hard, then I'll live without it, but please file a follow-up bug so that someone can fix it one day…
Comment 32 Blake Winton (:bwinton) (:☕️) 2012-08-28 14:10:56 PDT
Comment on attachment 655706 [details] [diff] [review]
patch

Given the followup bug to make things even better, I can live with this for now.  ui-r=me.
Comment 33 Mike Conley (:mconley) - (Needinfo me!) 2012-08-30 09:12:05 PDT
Comment on attachment 655706 [details] [diff] [review]
patch

Review of attachment 655706 [details] [diff] [review]:
-----------------------------------------------------------------

Thunderbird stuff looks good. Thanks aceman!
Comment 34 Jens Hatlak (:InvisibleSmiley) 2012-08-30 12:47:26 PDT
Comment on attachment 655706 [details] [diff] [review]
patch

I'm not an official SM MailNews reviewer (see http://www.seamonkey-project.org/dev/project-areas for who is; I'll delegate to IanN for now) but tested the patch on SM. Looks OK code-wise. The below is my opinion, but if everyone involved from the TB side wants this as-is nevertheless, all you need is a positive code review for the SM side.

Functionality-wise I don't really like the proposed behavior in its current, unfinished form. The current behavior is at least consistent in that it triggers the OK button (which then closes the dialog/window). The behavior implemented through the patch OTOH has two shortcomings:
1. The newly added text field is not focused. I understand this is to be done in a follow-up, but IMHO it should be done in one go. It's not really a separate issue (as opposed to the delete key discussion).
2. The OK button cannot be triggered anymore from one of the text fields since it doesn't even have an accesskey (until now it didn't need one). I'd expect Ctrl+Enter to trigger the OK button with the changed behavior. Again I feel this is not an issue that should be done in a follow-up but right here, right now.
Comment 35 :aceman 2012-08-30 13:12:23 PDT
Created attachment 657008 [details] [diff] [review]
patch v2

Yes, I'd agree with 2) and this is a new patch with that.
Point 1) is a new UI behaviour that we do not have so far and needs more thought that is why we have a separate bug for it.
Comment 36 Jens Hatlak (:InvisibleSmiley) 2012-08-30 13:40:16 PDT
Comment on attachment 657008 [details] [diff] [review]
patch v2

Review of attachment 657008 [details] [diff] [review]:
-----------------------------------------------------------------

f=me provided that issue 2 is addressed sooner than later (and with priority over the delete key discussion in case the latter becomes a lengthy one). Thanks!

::: mail/base/content/mailWidgets.xml
@@ +2187,5 @@
>      </implementation>
>      <handlers>
> +      <handler event="keypress" keycode="VK_RETURN"
> +               action="onEnterInSearchTerm(event);" preventdefault="true"/>
> +      <handler event="keypress" keycode="VK_RETURN" modifiers="control"

(Here and in the SM version of the file) I could imagine you want to use "accel" instead of "control". Better ask a Mac expert. ;-)
Comment 37 Mike Conley (:mconley) - (Needinfo me!) 2012-08-30 13:42:20 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #36)
> Comment on attachment 657008 [details] [diff] [review]
> (Here and in the SM version of the file) I could imagine you want to use
> "accel" instead of "control". Better ask a Mac expert. ;-)

Good catch! Unfortunately, the solution that generally seems to be used for this situation is more #ifdefs (http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#236).  *sigh*
Comment 38 Ian Neal 2012-09-03 16:29:22 PDT
(In reply to Mike Conley (:mconley) from comment #37)
> (In reply to Jens Hatlak (:InvisibleSmiley) from comment #36)
> > Comment on attachment 657008 [details] [diff] [review]
> > (Here and in the SM version of the file) I could imagine you want to use
> > "accel" instead of "control". Better ask a Mac expert. ;-)
> 
> Good catch! Unfortunately, the solution that generally seems to be used for
> this situation is more #ifdefs
> (http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
> messengercompose.xul#236).  *sigh*

Instead of having two <handler event="keypress" keycode="VK_RETURN"> lines, just have the one and then use javascript and event.*key to do the correct thing depending on platform?
so for non-Mac check for event.ctrlKey and then call this.document.getElementsByTagName('dialog')[0].acceptDialog(); otherwise onMore(event); etc
Comment 39 :aceman 2012-09-03 23:20:59 PDT
Cool idea!
Comment 40 :aceman 2012-09-03 23:22:39 PDT
But then, what is the JS test for Mac?
Services.appinfo.OS == "Darwin"
Comment 42 Ian Neal 2012-09-04 04:54:47 PDT
(In reply to :aceman from comment #40)
> But then, what is the JS test for Mac?
> Services.appinfo.OS == "Darwin"

Or what you said http://mxr.mozilla.org/comm-central/source/mail/base/modules/mailMigrator.js#146
Comment 43 :aceman 2012-09-04 05:11:02 PDT
No shortage of options. Let's flip a coin :-P
Comment 44 :aceman 2012-09-04 12:35:33 PDT
And what is the special key on Mac? Is it event.metaKey? That one is also triggered by Ctrl on Linux...
Comment 45 :aceman 2012-09-04 12:36:33 PDT
Oh, it is not.
Comment 46 :aceman 2012-09-04 12:49:09 PDT
Created attachment 658191 [details] [diff] [review]
patch v3

Please try this on Mac if the modifiers are right.
Comment 47 Ian Neal 2012-09-08 13:53:43 PDT
I think it would make more sense for the text input field on the new line to have focus rather than keeping it on the existing text input field.
Comment 48 :aceman 2012-09-10 00:07:13 PDT
Ian, that is now in bug 786436.
Comment 49 Blake Winton (:bwinton) (:☕️) 2012-09-13 07:33:17 PDT
Comment on attachment 658191 [details] [diff] [review]
patch v3

This fixes the listed problem…  ui-r=me.
Comment 50 Ryan VanderMeulen [:RyanVM] 2012-09-13 14:07:09 PDT
https://hg.mozilla.org/comm-central/rev/313e5a6f520b

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