101153 - Search messages window has no intrinsic way of closing itself.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Build: linux 2001-09-19-08 nightly.

Steps to reproduce:

1)  Open mailnews
2)  Select Search > Search Messages
3)  Try to close the resulting search window

Expected results:  A button, a menu, or a keyboard shortcut that will close the
                   window.

Actual results: have to use the windowmanager's window decorations or menus to
                close the window.

Comment 1

[Boris Zbarsky [:bzbarsky]]

mpt sez no menu/button, but add a ctrl-w shortcut.  Over to mailwindow front end.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to add a keyboard shortcut

Comment 3

[Boris Zbarsky [:bzbarsky]]

reviews?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 50469 [details] [diff] [review]
Patch to add a keyboard shortcut

Working on a better approach (focus first textfield instead of vbox)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Better patch (I'm starting to like xbl... :) )

Comment 6

[Håkan Waara]

Comment on attachment 50490 [details] [diff] [review]
Better patch (I'm starting to like xbl... :) )

This code looks good.

r=hwaara

Comment 7

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

some comments:

how do these changes affect the edit filter dialog?  looking at them, it looks
like it would put focus in the first term (for a new filter).  currently, it
looks like for a new filter (and an existing filter) we have focus in the filter
name area.

adding the call to searchVal.focus() in createSearchRow is bad for editing
existing filters, since we'll update focus on every row created, right?  where
does that leave the focus when we're done?

does hitting ctrl w do the right thing if a search is in progress?  make sure 
calling close() properly calls our onclose handler which stops the search.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 50490 [details] [diff] [review]
Better patch (I'm starting to like xbl... :) )

Seth, good points.  The edit filter
dialog would be adversely affected by this.

And onclose is _not_ called.

In view of that, and the fact
that focusing the textfield makes hitting
ctrl-w a pain on Linux anyway, I'm going to
attach a simpler patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: A more correct fix that avoids all the focus problems.

Comment 11

[Håkan Waara]

If you are just going to focus the outermost box, I don't see a reason to do it
manually at all.

Can't you just skip the .focus() call and do only the ctrl+w thing?

Comment 12

[Boris Zbarsky [:bzbarsky]]

No, because then nothing has focus as the dialog comes up.  At that point
closing the dialog requires clicking on something first (or hitting tab then
ctrl-w, maybe; I have not tested that).  This seems poor from an accessibility
keyboard-only-UI-access standpoint.

Oh, and clicking on the main XUL window does _not_ focus it (a feature
apparently).  So you have to know to click on a form control if you're clicking.

Comment 13

[Håkan Waara]

Isn't window.close() supposed to notify the onclose event handler?

Doesn't seem like something that is supposed to be done manually.

Comment 14

[Boris Zbarsky [:bzbarsky]]

According to hyatt, no.

window.close() will call onunload().

Clicking the windowmanager's [x] will call onclose() and then onunload()

Comment 15

[Håkan Waara]

May I ask why we have an onclose() at all if it's bogus (i.e., doesn't trigger
on window closure)?  This could be something serious we need to fix.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hakan, onclose() triggers if the window is closed externally (ie by the
windowmanager).  I believe the assumption is that if we're closing the window
from inside the window itself we will handle closure properly.

The point of onclose() is that if it returns "false" the window is _not_ closed.
 In other words, onclose() is where you put things like confirmation dialogs
that allow the user to "cancel" closing a window that has unsaved content.

Or so I understand from talking to hyatt.

Comment 17

[Boris Zbarsky [:bzbarsky]]

This is an accessibility issue..

Comment 18

[laurel]

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

Comment 19

[laurel]

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

Comment 20

[sairuh (rarely reading bugmail)]

nominating for aviary1.0, and giving to Scott (punt as needed) to see if this
could be fixed for Thunderbird. on tbird this would be applicable to the Search
Messages and Search Addresses dialogs.

on a related tangent, see also bug 183419 --which requests adding a Close button.

Comment 21

[Scott MacGregor]

Attachment: updated fix

I made Boris's patch also work for the address book search dialog and the
filter list dialog. I had already hooked up escape to close all of these
dialogs. This patch just adds ctrl-W support.

I didn't seem to need the JS that set focus within the box anymore. It worked
fine without it. I wonder if something changed over the last couple years that
made that not necessary :)