Closed Bug 953542 Opened 10 years ago Closed 10 years ago

When the user types, focus should be given to message area

Categories

(Instantbird Graveyard :: Conversation, enhancement)

0.1.2
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: romain, Assigned: romain)

Details

Attachments

(2 files, 3 obsolete files)

*** Original post on bio 91 at 2008-08-11 02:01:00 UTC ***

When the user begins to type in the Conversation Window, the focus should change and automatically be given to the message "textearera".
Attached patch patch (obsolete) — Splinter Review
*** Original post on bio 91 as attmnt 53 at 2008-08-13 03:09:00 UTC ***

I added something to allow Ctrl + return and Alt + return to have the good behavior even if the cursor is not at end of the textbox.

The code is pretty long, maybe a bit dirty, we have to find an other way to implement that in the future.
Attachment #8351797 - Flags: review?
Assignee: florian → romain
Status: NEW → ASSIGNED
Comment on attachment 8351797 [details] [diff] [review]
patch

*** Original change on bio 91 attmnt 53 at 2008-08-13 04:54:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351797 - Attachment is obsolete: true
Attachment #8351797 - Flags: review?
Attached patch patch V2 (obsolete) — Splinter Review
*** Original post on bio 91 as attmnt 55 at 2008-08-13 05:29:00 UTC ***

same notes as above, this version should be better for extensions
Attachment #8351799 - Flags: review?(florian)
Comment on attachment 8351799 [details] [diff] [review]
patch V2

*** Original change on bio 91 attmnt 55 at 2008-08-13 15:37:31 UTC ***

That's A LOT better than the previous version.

>         if (event.keyCode != 13)
>           return;
> 
>-        /* this method is called by an eventListener.
>-           This points to the textbox element. */
>-        var conv = this;
>-        while (conv.localName != "conversation")
>-          conv = conv.parentNode;
>+        /* "this" points to the textbox (addEventListener)
>+            or to conv if called by browserKeyPress */
>+        if (this.localName == "conversation")
>+          var conv = this;
>+        else
>+          var conv = document.getBindingParent(this);

Would it be possible to remove the test and the use this last line?
Then the comment would be something like: "this" can point to the textbox when this method is used by an eventListener, get the conversation element.

>@@ -271,9 +276,10 @@
> 
>         /* this method is called by an eventListener.
>            This points to the textbox element. */
>-        var conv = this;
>-        while (conv.localName != "conversation")
>-          conv = conv.parentNode;
>+        if (this.localName == "conversation")
>+          var conv = this;
>+        else
>+          var conv = document.getBindingParent(this);

Same comment here.


>+          if (!isHtmlMode) {
>+            if (editor.selectionStart > 0 && editor.selectionStart == editor.selectionEnd)
You can split this long line after &&.

>+              editor.selectionStart--;

When the result is the same, I prefer --foo rather than foo-- (same for ++ of course, a few lines further).


>+        // CharCode=0 means not a character
>+        if (event.charCode == 0)
>+          return;
>+
>+        editor.focus();
>+
>+        // Returns for Ctrl+V
>+        if (event.ctrlKey)
>+          return;

It seems that you can move up this code, and then you can remove 3 "editor.focus()" lines.



>+     <method name="addString">
>+       <parameter name="aString"/>
>+       <body>
>+       <![CDATA[
>+         var editor = this.editor;
>+         var isHtmlMode = this.isHtmlMode;
>+         var length = (aString != "")
>+                      ? aString.length
>+                      : 0;
>+         var cursorPosition = editor.selectionStart + length;
>+
>+         editor.value = editor.value.substr(0, editor.selectionStart) + aString +
>+                        editor.value.substr(editor.selectionEnd);
>+         editor.selectionStart = editor.selectionEnd = cursorPosition;
>+       ]]>
>+       </body>
>+     </method>

I would welcome a comment above this method, explaining what it does.
For example: replace the current selection in the editor by the given string


Looks good otherwise.
Attachment #8351799 - Flags: review?(florian) → review-
Attached patch patch V3 (obsolete) — Splinter Review
*** Original post on bio 91 as attmnt 56 at 2008-08-13 16:18:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351800 - Flags: review?(florian)
Comment on attachment 8351799 [details] [diff] [review]
patch V2

*** Original change on bio 91 attmnt 55 at 2008-08-13 16:18:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351799 - Attachment is obsolete: true
*** Original post on bio 91 as attmnt 57 at 2008-08-13 16:24:00 UTC ***

sorry, I did not send the right file :/
Attachment #8351801 - Flags: review?
Comment on attachment 8351800 [details] [diff] [review]
patch V3

*** Original change on bio 91 attmnt 56 at 2008-08-13 16:24:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351800 - Attachment is obsolete: true
Attachment #8351800 - Flags: review?(florian)
Comment on attachment 8351801 [details] [diff] [review]
Patch V3 (real ...)

*** Original change on bio 91 attmnt 57 at 2008-08-13 16:38:51 UTC ***

r=me
Attachment #8351801 - Flags: review? → review+
*** Original post on bio 91 at 2008-08-13 21:51:59 UTC ***

Sending        instantbird/base/content/instantbird/conversation.xml
Transmitting file data .
Committed revision 272.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
*** Original post on bio 91 as attmnt 83 at 2008-08-27 21:29:00 UTC ***

On Mac OS X the accel key is meta, not ctrl.
*** Original post on bio 91 at 2008-08-27 21:32:53 UTC ***

Comment on attachment 8351827 [details] [diff] [review] (bio-attmnt 83)
Follow up to fix it on Mac

Pushed as 300:dfcee3e73d48.
Target Milestone: --- → 0.1.3
You need to log in before you can comment on or make changes to this bug.