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

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[laurel]

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]

putterman and I just noticed this yesterday. should be a one-line fix

Comment 2

[laurel]

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

Comment 3

[Alec Flett]

sure, it's the same one liner in each file

Comment 4

[lchiang]

+ per mail triage

Comment 5

[selmer (gone)]

Elevating priority since we couldn't convince ourselves to cut this today.

Comment 6

[lchiang]

second pass: - per mail triage (we convinced ourselves)

Comment 7

[lchiang]

*** Bug 50971 has been marked as a duplicate of this bug. ***

Comment 8

[laurel]

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

Comment 9

[selmer (gone)]

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)]

rtm-, not sufficiently nasty to hold rtm for.

Comment 11

[Alec Flett]

reassign all search/filter UI bugs to gayatrib, part 2

Comment 12

[laurel]

The search dialog doesn't have this problem anymore, but still exists in Filters.

Comment 13

[laurel]

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

Comment 14

[(not reading, please use [e-mail] instead)]

currently, hitting enter in a search term will initiate a search.

to change the behaviour, change onEnterInSearchTerm in SearchDialog.js

Comment 15

[laurel]

Changed summary to reflect current issue is in Filter UI only.

Comment 16

[scottputterman]

reassigning to naving

Comment 17

[Henrik Gemal]

what is the bug about? Search or filter?
hitting Enter inthe search dialog starts searching...

Comment 18

[laurel]

Read the summary or a couple comments back. It's about Filters UI.

Comment 19

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 20

[Wayne Mery (:wsmwk)]

invalid, given that filter UI was redesigned?

Comment 21

[Serge Gautherie (:sgautherie)]

(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)]

(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)]

the problem is more general.  with focus on any field other than "+" or "-", hitting enter closes the dialog

Comment 24

[:aceman]

So it acts as OK.

Comment 25

[:aceman]

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]

Attachment: 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) (:☕️)]

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]

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)]

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]

Thanks.
Bwinton?

Comment 31

[Blake Winton (: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…

Comment 32

[Blake Winton (:bwinton) (:☕️)]

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) (:⚙️)]

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)]

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]

Attachment: 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)]

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) (:⚙️)]

(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]

(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]

Cool idea!

Comment 40

[:aceman]

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

Comment 41

[Ian Neal]

(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

[Ian Neal]

(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]

No shortage of options. Let's flip a coin :-P

Comment 44

[:aceman]

And what is the special key on Mac? Is it event.metaKey? That one is also triggered by Ctrl on Linux...

Comment 45

[:aceman]

Oh, it is not.

Comment 46

[:aceman]

Attachment: patch v3

Please try this on Mac if the modifiers are right.

Comment 47

[Ian Neal]

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]

Ian, that is now in bug 786436.

Comment 49

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 658191 [details] [diff] [review]
patch v3

This fixes the listed problem…  ui-r=me.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/313e5a6f520b