Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Rework on Input #580

Merged
merged 43 commits into from
Aug 5, 2019
Merged

Rework on Input #580

merged 43 commits into from
Aug 5, 2019

Conversation

byronz
Copy link
Contributor

@byronz byronz commented Jul 10, 2019

this fixes both #455 and #513

@byronz
Copy link
Contributor Author

byronz commented Jul 10, 2019

feedback is welcome before I'm adding a few test cases for type=number

@byronz byronz changed the title 🐛 fixes both #455 and #513 WIP fixes both #455 and #513 Jul 10, 2019
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the change enables the user to enter a negative number easily, I'm wondering if we are breaking an expectation in terms of API. When entering 1 or 1.4 in master the value in the callback is an int and a float respectively. With the change it's always a string.

The original issue comments is more debounce centric but does offer some comments about number vs string. #169

Current value proptype
Previous value proptype

Seems reasonable to want to continue providing the extra value of the cast when possible, if type=number.

Maybe we can use a state.value with the actual string and update props.value after a validation/cast to keep this behavior?

The HTML input does always have string as type of value.

@alexcjohnson
Copy link
Collaborator

Agreed - the prop value should be a number. Interestingly, playing with it now in Chrome, the browser does character filtering to the set 1234567890eE+-. but no more sanity checks than that (you can write +-+-eEeEe-+-+ if you so desire...) other than a built-in tooltip that shows up if the number is invalid:

Screen Shot 2019-07-12 at 12 25 02 PM

I'd propose we allow the user to type whatever they want, and have separate focused and blurred behavior:

  • While focused: don't rerender under any circumstances, and only set the prop if the value was successfully cast to a number.
  • On blur: keep current behavior - cast to a number, throw the input away if casting fails.

So here are two cases to consider

  • User types junk like eEeEe -> it stays there while focused, the prop is unchanged from whatever value it had previously. On blur we rerender, and either put back the previous value or clear the field (configurable like table edits? For now just do whatever matches the current behavior most closely)
    only set the prop if the value was successfully cast to a number.
  • User types a nonstandard variant of a number, like 1e+1 -> it stays like that while focused, while the prop is set to (number) 10. On blur we rerender as 10.

Seem reasonable?

@byronz
Copy link
Contributor Author

byronz commented Jul 12, 2019

demo

@Marc-Andre-Rivet
Copy link
Contributor

Looking at the code, this component is a bit weird at the moment.

There's 3 scenarios where we set value:

  • onChange: Only if debounce=false, this does a min/max check
  • onBlur / onEnter: Only if debounce=true, this does NOT do a min/max check

🐛 Separate bug to fix the min/max check -- whether we fix it at the same time or not is up to us.

While focused: don't rerender under any circumstances, and only set the prop if the value was successfully cast to a number.
On blur: keep current behavior - cast to a number, throw the input away if casting fails.

  • Whatever happens on blur should happen on Enter too
  • Clearing the input if the cast fails seems counter-intuitive to me. This is not how the input component behaves by default. I'd try to stay as close to the normal behavior as possible
  • I'd suggest that setting value should ALWAYS happen for blur/enter whether debounce is true/false.
    If the cast fails we send back None
  • Maybe we could add some usability / visual indicator when the value is invalid instead of erasing it: some red border, etc. (this is technically an enhancement and out of scope)

@byronz
Copy link
Contributor Author

byronz commented Jul 12, 2019

  • Clearing the input if the cast fails seems counter-intuitive to me. This is not how the input component behaves by default. I'd try to stay as close to normal behavior as possible

yes, I don't feel the clearing input while onblur or Enter keypress when cast failed is a good solution, something else like a tooltip or red box might be more intuitive.

  • I'd suggest that setting value should ALWAYS happen for blur/enter whether debounce is true/false.
    If the cast fails we send back None

None is not clear for me @Marc-Andre-Rivet can you be more specific?

@Marc-Andre-Rivet
Copy link
Contributor

@byronz By None I mean what the DashPy user would get. In JS, I'm suggesting we set the value to null.

@byronz
Copy link
Contributor Author

byronz commented Jul 15, 2019

debounce => False

debounce_false

debounce => True

debounce_true

@alexcjohnson @Marc-Andre-Rivet please review the improved error handling behavior with numbers

take note that browsers have different native behavior for input[number] mode:

  • Chrome is the only one does the key filtering, i.e. only accepts valid number range, but has no number validation
  • Firefox accepts any keypress but does a number validation with a css effect of box-shadow: red
  • Safari does neither key filtering nor number validation

the current solution will have a harmonized effect similar to native firefox validation

@Marc-Andre-Rivet
Copy link
Contributor

The behavior seems inconsistent between Firefox and (Chrome + Safari) when entering an invalid value — in Firefox the value stays in the input while in the other two the value becomes blank. In all cases the default behavior of the html field is to retain the value.

I think this is a side-effect of the implementation causing undefined to be passed to the input element / handled differently across browsers.

@Marc-Andre-Rivet
Copy link
Contributor

@byronz From our previous discussion and using the code you gave me, here's the behavior I'm seeing in Chrome (top) and Firefox (bottom). Notice that Chrome clears the input while Firefox does not.

I am using tab and shift+tab to move focus around and trigger the blur.

import dash
from dash.dependencies import Input, Output
import dash_core_components as dcc
import dash_html_components as html

app = dash.Dash(__name__)
app.layout = html.Div(
    [
        dcc.Input(
            id="dfalse",
            type="number",
            debounce=False,
            placeholder="debounce=>False",
        ),
        html.Div(id='out_false', children=''),
        html.Hr(id='bl'),
        dcc.Input(
            id="dtrue",
            type='number',
            debounce=True,
            placeholder="debounce=>True",
        ),
        html.Div(id='out_true', children=''),
    ]
)

@app.callback(
    [Output('out_false', 'children'), Output('out_true', 'children')],
    [Input('dfalse', 'value'), Input('dtrue', 'value')])
def render(fval, tval):
    return fval, tval


if __name__ == "__main__":
    app.run_server(debug=True)

Chrome-input-undef

Firefox-input-undef

@byronz
Copy link
Contributor Author

byronz commented Jul 17, 2019

this is another approach by hijacking the number type inside the Input component and applying similar validation to have consistent behavior.

textmode_2

the problem I see:

  1. we don't have number input filtering from Chrome
  2. the input is not a native number input with the right range selector, the auto-search popup is also annoying.

@Marc-Andre-Rivet @alexcjohnson I feel the native number input might be a better compromise for the time being.

@byronz byronz changed the title WIP fixes both #455 and #513 Rework on Input Aug 2, 2019
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo commits for the Python and R build artifacts and we're good to go.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 🕺

@byronz byronz merged commit 560d4e1 into master Aug 5, 2019
@byronz byronz deleted the input-fix branch August 5, 2019 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants