Allow text input on slider widgets #6106

chronitis · 2014-07-10T07:04:32Z

This adds "click-to-edit" to the text readout to the right of a slider, allowing precise values to be set.

Changes are entirely on the javascript. Editing is handled using the HTML5 contentEditable attribute rather than explicitly creating any input widget. The modified value is committed by either clicking outside the text, or pressing enter. Works for both Int and FloatSliderView.

The new value is validated for float/int and min/max, but the step size (if set) is not enforced.

This will need updating if #6050 is merged.

minrk · 2014-07-23T18:09:18Z

ping @ellisonbg, @jdfreder for review

ellisonbg · 2014-07-23T19:00:58Z

Haven't looked at the implementation yet.

Is this something we want to support? It adds complexity and textual editing of the slider's value can already be done by linking it to a second text input widget. It also impacts the visual size of the slider - I think it is important to keep it compact. I am probably 50/50 myself.

chronitis · 2014-07-23T22:01:48Z

The layout of the slider is not actually changed.

If the slider is created with readout=True (the default) a label with the value is already displayed to the right. This just allows you to click the existing label and edit the contents inplace.

If readout is disabled, no label is displayed and this does nothing.

The use case is wanting to normally be able to try a range of values, in typical data-exploration style, but occasionally wanting to be able to cherry-pick a specific (probably round) value which mouse resolution makes difficult to select with the slider (eg, when you can only manage to select 99 or 101). This I would assert is something that people might want to do relatively often without wanting to go back and alter the slider parameters or set up a bound text entry widget.

The change consists only of setting the contentEditable attribute on the label and adding an event handler to detect a new value being entered.

SylvainCorlay · 2014-07-23T22:39:04Z

content_editable could also be a key of the widget.

jdfreder · 2014-08-05T23:19:45Z

I think this implementation is overly complicated- it seems like you are just reimplementing the IntTextView and FloatTextView here. Did you look at replacing the readout with them instead?

chronitis · 2014-08-07T11:24:48Z

Using IntTextView as a child of the slider would require dealing with synchronisation between the traits and probably complications in nesting the box models and events - probably doable, but I don't think it could be done more simply than this.

It is true that the handleTextChange function largely duplicates IntTextView.handleChanging. I can see if it is possible to re-use the existing function.

chronitis · 2014-08-07T13:54:32Z

@jdfreder https://gist.github.com/chronitis/5bb76f913aa91eebc177 shows a possible re-working (the common validate and update logic is removed from both widgets to a separate function). I'm not sure if this actually makes it more readable though - let me know if this seems a reasonable direction and I'll commit this to the PR.

jasongrout · 2014-08-07T17:41:50Z

Not using content editable makes it work better for numeric input on mobile, I think. For example, see http://sagecell.sagemath.org/?q=dxgrse

chronitis · 2014-08-07T18:45:07Z

http://caniuse.com/#feat=contenteditable claims very wide support (everything minus android 2.3 and opera mini) for contenteditable, but the Sage version using input does look pretty good too - preferences?

jasongrout · 2014-08-07T18:46:57Z

but the input also pulls up a numeric keyboard on mobile, IIRC. Would contenteditable? I think mobile in general has a hard time with knowing when to pull up a keyboard on content editable, and definitely wouldn't know to bring up a numeric keyboard.

ellisonbg · 2014-09-12T17:22:43Z

@jdfreder @jasongrout @SylvainCorlay can you folks make a decision on this widget related PR and push it through review. In our call today, we decided that in principle we are OK with it - but you 3 should make the final decision.

SylvainCorlay · 2014-09-14T16:24:20Z

I have not read the code yet. I imagine that one may not want to enable content-editable for all sliders. Having a widget key to enable/disable it (set to true by default) could be an option.

chronitis · 2014-09-15T09:09:23Z

I can add a trait to the widget that disables this behaviour easily (and will, if there is consensus to do so) - but I wonder if doing it on a per-widget basis makes sense?

It is not obvious to me that a user is likely to care enough to selectively disable this on certain sliders (or other widgets) but not others. A global option to enable or disable use of content-editable perhaps makes more sense?

jasongrout · 2014-09-15T17:05:15Z

It is not obvious to me that a user is likely to care enough to selectively disable this on certain sliders (or other widgets) but not others. A global option to enable or disable use of content-editable perhaps makes more sense?

