Filter editor UI: Enter key after entering criteria closes dialog.

RESOLVED FIXED in Thunderbird 18.0

Status

MailNews Core
Filters
--
minor
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: laurel, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 18.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

2.98 KB, patch
Ian Neal
: review+
bwinton
: ui-review+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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).
(Reporter)

Updated

17 years ago
QA Contact: lchiang → laurel

Comment 1

17 years ago
putterman and I just noticed this yesterday. should be a one-line fix
Status: NEW → ASSIGNED
Keywords: nsbeta3
(Reporter)

Comment 2

17 years ago
You know, if you're fixing this dialog's Enter, can you fix filters too?

Comment 3

17 years ago
sure, it's the same one liner in each file
Summary: Search UI: Enter after entering criteria closes dialog. → Search/Filters UI: Enter after entering criteria closes dialog.

Comment 4

17 years ago
+ per mail triage
Whiteboard: [nsbeta3+]
Target Milestone: --- → M18

Comment 5

17 years ago
Elevating priority since we couldn't convince ourselves to cut this today.
Priority: P3 → P2

Comment 6

17 years ago
second pass: - per mail triage (we convinced ourselves)
Whiteboard: [nsbeta3+] → [nsbeta3-][cut 8/29]

Comment 7

17 years ago
*** Bug 50971 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 8

17 years ago
*** Bug 52952 has been marked as a duplicate of this bug. ***

Comment 9

17 years ago
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.
Keywords: rtm

Comment 10

17 years ago
rtm-, not sufficiently nasty to hold rtm for.
Whiteboard: [nsbeta3-][cut 8/29] → [nsbeta3-][rtm-]

Comment 11

17 years ago
reassign all search/filter UI bugs to gayatrib, part 2
Assignee: alecf → gayatrib
Status: ASSIGNED → NEW
(Reporter)

Comment 12

17 years ago
The search dialog doesn't have this problem anymore, but still exists in Filters.
(Reporter)

Comment 13

17 years ago
*** Bug 40495 has been marked as a duplicate of this bug. ***

Updated

17 years ago
Target Milestone: M18 → ---
currently, hitting enter in a search term will initiate a search.

to change the behaviour, change onEnterInSearchTerm in SearchDialog.js
(Reporter)

Comment 15

17 years ago
Changed summary to reflect current issue is in Filter UI only.
Summary: Search/Filters UI: Enter after entering criteria closes dialog. → Filters UI: Enter after entering criteria closes dialog.

Comment 16

17 years ago
reassigning to naving
Assignee: gayatrib → naving

Comment 17

16 years ago
what is the bug about? Search or filter?
hitting Enter inthe search dialog starts searching...
(Reporter)

Comment 18

16 years ago
Read the summary or a couple comments back. It's about Filters UI.
mass re-assign.
Assignee: naving → sspitzer
Product: Browser → Seamonkey

Updated

13 years ago
Assignee: sspitzer → mail
invalid, given that filter UI was redesigned?
Severity: normal → minor
QA Contact: laurel → search

Updated

9 years ago
Component: MailNews: Search → MailNews: Message Display
(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) :-(
Assignee: mail → nobody
Priority: P2 → --
QA Contact: search → message-display
Whiteboard: [nsbeta3-][rtm-] → [See comment 12]
(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.
Component: MailNews: Message Display → Filters
Product: SeaMonkey → MailNews Core
QA Contact: message-display → filters
the problem is more general.  with focus on any field other than "+" or "-", hitting enter closes the dialog
Summary: Filters UI: Enter after entering criteria closes dialog. → Filters UI: Enter key after entering criteria closes dialog.
Whiteboard: [See comment 12]
(Assignee)

Comment 24

6 years ago
So it acts as OK.
(Assignee)

Comment 25

5 years ago
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.
Assignee: nobody → acelists
Summary: Filters UI: Enter key after entering criteria closes dialog. → Filter editor UI: Enter key after entering criteria closes dialog.
(Assignee)

Comment 26

5 years ago
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.
Attachment #655706 - Flags: ui-review?(bwinton)
Attachment #655706 - Flags: review?(jh)
Attachment #655706 - Flags: feedback?(kent)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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.
Attachment #655706 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 28

5 years ago
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

5 years ago
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.
Attachment #655706 - Flags: feedback?(kent) → feedback+
(Assignee)

Comment 30

5 years ago
Thanks.
Bwinton?
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…
(Assignee)

Updated

5 years ago
Blocks: 786436
(Assignee)

Updated

5 years ago
Attachment #655706 - Flags: review?(mconley)
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.
Attachment #655706 - Flags: ui-review- → ui-review+
Comment on attachment 655706 [details] [diff] [review]
patch

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

Thunderbird stuff looks good. Thanks aceman!
Attachment #655706 - Flags: review?(mconley) → review+
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.
Attachment #655706 - Flags: review?(jh)
Attachment #655706 - Flags: review?(iann_bugzilla)
Attachment #655706 - Flags: feedback-
(Assignee)

Comment 35

5 years ago
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.
Attachment #655706 - Attachment is obsolete: true
Attachment #655706 - Flags: review?(iann_bugzilla)
Attachment #657008 - Flags: ui-review?(bwinton)
Attachment #657008 - Flags: review?(iann_bugzilla)
Attachment #657008 - Flags: feedback?(jh)
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. ;-)
Attachment #657008 - Flags: feedback?(jh) → feedback+
(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

5 years ago
(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
(Assignee)

Comment 39

5 years ago
Cool idea!
(Assignee)

Comment 40

5 years ago
But then, what is the JS test for Mac?
Services.appinfo.OS == "Darwin"

Comment 41

5 years ago
(In reply to :aceman from comment #40)
> But then, what is the JS test for Mac?
> Services.appinfo.OS == "Darwin"

http://mxr.mozilla.org/comm-central/source/mail/base/content/utilityOverlay.js#128

or

http://mxr.mozilla.org/comm-central/source/mail/base/content/nsContextMenu.js#325

or

http://mxr.mozilla.org/comm-central/source/mail/components/test/unit/test_about_support.js#138

Comment 42

5 years ago
(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
(Assignee)

Comment 43

5 years ago
No shortage of options. Let's flip a coin :-P
(Assignee)

Comment 44

5 years ago
And what is the special key on Mac? Is it event.metaKey? That one is also triggered by Ctrl on Linux...
(Assignee)

Comment 45

5 years ago
Oh, it is not.
(Assignee)

Comment 46

5 years ago
Created attachment 658191 [details] [diff] [review]
patch v3

Please try this on Mac if the modifiers are right.
Attachment #657008 - Attachment is obsolete: true
Attachment #657008 - Flags: ui-review?(bwinton)
Attachment #657008 - Flags: review?(iann_bugzilla)
Attachment #658191 - Flags: ui-review?(bwinton)
Attachment #658191 - Flags: review?(iann_bugzilla)

Comment 47

5 years ago
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.
(Assignee)

Comment 48

5 years ago
Ian, that is now in bug 786436.

Updated

5 years ago
Attachment #658191 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 658191 [details] [diff] [review]
patch v3

This fixes the listed problem…  ui-r=me.
Attachment #658191 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/313e5a6f520b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.