Recent shortcut change eschews cursor shortcuts to moves tabs instead

RESOLVED FIXED in 1.3

Status

Instantbird
Conversation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Instantbot, Assigned: aleth)

Tracking

Details

(Whiteboard: [1.3-blocking][regression])

Attachments

(1 attachment, 1 obsolete attachment)

636 bytes, patch
Mic
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
*** Original post on bio 1746 by spiffytech AT gmail.com at 2012-10-29 21:25:00 UTC ***

A recent (probably in the last week) nightly makes Cmd-<arrow> on my Mac move conversation window tabs, instead of the OS default of moving the cursor to the beginning/end of the line or textbox. Please revert this, as it makes editing my messages quite a pain.
(Assignee)

Comment 1

4 years ago
*** Original post on bio 1746 at 2012-10-29 21:27:52 UTC ***

I can confirm this happens while editing in the input box for Linux too (Ctrl-left/right).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Other → All
Hardware: x86 → All
Blocks: 953924
(Assignee)

Comment 2

4 years ago
*** Original post on bio 1746 at 2012-10-29 21:29:48 UTC ***

The checkin that broke this was http://hg.instantbird.org/instantbird/rev/778e140443c5
Whiteboard: [regression]
(Assignee)

Comment 3

4 years ago
*** Original post on bio 1746 at 2012-10-29 21:39:31 UTC ***

We should restrict this shortcut to when the tabbar has focus.
(Assignee)

Comment 4

4 years ago
Created attachment 8353782 [details] [diff] [review]
Patch

*** Original post on bio 1746 as attmnt 2022 at 2012-10-30 20:31:00 UTC ***

Simple fix.
Attachment #8353782 - Flags: review?(benediktp)
*** Original post on bio 1746 at 2012-10-31 11:59:51 UTC ***

Comment on attachment 8353782 [details] [diff] [review] (bio-attmnt 2022)
Patch

>diff --git a/chrome/instantbird/content/instantbird/tabbrowser.xml b/chrome/instantbird/content/instantbird/tabbrowser.xml
>index 8320299..7792f78 100644
>--- a/chrome/instantbird/content/instantbird/tabbrowser.xml
>+++ b/chrome/instantbird/content/instantbird/tabbrowser.xml
>@@ -1279,7 +1279,8 @@
>               aEvent.preventDefault();
>               return;
>             }
>-            if (aEvent.target.localName == "tabconversation") {
>+            if (aEvent.target.localName == "tabconversation" &&
>+                aEvent.originalTarget.localName == "tab") {

The originalTarget-part is sufficient as far as I can tell.
I tried it on its own and the keyboard shortcuts worked fine when a tab was focused (tab moved) or the input box was focused (jumped from word to word). It did nothing when the conversation browser was focused as it currently does (not).
If there's no other reason why we should check the target's localname, I'd say we only take the originalTarget-part.
(Assignee)

Comment 6

4 years ago
*** Original post on bio 1746 at 2012-10-31 16:17:00 UTC ***

(In reply to comment #5)
> The originalTarget-part is sufficient as far as I can tell.
...
> If there's no other reason why we should check the target's localname, I'd say
> we only take the originalTarget-part.
I can see why it would work without, but I don't like removing something if I don't know why it was there. Why did you check for "tabconversation" at that place?
(Assignee)

Comment 7

4 years ago
Created attachment 8353786 [details] [diff] [review]
Patch

*** Original post on bio 1746 as attmnt 2026 at 2012-10-31 22:01:00 UTC ***

So, looking at this again, I also fail to see a way aEvent.target can be != "tabconversation", so I removed it as requested. Still wondering why it was there in the first place though.
Attachment #8353786 - Flags: review?(benediktp)
(Assignee)

Comment 8

4 years ago
Comment on attachment 8353782 [details] [diff] [review]
Patch

*** Original change on bio 1746 attmnt 2022 at 2012-10-31 22:01:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353782 - Attachment is obsolete: true
Attachment #8353782 - Flags: review?(benediktp)
(Assignee)

Updated

4 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Whiteboard: [regression] → [1.3-blocking][regression]
Comment on attachment 8353786 [details] [diff] [review]
Patch

*** Original change on bio 1746 attmnt 2026 at 2012-11-02 12:50:45 UTC ***

That's exactly what I tried and it worked fine.
Attachment #8353786 - Flags: review?(benediktp) → review+
Whiteboard: [1.3-blocking][regression] → [1.3-blocking][regression][checkin-needed]
*** Original post on bio 1746 at 2012-11-03 04:22:05 UTC ***

Thanks for fixing this so quickly! (And thanks for reporting this!)

http://hg.instantbird.org/instantbird/rev/dcab10510f48
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [1.3-blocking][regression][checkin-needed] → [1.3-blocking][regression]
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.