Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form focus #268

Closed
wants to merge 15 commits into from
Closed

Form focus #268

wants to merge 15 commits into from

Conversation

alephex
Copy link

@alephex alephex commented May 8, 2015

Further to #244, #249 and #250

Current behaviour

When a page is loaded, the first input of the first form is automatically focused.

Problematic behaviour

If the first form is below the fold, web browsers automatically scroll down to the focused element. This means that, in some cases, a user may load a page and immediately be sent down to wherever the form is. This issue is particularly apparent on mobile/responsive pages.

Proposed solution

Forms are to have an additional argument to determine if input focusing should be done automatically, manually or not at all. In the case of manual focusing, child input fields of the form will have an additional argument to determine if they are to receive focus.

Forms can take an additional argument when they are created.
focus_form = 'auto' | 'manual' | 'off'
This results in the form being rendered with a HTML attribute data-deform-focus-form with any of the above values.

The javascript function focusFirstInput will perform the following.

focus_form action
'auto' First input of the first form is focused
'manual' First input with manual_focus set is focused
'off' No focusing is done
omitted Same as 'auto'

Examples

Default

class SchemaInputDefault(colander.Schema):
    name = 'Default Input Focus'
    input = colander.SchemaNode(
    colander.String(),
    title = 'Input',
    missing = '')

class TestForm():
    def __init__(self, request):
        schema = SchemaInputDefault(validator=self.validate).bind(request=request)
        self.form = deform.Form(schema, buttons=(button,))

Auto

class SchemaInputAuto(colander.Schema):
    name = 'Auto Input Focus'
    input = colander.SchemaNode(
                  colander.String(),
                  title = 'Input',
                  missing = '')

class TestForm():
    def __init__(self, request):
        schema = SchemaInputAuto(validator=self.validate).bind(request=request)
        self.form = deform.Form(schema, buttons=(button,), focus_form='auto')

Manual

class SchemaInputManual(colander.Schema):
    name = 'Manual Input Focus'

    input1 = colander.SchemaNode(
                    colander.String(),
                    title = 'Input 1',
                    missing = '')

    input2 = colander.SchemaNode(
                    colander.String(),
                    title = 'Input 2 (Will be focused)',
                    missing = '',
                    manual_focus = 'on')

    input3 = colander.SchemaNode(
                    colander.String(),
                    title = 'Input 3',
                    missing = '')

class TestForm():
    def __init__(self, request):
        schema = SchemaInputManual(validator=self.validate).bind(request=request)
        self.form = deform.Form(schema, buttons=(button,), focus_form='manual')

Off

class SchemaInputOff(colander.Schema):
    name = 'No Input Focus'
    input = colander.SchemaNode(
                 colander.String(),
                 title = 'Input',
                 missing = '')

class TestForm():
    def __init__(self, request):
        schema = SchemaInputOff(validator=self.validate).bind(request=request)
        self.form = deform.Form(schema, buttons=(button,), focus_form='off')

Results

  • Default behaviour is unchanged
  • On page load, the first input field of the first form receives focus (default)
  • Any one input field can be chosen to receive focus
  • Automatic focusing can be disabled on a per-form basis

Existing nose tests pass. Some basic tests were added (checking defaults etc). I'm not sure if this is a big enough change to warrant any extra examples on deformdemo, but I can put together a pull request for that if it might be worthwhile.

alephex added 5 commits May 7, 2015 22:00
…deform-focus-form to auto, manual or off. Input fields now have a manual_focus parameter which sets HTML attribute data-deform-manual-focus to on if the parameter is present.
The first form whose data-deform-focus-form attribute is not set to 'off' will
have one of its child inputs receive focus on page load.

If the form's data-deform-focus-form attribute is 'manual', the first child
input field with data-deform-manual-focus set to 'on' will receive focus.

If the form's data-deform-focus-form attribute is 'auto', the first child
input field will receive focus.

If all forms have data-deform-focus-form = 'off', then no focusing will occur.

Default behaviour is 'auto'.
Tests added for form focus_form=auto|manual|off.
Test added for fields manual_focus=None default.
@tisdall
Copy link
Contributor

tisdall commented May 8, 2015

I'm not sure I see the need for both "manual" and "off". I think it should just be "manual" and then it focuses on the first field that it finds that's marked as requiring focus. "off" would be covered by using "manual" but just not giving any item focus.

Or, alternatively... The default is to autofocus="on" and the first field is given focus if no specific field is marked for focus, if a field is specified for focus then the first one specified is used, the last option is autofocus="off" to the Form object to stop all focusing. That avoids confusing combinations like specifying on a field to give it focus but you set autofocus to "auto" and it's not used.

@stevepiercy @mcdonc @tseaver @mmerickel - what's your opinion on how to best implement this?

@alephex
Copy link
Author

alephex commented May 8, 2015

I'm not sure I see the need for both "manual" and "off".

It's pretty trivial to get rid of "off", but I think it would be better to have the ability to explicitly switch off focusing on a particular form.

The current process:
form.focus_form field.manual_focus action
n/a n/a focus the first input of the first form
This pull request:
form.focus_form field.manual_focus action
omitted n/a focus the first input of the first form
'auto' n/a focus the first input of the first form
'manual' 'on' focus on the first input of this form with 'manual_focus' set
'manual' omitted no focus on this form
'off' n/a no focus
@tisdall alternative:
form.focus_form field.manual_focus action
omitted n/a focus the first input of the first form
'auto' n/a focus the first input of the first form
'manual' 'on' focus on the first input of this form with 'manual_focus' set
'manual' omitted focus on the first input of this form
'off' n/a no focus

The difference between this PR and the alternative is how we want to failover if the user does not set field.manual_focus either by forgetting or by cut-and-paste without realising it's 'manual' rather than 'auto'.

Given that, in the context of this 'browser scrolling on load' problem, automatically focusing an input below the fold is very undesirable. If done unintentionally, it effectively screws up the user's initial view of the page. As such, I think it will be better to fail safe with no focus if field.manual_focus is omitted.

Omitted, False or 'off' = None
True or 'on' = 'on'
@tisdall
Copy link
Contributor

tisdall commented May 8, 2015

Actually my first paragraph was saying this (effectively merge "manual" and "off"):

form.focus_form field.manual_focus action
'auto' or omitted n/a focus the first input of the first form
'manual' 'on' focus on the first input of this form with 'manual_focus' set
'manual' omitted no focus

and my "alternative" was this:

form.focus_form field.manual_focus action
'on' or omitted omitted focus the first input of the first form
'on' 'on' focus on the first input of this form with 'manual_focus' set
'off' n/a no focus

I think my later suggestion is most clear. So, "on" means there's always an autofocus of some sort, and "off" means never auto focus.

@alephex
Copy link
Author

alephex commented May 8, 2015

I think my later suggestion is most clear. So, "on" means there's always an autofocus of some sort, and "off" means never auto focus.

That makes a lot more sense. I'll throw that together in this PR in a few minutes.

alephex added 4 commits May 8, 2015 15:50
False, 'off' = 'off'
Omitted or any other value = 'on'
If a form's data-deform-focus-form attribute is 'off', no focusing
will be done on that form.

If a form's data-deform-focus-form attribute is 'on' AND a child
input has its data-deform-manual-focus attribute set to 'on', that
input will be focused.

If a form's data-deform-focus-form attribute is 'on' AND no child has
its data-deform-manual-focus attribute set to 'on', the first input
of the form will be focused.
@alephex
Copy link
Author

alephex commented May 8, 2015

Current Process

form.focus_form field.manual_focus action
'on' or omitted omitted focus the first input of the first form
'on' or omitted 'on' focus on the input with manual_focus='on'
'off' n/a no focusing

Current Examples

Default (form.focus_form ='on' or omitted; field.manual_focus omitted)
class SchemaInputDefault(colander.Schema):
    name = 'Default Input Focus'
    input = colander.SchemaNode(
    colander.String(),
    title = 'Input (receives focus)',
    missing = '')

class TestForm():
    def __init__(self, request):
        schema = SchemaInputDefault(validator=self.validate).bind(request=request)
        self.form = deform.Form(schema, buttons=(button,))
Manual (form.focus_form='on', field.manual_focus='on')
class SchemaInputManual(colander.Schema):
    name = 'Manual Input Focus'

    input1 = colander.SchemaNode(
                    colander.String(),
                    title = 'Input 1',
                    missing = '')

    input2 = colander.SchemaNode(
                    colander.String(),
                    title = 'Input 2 (Will be focused)',
                    missing = '',
                    manual_focus = 'on')

    input3 = colander.SchemaNode(
                    colander.String(),
                    title = 'Input 3',
                    missing = '')

class TestForm():
    def __init__(self, request):
        schema = SchemaInputManual(validator=self.validate).bind(request=request)
        self.form = deform.Form(schema, buttons=(button,), focus_form='on')
Off (form.focus_form='off')
class SchemaInputOff(colander.Schema):
    name = 'No Input Focus'
    input = colander.SchemaNode(
                 colander.String(),
                 title = 'Input',
                 missing = '')

