576642 - JSTerm: expanding/shrinking inputNode

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

In Jesse's JSShell, if you hit shift-enter the input node on the shell expands in height by one line, and a newline is added to the input value. 

It will also be handy to have a button that expands and shrinks the code input node.

Comment 1

[Julian Viereck]

Adding multi line support to the textbox is as easy as adding a "multiline": "true" to the input node.

The bigger problem is to adjust the size when new lines are added. There is an `overflow` event that is fired on the textbox when a new line is added and lines doesn't fit in the current viewport of the textbox. I found an example on the internet that increases the number of rows by one when the `overflow` event occurs. Whereas this works for the hitting SHIFT+ENTER case, it doesn't work when pasting some text that causes more then 1 line to be added. The overflow event is fired just once although more rows need to be added. There is also the problem of shrinking the textbox again.

To implement this feature, a function/property is required, that returns the height of all the textbox's lines. Then after hitting a key combo or after the paste event took place, the height of the inputNode could get recalculated and the layout aligned. I tried things like 

    inputNode.scrollHeight
    inputNode.boxObject.height
    inputNode.inputField.scrollHeight

but all this retuned false values.

I'll try to think of other solutions, but ideas are welcome ;)

Comment 2

[David Dahl :ddahl]

did you look at the code in Jesse Ruderman's JSShell? Perhaps it is only expanding by one line when you hit shift-enter.

What about on keypress you check the number of newlines and adjust the height to 1 line height higher? when there are no newlines you keep it to 1 line high.

At some point in the future we may have to think about XBL for this entire interface.

Comment 3

[Julian Viereck]

(In reply to comment #2)
> What about on keypress you check the number of newlines and adjust the height
> to 1 line height higher? when there are no newlines you keep it to 1 line high.

The problem is, that I don't know the height of one line. Otherwise it would be easy.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Jesse's jsshell is using a textarea in HTML. Implementation's going to be a little different.

Comment 5

[David Dahl :ddahl]

I was thinking that if you had to, you could use 2 input nodes, swapping between them as needed. One that is set as not multiline and one that is multiline

Comment 6

[Rob Campbell [:rc] (:robcee)]

That sounds tricksy. You should be able to do this with only one textbox. I'm wondering if there's some magical event you can fire to get the elements to resize/reflow themselves appropriately?

Comment 7

[David Dahl :ddahl]

(In reply to comment #6)
> That sounds tricksy. You should be able to do this with only one textbox. 

I think we decided it would be best to just set both attributes (multiline and rows) when we detect the first line break 

what do you think enn?

Comment 8

[Julian Viereck]

(In reply to comment #7)
> (In reply to comment #6)
> > That sounds tricksy. You should be able to do this with only one textbox. 
> 
> I think we decided it would be best to just set both attributes (multiline and
> rows) when we detect the first line break 
> 
> what do you think enn?

I tried this. The problem is, that the first shift+enter can be detected using a keydown event listener. When you set multiline to true, the pressed key shift+enter doesn't cause a new line break (I guess because the textbox was not multiline when the shift+enter key was pressed) which means you have to emulate this inside of the keypress method. That looks like a hack to me.

Something I want to try is setting multiline="true", rows="1"for the xul:textbox and on the html:textarea used by the xul:textbox style="overflow-x:hidden", which should give a textbox that looks like a textbox with multiline="false". When the shift+enter key is pressed, the number of rows is increased.

Comment 9

[David Dahl :ddahl]

according to Enn on irc:

 ddahl: using two elements, one multiline and one single is your best bet I think

Comment 10

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #9)
> according to Enn on irc:
> 
>  ddahl: using two elements, one multiline and one single is your best bet I
> think

well, I sit corrected!

Comment 11

[Julian Viereck]

Attachment: Patch

Note: I changed the line-height from 1.6em to 1.2em because that looks better to me when having multiple lines. I'm not sure how this behaves on Linux/Windows.

Comment 12

[David Dahl :ddahl]

(In reply to comment #11)
> Created attachment 459412 [details] [diff] [review]
> Patch
> 
> Note: I changed the line-height from 1.6em to 1.2em because that looks better
> to me when having multiple lines. I'm not sure how this behaves on
> Linux/Windows.

That looks good on linux too, no worries

Comment 13

[David Dahl :ddahl]

Comment on attachment 459412 [details] [diff] [review]
Patch

Looks good, Julian

Comment 14

[Julian Viereck]

Comment on attachment 459412 [details] [diff] [review]
Patch

Requesting approval for this bug because it improves the way the user can work with the WebConsole. Instead of entering only one line strings, he can enter multiple lines as well and edit them. Also, multiline text pasted into the WebConsole's inputNode stays multiline which is more user friendly.

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 459412 [details] [diff] [review]
Patch

First reaction whenever I see a setTimeout is always r-. I'd prefer it if you got enn to review it since he usually comes up with elegant solutions that don't require a setTimeout.

Comment 16

[Julian Viereck]

Attachment: Patch rebased

Comment 17

[David Dahl :ddahl]

Who are you looking for review from? it is not marked with a reviewer.:)

Comment 18

[Neil Deakin]

Comment on attachment 462082 [details] [diff] [review]
Patch rebased

>+      // When the keyDown event was performed, set the number of rows to the
>+      // number of new lines.
>+      setTimeout(function() {
>+        self.inputNode.setAttribute("rows",
>+          Math.min(8, self.inputNode.value.split("\n").length));
>+      }, 0);
>+

This code will only get called when a key is pressed. It will not be called when the user pastes into it or drags text and drops it there (both of which can change the value). You likely really want to use the 'input' event, which fires when the user changes the value by any means (and only then, as keydown can also fire when the value hasn't changed as well.)


>+            if (aEvent.cancelable) {
>+              aEvent.preventDefault();
>+            }

Just aEvent.preventDefault() is sufficient.

I'm not particularly fond of auto-resizable textfields though. I would just add resizable="true" so that the user can resize it themselves, although maybe the frame size will prevent that from working.

Comment 19

[Julian Viereck]

(In reply to comment #18)
> Comment on attachment 462082 [details] [diff] [review]
> Patch rebased
> 
> >+      // When the keyDown event was performed, set the number of rows to the
> >+      // number of new lines.
> >+      setTimeout(function() {
> >+        self.inputNode.setAttribute("rows",
> >+          Math.min(8, self.inputNode.value.split("\n").length));
> >+      }, 0);
> >+
> 
> This code will only get called when a key is pressed. It will not be called
> when the user pastes into it or drags text and drops it there (both of which
> can change the value). You likely really want to use the 'input' event, which
> fires when the user changes the value by any means (and only then, as keydown
> can also fire when the value hasn't changed as well.)
> 
> 
> >+            if (aEvent.cancelable) {
> >+              aEvent.preventDefault();
> >+            }
> 
> Just aEvent.preventDefault() is sufficient.

Thanks for these hints Neil.

> 
> I'm not particularly fond of auto-resizable textfields though. I would just add
> resizable="true" so that the user can resize it themselves, although maybe the
> frame size will prevent that from working.

This works, but it's a strange behavior when the user adds a line break and then has to realize he has to drag the resizer at the right-bottom edge to see what he is typing. That's why I like the auto-resizeable way it is implemented in this patch.

@ddahl et. all: What do you think about this?

Comment 20

[Kevin Dangoor]

In this particular case, I think auto-resizing would actually be quite nice. From my experience using Firebug's console, many times I'm just running single line statements to evaluate. When I *do* want to write out a function, I find the switch from the single line interface to the multi-line one a bit jarring. Having to manually resize the element would be even more jarring (because I would have to reach for the mouse... if there's a way to expand the element via the keyboard, I don't know what it is)

Expanding to accommodate multiline input sounds great to me.

Comment 21

[David Dahl :ddahl]

(In reply to comment #20)

> Expanding to accommodate multiline input sounds great to me.
Agreed, at the very least, lets get some feedback on this from beta users. I think this is a useful feature.

Comment 22

[Julian Viereck]

Attachment: [backed-out] Patch 2 improved on Neil's feedback + unit tests

Comment 23

[David Dahl :ddahl]

We should get approval for this. cool!

Comment 24

[David Dahl :ddahl]

(In reply to comment #23)
> We should get approval for this. cool!

Getting feedback for this cool feature in the next beta would be very useful.

Comment 26

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 463176 [details] [diff] [review]
[backed-out] Patch 2 improved on Neil's feedback + unit tests

http://hg.mozilla.org/mozilla-central/rev/cfc0accaf8b2

Comment 27

[Patrick Walton (:pcwalton)]

Doesn't work due to conflict with the prompt (bug 583349). Will likely back out.

Comment 28

[Rob Campbell [:rc] (:robcee)]

we need to back this out due to the conflict mentioned in comment #27. It's 10:30pm here and we're waiting for a clean run on a windows clobber build. ETA is no less than 3 hours so I'm waiting to do this tomorrow unless somebody in a different timezone can manage it.

Comment 29

[Rob Campbell [:rc] (:robcee)]

backed out:
http://hg.mozilla.org/mozilla-central/rev/d50d2ccb5bbf

Comment 30

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, version 2 (rebased to trunk 2010-08-10).

Patch rebased to trunk.

Comment 31

[Patrick Walton (:pcwalton)]

http://hg.mozilla.org/mozilla-central/rev/a4b18999dc5b

Comment 32

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 464576 [details] [diff] [review]
[checked-in] Proposed patch, version 2 (rebased to trunk 2010-08-10).

http://hg.mozilla.org/mozilla-central/rev/a4b18999dc5b