Closed Bug 955484 Opened 10 years ago Closed 10 years ago

Escape key should close awesometab

Categories

(Instantbird Graveyard :: Other, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: nhnt11)

References

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 2047 at 2013-07-12 15:02:00 UTC ***

For consistency with conv tabs
Blocks: 955452
*** Original post on bio 2047 as attmnt 2585 at 2013-07-12 15:11:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354354 - Flags: review?(aleth)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Comment on attachment 8354354 [details] [diff] [review]
Escape key closes awesometab if filterbox is empty. Else, clear filterbox (default behavior)

*** Original change on bio 2047 attmnt 2585 at 2013-07-12 15:13:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354354 - Attachment is patch: true
Attachment #8354354 - Attachment mime type: application/octet-stream → text/plain
Attached patch Fix indentation (obsolete) — Splinter Review
*** Original post on bio 2047 as attmnt 2586 at 2013-07-12 15:14:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354355 - Flags: review?(aleth)
Comment on attachment 8354354 [details] [diff] [review]
Escape key closes awesometab if filterbox is empty. Else, clear filterbox (default behavior)

*** Original change on bio 2047 attmnt 2585 at 2013-07-12 15:14:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354354 - Attachment is obsolete: true
Attachment #8354354 - Flags: review?(aleth)
Comment on attachment 8354355 [details] [diff] [review]
Fix indentation

*** Original change on bio 2047 attmnt 2586 at 2013-07-12 15:25:33 UTC ***

I don't think <key>s correspond to keydown events, so you can save yourself the extra handler.
http://lxr.instantbird.org/instantbird/source/instantbird/content/instantbird.xul#83
Attachment #8354355 - Flags: review?(aleth) → review-
Attached patch Better solution (obsolete) — Splinter Review
*** Original post on bio 2047 as attmnt 2587 at 2013-07-12 16:06:00 UTC ***

This enables "escape to close" for all tabs.
In awesometabs, when there is text entered in the filterbox, pressing escape clears it and the command is not invoked.
Attachment #8354356 - Flags: review?(florian)
Comment on attachment 8354355 [details] [diff] [review]
Fix indentation

*** Original change on bio 2047 attmnt 2586 at 2013-07-12 16:06:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354355 - Attachment is obsolete: true
*** Original post on bio 2047 at 2013-07-12 16:32:38 UTC ***

(In reply to comment #4)
> Created attachment 8354356 [details] [diff] [review] (bio-attmnt 2587) [details]
> Better solution
> 
> This enables "escape to close" for all tabs.
> In awesometabs, when there is text entered in the filterbox, pressing escape
> clears it and the command is not invoked.

A.k.a. "this might lose data on any tab except for conversations which are put on hold"? ;)

I don't think we should do this in general.
Comment on attachment 8354356 [details] [diff] [review]
Better solution

*** Original change on bio 2047 attmnt 2587 at 2013-07-13 11:59:37 UTC ***

It's no good to change the command in this way and still call it cmd_putOnHold, as I mentioned on IRC.

But before you change this, it seems we need to discuss with everyone on IRC what the expected behaviour for ESC is, as there seem to be some differing opinions ;)
Attachment #8354356 - Flags: review?(florian) → review-
*** Original post on bio 2047 at 2013-07-13 22:00:36 UTC ***

(In reply to comment #6)

> But before you change this, it seems we need to discuss with everyone on IRC
> what the expected behaviour for ESC is, as there seem to be some differing
> opinions ;)

This seems like it would take us a while to reach complete agreement. Is there any reason why a solution along the lines of attachment 8354355 [details] [diff] [review] (bio-attmnt 2586) (after addressing comment 3) wouldn't be acceptable?
*** Original post on bio 2047 at 2013-07-15 09:01:32 UTC ***

(In reply to comment #7)
> (In reply to comment #6)
> 
> > But before you change this, it seems we need to discuss with everyone on IRC
> > what the expected behaviour for ESC is, as there seem to be some differing
> > opinions ;)
> 
> This seems like it would take us a while to reach complete agreement. Is there
> any reason why a solution along the lines of attachment 8354355 [details] [diff] [review] (bio-attmnt 2586) [details] (after addressing
> comment 3) wouldn't be acceptable?

It would be fine to do something along the lines of that attachment, but in the existing awesometab keyboard handlers. It's a bit odd though to add ESC here so it "works everywhere", but not to code it so it actually works everywhere (e.g. it will not close about:* tabs). I guess we can live with that for now, as we don't seem to have decided what "works" means in general here ;)
*** Original post on bio 2047 as attmnt 2599 at 2013-07-15 19:36:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354368 - Flags: review?(florian)
Comment on attachment 8354356 [details] [diff] [review]
Better solution

*** Original change on bio 2047 attmnt 2587 at 2013-07-15 19:36:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354356 - Attachment is obsolete: true
Comment on attachment 8354368 [details] [diff] [review]
Don't handle escape for all tabs. Use existing keydown handler in newtab.xml.

*** Original change on bio 2047 attmnt 2599 at 2013-07-15 20:22:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354368 - Flags: review?(florian) → review+
*** Original post on bio 2047 at 2013-07-15 21:27:12 UTC ***

http://hg.instantbird.org/instantbird/rev/d04931876b00
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.