class TestForm():
    def __init__(self, request):
        schema = SchemaInputOff(validator=self.validate).bind(request=request)
        self.form = deform.Form(schema, buttons=(button,), focus_form='off')

@@ -109,6 +109,12 @@ class Field(object):
resource_registry
The :term:`resource registry` associated with this field.

manual_focus
If the field's parent form has its ``focus_form`` argument set to
``manual``, the first field with ``manual_focus`` set to ``on``
Copy link
Contributor

Choose a reason for hiding this comment

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

manual is no longer a valid value

@tisdall
Copy link
Contributor

tisdall commented May 12, 2015

Maybe this is just preference on my part, but I kind of prefer the name "focus" for the argument on the Form constructor. At the very least the "form" part of "focus_form" is kind of redundant.

Also, on the individual widgets maybe it should be "autofocus" instead of "manual_focus" to better match the HTML attribute name (whether it's being used or not, it accomplishes the same function).

What's your opinion?

@alephex
Copy link
Author

alephex commented May 12, 2015

I'm agreeable to the constructor name changes. They're more succinct and don't lose any information.

The HTML attributes should still be data-something though for the sake of producing valid HTML.

How does this sound?
form.focus and field.autofocus
<form data-deform-focus ... and <input data-deform-autofocus ...

@tisdall
Copy link
Contributor

tisdall commented May 12, 2015

Yeah, that looks good.

Set form.focus to 'on' to focus on the first input of the form
Set form.focus to 'on' and an input.autofocus to force focus on that input
Set form.focus to 'off' to disable focusing
@tisdall
Copy link
Contributor

tisdall commented May 12, 2015

Have you thought at all about using something like this: https://gist.github.com/terkel/1236495

It's basically a polyfill to allow HTML5 autofocus to work properly in older browsers.

@alephex
Copy link
Author

alephex commented May 12, 2015

Not a bad idea. We could scrap automatic focusing entirely and just let the user add field.autofocus to whatever input they like and use that polyfill in deform.js to support older browsers. The downside is, if the user omits field.autofocus from all inputs, no focusing is done. This changes default behaviour.

If I was the only one using deform, I would be ok with it. The whole reason I forked deform was to find out why my pages were scrolling all over the place and stop it. But there may be many people for whom the current default focusing behaviour is desired in their UX. It's probably better to leave the default as is.

@tisdall
Copy link
Contributor

tisdall commented May 12, 2015

hmm.. I was thinking of just doing what you're currently doing, but use autofocus instead of data-__ and utilize the polyfill. We'd still need to ensure only one field had the autofocus on it, and we'd also have to maintain backwards-compatibility by putting a autofocus on the first field by default.

@alephex
Copy link
Author

alephex commented May 13, 2015

That's not a bad idea. Moving all the deform.js:focusFirstInput() logic to field.py would leave everything nice and neat. (in terms of js anyway). The only HTML attribute set would be <input autofocus="autofocus" ....

One downside is deform does not (and cannot) know how many forms will be rendered on a page. This could result in more than one input having the autofocus attribute set. Default behaviour for a page with two forms would have autofocus set on (form 1, input 1) and (form 2, input 1). That doesn't meet the HTML5 spec but browsers will degrade gracefully and focus only the first input.

It's fairly trivial to implement this kind autofocus setup. However, we'll first need to decide if default behaviour in a fairly common use case (multiple forms) which technically fails to meet HTML5 specifications is acceptable. We could just document the behaviour and leave it up to the user.

Options:

  1. Use data-* attributes and js
    • + Produces valid HTML
    • - Cluttered
    • - More js to run on document.ready
  2. Add an autofocus attribute on rendering with no js (except for the polyfill) and leave the decisions to the user
    • + Much cleaner
    • + Uses autofocus attr. for its intended purpose
    • - Does not meet HTML5 spec in certain cases by default

Thoughts?

@tisdall
Copy link
Contributor

tisdall commented May 13, 2015

I think you've outlined it pretty clearly... unfortunately this is where someone with the ability to commit the changes needs to pipe in.

For me, I'd prefer the autofocus option with some documentation to warn people about avoiding having more than one autofocus on a page.

alephex added 2 commits May 13, 2015 20:05
Forms take a 'focus' parameter and fields take an 'autofocus' parameter

focus='on': If an input has autofocus='on', that input's autofocus HMTL
            attribute will be set.
focus='on': If no input has autofocus='on', the form's first valid input will
            have its autofocus HTML attribute set.
focus='off':No autofocus attributes will be set on this form

focusFirstInput() has been removed from deform.js and the logic has been
implemented in form.py and field.py

Templates have been updated to render autofocus="autofocus" when appropriate

Functional tests have been added to test autofocus attribute rendering

CHANGES.txt was updated to reflect current behaviour
@tisdall
Copy link
Contributor

tisdall commented May 17, 2015

Right now you have 3 PR's open for this same issue. If this one is the preferred one, can you close the other 2?

@alephex
Copy link
Author

alephex commented May 17, 2015

Fair enough. I'll leave this PR as it is and close the "default to no autofocus" and "selectable autofocus" PRs. This one performs form focusing with javascript. I have another branch that can be merged with this one which takes the alternate approach (focusing via autofocus HTML attributes).

We can go either way, but someone with write access will have to make a decision at some point on which direction to take.

This was referenced May 17, 2015
@alephex
Copy link
Author

alephex commented May 17, 2015

While I think of it, I may as well merge that branch now. We can always roll back later.

I have not included the js autofocus polyfill yet. While looking up which browsers support autofocus attributes, I found that that Safari on iOS does not support it because Apple decided autofocusing inputs on mobile was undesired (triggering keyboards etc).

@rbu
Copy link
Contributor

rbu commented May 21, 2015

Thanks for the work on this. Hoping this gets merged.

Removed unnecessary autofocus parameter checking in field.py
Added an extra unit test to test_field.py to get 100% coverage again.
Cleaned up some trailing whitespace.
@piotr-dobrogost
Copy link

Any reason this hasn't been merged, yet?

@stevepiercy
Copy link
Member

For starters, @piotr-dobrogost @alephex this branch has conflicts with master that must be resolved before merging.

You could try contacting the package maintainer.

@alephex
Copy link
Author

alephex commented Mar 8, 2016

this branch has conflicts with master that must be resolved before merging.

This PR has been sitting here for almost a year. There were no conflicts then but master has had a few changes in the mean time. I've moved away from using deform myself, but if a maintainer or someone with write access wants to merge this PR, I'll try to resolve them. But until I hear from a maintainer that they want to merge, I won't waste time on it.

@miohtama
Copy link
Contributor

miohtama commented Jun 2, 2016

Thank you for your pull request.

I am the new Deform project maintainer. I apologize the issue of this pull request not being commented earlier. Deform project is again under activate maintenance.

This is issue tracker / pull request clean to prepare a new release.

I am willing to merge this pull request. However looks like the original author is no longer championing this change. Just to clean up the issue tracker I am closing this pull request. If there is motivation to push this change through I'll reopen the pull request.

@miohtama miohtama closed this Jun 2, 2016
@piotr-dobrogost
Copy link

@alephex

But until I hear from a maintainer that they want to merge, I won't waste time on it.

In case you missed it, now you can be sure your time won't be wasted as the new maintainer miohtama wrote I am willing to merge this pull request. above.

@castaf castaf mentioned this pull request Feb 24, 2017
@stevepiercy
Copy link
Member

@ericof @miohtama it appears that @alephex has abandoned this PR and @castaf abandoned theirs in #337. To get this code merged, what needs to be done? Does someone else just fork the branch alephex:form_focus and pick up from there? It seems there's no follow-through by the people submitting the PRs.

We just had a request in IRC for this again. Please let us know. Thank you!

@ericof
Copy link
Member

ericof commented Mar 13, 2018

@stevepiercy I will tackle that on the next few days.

@stevepiercy
Copy link
Member

@ericof there's a person in IRC who also expressed interest in doing the work, but we are not sure of the process or licensing issues, if any. Can you elaborate?

  1. For the process, should the branch in the PR be forked, updated, and submitted in a new PR?
  2. For licensing, is it OK to take someone else's work from the PR submitted here who did not sign CONTRIBUTORS.txt?

There's no rush. It would be good to bring in new contributors. Thank you!

@tisdall
Copy link
Contributor

tisdall commented Mar 14, 2018

@stevepiercy - there's a signature in CONTRIBUTORS.txt in this PR. I think it's okay to use this PR and then just adjust it as needed.

@stevepiercy
Copy link
Member

@tisdall so there is. Yes, we're OK to use this work.

If they had not, someone else could reimplement the work and that could be a legal grey area between the two contributors, but from our perspective it's a non-issue.

@mamalos mamalos mentioned this pull request Mar 22, 2018
@mamalos
Copy link
Contributor

mamalos commented Mar 22, 2018

Hi guys, I've tried to make the least-necessary changes for @alephex's PR to be merged with master in PR #365. Hope it works this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants