Closed Bug 953681 Opened 7 years ago Closed 7 years ago

Make the textbox auto-expandable

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: romain, Assigned: romain)

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 235 at 2009-09-04 22:13:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch V3 (obsolete) — Splinter Review
*** Original post on bio 235 as attmnt 235 at 2009-09-04 22:13:00 UTC ***

On the conversation window, it would be nice if the textbox (input area) could
be resized depending of the content (like on Gmail, Gtalk, Adium).

- There can be a preference to disable that behavior
- we have to make sure that when the splitter moves, the height of the box is
kept as "default height"
- the default height on a new window / conversation should reflect the window
size so that on a small window there is a one line text box, and on a big one
several ones
- make sure not to break the status bar by resizing the textbox
- make sure that the vertical scrollbar doesn't flicker.
Attachment #8351979 - Flags: review?(florian)
Attached patch Patch V4 (obsolete) — Splinter Review
*** Original post on bio 235 as attmnt 237 at 2009-09-05 18:56:00 UTC ***

- Handle underflow event to avoid flickers on the scrollbar in some "rare" cases
- Using now DOMContentLoaded, a bit faster than conversation-loaded (but the resizing is still visible during a short period)
- Now behaves correctly when the conversation window is resized
Attachment #8351981 - Flags: review?(florian)
Comment on attachment 8351979 [details] [diff] [review]
Patch V3

*** Original change on bio 235 attmnt 235 at 2009-09-05 18:56:53 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351979 - Attachment is obsolete: true
Attachment #8351979 - Flags: review?(florian)
*** Original post on bio 235 at 2009-09-06 20:36:04 UTC ***

I tested this patch:
 - I like this a lot :)
 - The jump of the splitter when the conversation is opened is still visible.
 - The splitter jumps around while resizing the window. We should either base the sizing algorithm on the size of the window instead of the size of the browser, or add a threshold.
Attached patch Patch V5 (!) (obsolete) — Splinter Review
*** Original post on bio 235 as attmnt 238 at 2009-09-07 00:42:00 UTC ***

> - The jump of the splitter when the conversation is opened is still visible.
Now only visible when the window does not exist (instant on tabs).

>  - The splitter jumps around while resizing the window. We should either base the sizing algorithm on the size of the window instead of the size of the browser, or add a threshold.
Now calculated from the window.outerHeight (update from calculated from browser).
Attachment #8351982 - Flags: review?(florian)
Comment on attachment 8351981 [details] [diff] [review]
Patch V4

*** Original change on bio 235 attmnt 237 at 2009-09-07 00:42:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351981 - Attachment is obsolete: true
Attachment #8351981 - Flags: review?(florian)
*** Original post on bio 235 at 2009-09-07 10:07:45 UTC ***

(In reply to comment #3)

> >  - The splitter jumps around while resizing the window. We should either base the sizing algorithm on the size of the window instead of the size of the browser, or add a threshold.
> Now calculated from the window.outerHeight (update from calculated from
> browser).

Actually, I think it would be better if possible to base the computation on the size of the conversation binding, so that the textbox still behaves correctly if used in another window (created by extension authors maybe) that have other content around the binding.

Also, it seems a constant is not right in the sizing algorithm, the height of the textbox goes down to one line of text after the statusbar has already been cropped.

By the way, could you please change the min-height of .conv-top to 115px so that it is the same height that the grippy of the splitter of the nicklist, and doesn't look broken on Windows?
Attached patch Patch V6 (!!) (obsolete) — Splinter Review
*** Original post on bio 235 as attmnt 239 at 2009-09-07 17:48:00 UTC ***

Changes:
- All the stuff discussed above
- Fit the new folders hierarchy
- Fixing a bug that made the textbox in the first tab slightly bigger than the ones in the other tabs.
Attachment #8351983 - Flags: review?(florian)
Comment on attachment 8351982 [details] [diff] [review]
Patch V5 (!)

*** Original change on bio 235 attmnt 238 at 2009-09-07 17:48:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351982 - Attachment is obsolete: true
Attachment #8351982 - Flags: review?(florian)
*** Original post on bio 235 at 2009-09-07 19:01:17 UTC ***

Comment on attachment 8351983 [details] [diff] [review] (bio-attmnt 239)
Patch V6 (!!)

>diff --git a/instantbird/content/instantbird/conversation.xml b/instantbird/content/instantbird/conversation.xml
>+          Math.round(((totalSpace / 8) - this._TEXTBOX_VERTICAL_OVERHEAD) / lineHeight);
Why do you divide by 8 before removing the textbox verical overhead?

When resizing to a very small window, I can't get a 1-line sized textbox anymore, even when the statusbar is cropped.

>+        if (numberOfLines <= 0)
>+          numberOfLines = 1;
>+        else if (numberOfLines > 5)
>+          numberOfLines = 5;
The numerical values here (8 and 5) should at least be constants, and maybe they should even be preferences, but... constants will do for now :).

>diff --git a/instantbird/content/instantbird/instantbird.js b/instantbird/content/instantbird/instantbird.js

>+  onresize: function mo_onresize(aEvent) {
>+    if (aEvent.originalTarget != window)
>+      return;
>+
>+    // Resize each textbox (if the splitter has not been used).
>+    let convs = getBrowser().conversations;
>+    for (let i = 0; i < convs.length; ++i) {
>+      let conv = convs[i];
You probably want to use something like:
  for each (let conv in convs)

>+      let splitter =
>+        document.getAnonymousElementByAttribute(conv, "anonid", "splitter-bottom");
>+      if (!splitter.hasAttribute("state")) {
>+        let textbox = conv.calculateTextboxDefaultHeight();
>+        textbox.parentNode.height = textbox.defaultHeight;
>+      }
>+      if (Conversations.textboxAutoResize)
>+        conv.inputExpand();
>+    }
>   }
Is there anyway to put this code in the conversation binding, so that resizing still works even if the binding is used in another window that doesn't use instantbird.js?
I'm not sure if the resize event is dispatched to all elements that are resized or only to the window.

If we can't put all of this in the binding, we can still move some of this code into an onresize method in the binding, and call it from here.

Looks good otherwise!
I can't wait to see this in nightlies :).
Attached patch Patch V7 (!!!)Splinter Review
*** Original post on bio 235 as attmnt 240 at 2009-09-07 22:40:00 UTC ***

> Why do you divide by 8 before removing the textbox verical overhead?
Well, we don't really need to subtract by _TEXTBOX_VERTICAL_OVEREHAD

>The numerical values here (8 and 5) should at least be constants, and maybe
they should even be preferences, but... constants will do for now :).
Done, I have also changed the 8 in 10 (personal appreciation)


> Is there anyway to put this code in the conversation binding, so that resizing
still works even if the binding is used in another window that doesn't use
instantbird.js?
Done in a new method.

> I'm not sure if the resize event is dispatched to all elements that are resized or only to the window.
Resize is a window related event, the only way to get a resize event on something inside the window is to filter the originalTarget in the window.onResize(event).
And after some checks, it seems that only a few elements are concerned (browser, window and hu ... another one)


> Looks good otherwise!
> I can't wait to see this in nightlies :).
Me too ... 7th patch so far.
Attachment #8351984 - Flags: review?(florian)
Comment on attachment 8351983 [details] [diff] [review]
Patch V6 (!!)

*** Original change on bio 235 attmnt 239 at 2009-09-07 22:40:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351983 - Attachment is obsolete: true
Attachment #8351983 - Flags: review?(florian)
Comment on attachment 8351984 [details] [diff] [review]
Patch V7 (!!!)

*** Original change on bio 235 attmnt 240 at 2009-09-08 09:07:59 UTC ***

>diff --git a/instantbird/content/instantbird/conversation.xml b/instantbird/content/instantbird/conversation.xml

>+     <method name="_onSplitterChange">
>+        let textbox = document.getAnonymousElementByAttribute(document.getBindingParent(this),
>+                                                              "anonid", "inputBox");
This line is well over 80 columns and should be wrapped.

>diff --git a/instantbird/content/instantbird/imWindows.jsm b/instantbird/content/instantbird/imWindows.jsm

>     if (aTopic == "purple-quit") {
>       for (let id in this._purpleConv)
>         this._purpleConv[id].unInit();
>+      this._prefBranch.removeObserver(Conversations._textBoxAutoResizePrefName,
>+                                      Conversations);
Typo here, you mean _textboxAutoResizePrefName, not _textBoxAutoResizePrefName

r=me!
I'll fix the 2 details mentioned above before checking in the patch!
Thanks, this improvement is awesome!
Attachment #8351984 - Flags: review?(florian) → review+
*** Original post on bio 235 at 2009-09-08 10:08:12 UTC ***

https://hg.instantbird.org/instantbird/rev/6cbbe986e33e
Fixed!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.