If that's the case, then it would make more sense to have a subclass the user can use that has the current behavior, rather than a global option.

jasongrout · 2014-09-15T17:06:23Z

I have not read the code yet. I imagine that one may not want to enable content-editable for all sliders. Having a widget key to enable/disable it (set to true by default) could be an option.

I think we ought to just enable it always, and wait for usecases to come up where you don't want enabled before adding an option to turn it off.

jdfreder · 2014-09-15T17:08:11Z

TODO: I'm going to review the gist you published.

Sorry for the delay, I've been away a couple of weeks.

jdfreder · 2014-09-15T19:00:50Z

IPython/html/static/widgets/js/widget_float.js

@@ -9,6 +9,10 @@ define([
    var IntTextView = int_widgets.IntTextView;

    var FloatSliderView = IntSliderView.extend({
+        _parse_text_input: parseFloat,
+
+        _range_regex: /^\s*([+-]?\d*\.?\d+)\s*[-:]\s*([+-]?\d*\.?\d+)/,


I think this should be ^\s*([+-]?\d*\.?\d?)\s*[-:]\s*([+-]?\d*\.?\d?) which allows the python shorthand float declaration, i.e. 1.-2..

As written this matches a lone decimal point, and does not match numbers with >1 decimal place - but I agree with the point. I'll fix it so it matches 1..

Edit: I think ([+-]?(?:\d*\.?\d+|\d+\.)(?:[eE][+-]?\d+)?) (x2) matches all appropriate patterns, including exponents (which are probably wanted for scientific use).

jdfreder · 2014-09-15T19:29:01Z

@jdfreder https://gist.github.com/chronitis/5bb76f913aa91eebc177 shows a possible re-working (the common validate and update logic is removed from both widgets to a separate function). I'm not sure if this actually makes it more readable though - let me know if this seems a reasonable direction and I'll commit this to the PR.

Hey @chronitis , thanks for trying this. This isn't actually what I was envisioning, I was talking about scrapping the use of content-editable all together and just re-using the numeric text widgets as readouts (of course with some styling changes). I don't think this makes sense anymore, now that the slider widgets support value ranges. I added a couple of comments inline, otherwise the code looks good.

jdfreder · 2014-09-15T19:39:01Z

IPython/html/static/widgets/js/widget_int.js

+                    } else {
+                        // clamp to range
+                        values = [Math.max(Math.min(values[0], vmax), vmin),
+                                  Math.max(Math.min(values[1], vmax), vmin)];


At first glance I thought this would cause trouble, since you do not check if min and max are undefined, but the widget won't exist in the front-end until it's been opened from the back-end, and the second it's opened from the back-end, a full state is pushed. When a widget like this is opened from the front-end, it will be important to remember that some values have to be defined on the model before it will work properly (this is probably not the only place) @jasongrout @SylvainCorlay .

Carreau · 2014-09-24T12:51:23Z

Leaving on need review (cause we need to finish reviewing), but will need a rebase at some point also.

chronitis · 2014-09-24T14:03:48Z

Rebased to current master.

Carreau · 2014-10-01T08:15:55Z

IPython/html/static/widgets/js/widget_int.js

        }, 

+        handleKeyDown: function(e) {
+            if (e.keyCode == 13) {


Do you want to require static/base/js/keyboard and user keycode.enter here ?

👍 for this suggestion

Carreau · 2014-10-01T08:19:28Z

Looks good. I had a quick overview, but I'm not conforable on widgets.

One potential minor comment to use explicit keymap with enter instead of hardcoded 13.
But it is just a suggestion.

jdfreder · 2014-10-08T20:15:53Z

Thanks @chronitis this looks good. I'm marking it as ready to merge and I'll let someone else click the button!

Carreau · 2014-10-09T09:09:39Z

Agreed looks good. I'll push the green button.

Allow text input on slider widgets

minrk added this to the 3.0 milestone Jul 23, 2014

minrk added the Needs review label Jul 29, 2014

jdfreder added Waiting for author and removed Needs review labels Aug 5, 2014

Carreau added Needs review and removed Waiting for author labels Sep 3, 2014

takluyver assigned jdfreder Sep 12, 2014

ellisonbg added widgets type-enhancement labels Sep 12, 2014

minrk removed the needs-decision label Sep 15, 2014

ellisonbg removed the needs-decision label Sep 15, 2014

jdfreder reviewed Sep 15, 2014
View reviewed changes

jdfreder added Waiting for author and removed Needs review labels Sep 15, 2014

jdfreder removed their assignment Sep 15, 2014

Carreau added Needs review and removed Waiting for author labels Sep 20, 2014

chronitis added 6 commits September 24, 2014 15:56

Use contentEditable to allow modification via the the slider readout

66b32ee

Add support to the float slider

c6ecc98

Add keydown listener to commit changes on <enter>

d741bc7

Ignore the event object for handleTextChange

cdab764

Add support for parsing pairs of numbers for range sliders

71d59e8

Change _parse_text_input to _parse_value and update float range regex

e4c2069

chronitis force-pushed the interact-slider-textedit branch from fec3566 to e4c2069 Compare September 24, 2014 14:02

Carreau reviewed Oct 1, 2014
View reviewed changes

Require base/js/keyboard and use keycode.enter

8d226ee

jdfreder added Ready To Merge and removed -Needs review labels Oct 8, 2014

Carreau added a commit that referenced this pull request Oct 9, 2014

Merge pull request #6106 from chronitis/interact-slider-textedit

d4de7b7

Allow text input on slider widgets

Carreau merged commit d4de7b7 into ipython:master Oct 9, 2014

chronitis deleted the interact-slider-textedit branch October 9, 2014 15:46

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#6106 from chronitis/interact-slider-textedit

a529af5

Allow text input on slider widgets

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow text input on slider widgets #6106

Allow text input on slider widgets #6106

chronitis commented Jul 10, 2014

minrk commented Jul 23, 2014

ellisonbg commented Jul 23, 2014

chronitis commented Jul 23, 2014

SylvainCorlay commented Jul 23, 2014

jdfreder commented Aug 5, 2014

chronitis commented Aug 7, 2014

chronitis commented Aug 7, 2014

jasongrout commented Aug 7, 2014

chronitis commented Aug 7, 2014

jasongrout commented Aug 7, 2014

ellisonbg commented Sep 12, 2014

SylvainCorlay commented Sep 14, 2014

chronitis commented Sep 15, 2014

jasongrout commented Sep 15, 2014

jasongrout commented Sep 15, 2014

jdfreder commented Sep 15, 2014

jdfreder Sep 15, 2014

chronitis Sep 16, 2014

jdfreder commented Sep 15, 2014

jdfreder Sep 15, 2014

Carreau commented Sep 24, 2014

chronitis commented Sep 24, 2014

Carreau Oct 1, 2014

jdfreder Oct 8, 2014

Carreau commented Oct 1, 2014

jdfreder commented Oct 8, 2014

Carreau commented Oct 9, 2014

Allow text input on slider widgets #6106

Allow text input on slider widgets #6106

Conversation

chronitis commented Jul 10, 2014

minrk commented Jul 23, 2014

ellisonbg commented Jul 23, 2014

chronitis commented Jul 23, 2014

SylvainCorlay commented Jul 23, 2014

jdfreder commented Aug 5, 2014

chronitis commented Aug 7, 2014

chronitis commented Aug 7, 2014

jasongrout commented Aug 7, 2014

chronitis commented Aug 7, 2014

jasongrout commented Aug 7, 2014

ellisonbg commented Sep 12, 2014

SylvainCorlay commented Sep 14, 2014

chronitis commented Sep 15, 2014

jasongrout commented Sep 15, 2014

jasongrout commented Sep 15, 2014

jdfreder commented Sep 15, 2014

jdfreder Sep 15, 2014

Choose a reason for hiding this comment

chronitis Sep 16, 2014

Choose a reason for hiding this comment

jdfreder commented Sep 15, 2014

jdfreder Sep 15, 2014

Choose a reason for hiding this comment

Carreau commented Sep 24, 2014

chronitis commented Sep 24, 2014

Carreau Oct 1, 2014

Choose a reason for hiding this comment

jdfreder Oct 8, 2014

Choose a reason for hiding this comment

Carreau commented Oct 1, 2014

jdfreder commented Oct 8, 2014

Carreau commented Oct 9, 2014