Mask & Timepicker - Code Review #494

Closed
wants to merge 118 commits into
from

Conversation

Projects
None yet
7 participants
Owner

gnarf commented Oct 11, 2011

Not much stopping me from wanting to pull this into master... We need a code review, and also need some demos pages.

gnarf and others added some commits Jul 27, 2011

Mask: Updating regexp to include ö and å (etc), also making sure inpu…
…ts have focus in the tests to stop an odd sometimes bug
Merge branch 'master' into mask
Conflicts:
	tests/unit/all.html
Mask: Adding left key event to also highlight each field in multi-cha…
…racter fields like the right arrow was doing

Conflicts:

	ui/jquery.ui.mask.js
Mask: Fixing a focus caret position issue, adding _caretSelect to sel…
…ect a multi-byte field, canceling the caret on focus timeout in blur to prevent a possible loop when tabbing quickly between masked inputs
Owner

gnarf commented Sep 4, 2012

/botio test

From: jQuery Bot.io


Received

Command cmd_test from @gnarf37 received. Current queue size: 0

Live output at: http://swarm.jquery.org:8001/927662967145a28/output.txt

From: jQuery Bot.io


Failed

Full output at http://swarm.jquery.org:8001/927662967145a28/output.txt

Total script time: 0.32 mins

Owner

gnarf commented Sep 4, 2012

/botio test

From: jQuery Bot.io


Received

Command cmd_test from @gnarf37 received. Current queue size: 0

Live output at: http://swarm.jquery.org:8001/61728cf2e41c9e5/output.txt

From: jQuery Bot.io


Failed

Full output at http://swarm.jquery.org:8001/61728cf2e41c9e5/output.txt

Total script time: 0.14 mins

Owner

gnarf commented Sep 4, 2012

/botio test

From: jQuery Bot.io


Received

Command cmd_test from @gnarf37 received. Current queue size: 0

Live output at: http://swarm.jquery.org:8001/7e1f8788e6791c0/output.txt

From: jQuery Bot.io


Failed

Full output at http://swarm.jquery.org:8001/7e1f8788e6791c0/output.txt

Total script time: 0.53 mins

Owner

gnarf commented Sep 4, 2012

/botio test

From: jQuery Bot.io


Received

Command cmd_test from @gnarf37 received. Current queue size: 0

Live output at: http://swarm.jquery.org:8001/96980b83878a79d/output.txt

From: jQuery Bot.io


Failed

Full output at http://swarm.jquery.org:8001/96980b83878a79d/output.txt

Total script time: 1.42 mins

Failed running:

Owner

jzaefferer commented Oct 22, 2012

@zjs has been working on this during the summit and beyond. Still a few open issues.

Owner

gnarf commented Oct 22, 2012

@zjs thanks again for picking this up - I'll leave it in good hands 👍

I still want to rename timepicker to timemask - seemed others were in agreement.

Contributor

scottjehl commented Oct 22, 2012

Hey all,

I meant to bring this up at Dev days but didn't end up having time.

I was curious if there's a general strategy in place for how this component will end up presenting itself from an a11y standpoint.

I did a quick test and currently, the masked input is very loud and confusing when used in a screenreader, like VoiceOver. When you focus on the input, it reads aloud ("underline underline underline underline underline underline underline underline underline underline "). As you continue to use it, I think it gets even more confusing. I'm not an everyday screenreader user, but this seems like a valid issue with masked input patterns in general, unless there are known ways to avoid the issue that are not yet implemented?

I'm not sure this situation is avoidable when manipulating a form control's value, which raises the question of whether that practice is a good idea, or whether an alternative approach might be needed that only affects sighted users (I've no idea what that may be).

Thanks for your time!
Scott

On Oct 22, 2012, at 4:45 PM, Corey Frang wrote:

@zjs thanks again for picking this up - I'll leave it in good hands

I still want to rename timepicker to timemask - seemed others were in agreement.


Reply to this email directly or view it on GitHub.

Contributor

scottjehl commented Oct 22, 2012

In addition, I'm curious how this would play out on mobile devices. I haven't done any testing there, but wondered if the behavior is workable with touchscreen keyboards and their event handling (even if not designed to be mobile-optimized).

zjs commented Oct 22, 2012

@gnarf37 - I believe @artzstudio (who was looking at timepicker during the summit) and @jzaefferer discussed extracting the notion of a more general purpose spinnermask that contained the logic to build a mask containing spinner-controlled numeric ranges and using that as a foundation for timemask.

This would equate to extracting the makeBetweenMaskFunction function and some of the spinner logic from timepicker into a widget that took a string based on the mask format string with additional constructs to express the numeric range. That is, instead of renaming timepicker as it exists today to timemask, someone could split the general-purpose code into a spinnermask and the time-specific code into a timemask.

Once implemented, timemask would basically be a thin wrapper which focused on the actual time-related constructs (i.e. the logic currently expressed in maskDefinitions) by enumerating sensible format strings (e.g. the 12-hour format string might look something like [1-12]:[0-59]:[0-59] tt (where [1-12] is shorthand for the makeBetweenMaskFunction and tt defined as it is today).

It seems like this general widget might be easily re-used for other purposes (height: [0-9]ft [0-12]in, duration: [0-99]h? [0-59]m, ipv4 address: [0-255].[0-255].[0-255].[0-255], etc.). (Essentially any case where you might want multiple spinners in the same input.)

zjs commented Oct 22, 2012

@scottjehl: Good point about a11y. I know I haven't looked at that at all, but perhaps we could leverage some of the placeholder string-related handling. I'm not sure what investigation (if any) @gnarf37 has already done in that space.

As for mobile devices, both Chrome and Safari on iOS seem to play nicely with the current implementation. I would expect others would as well, but it's certainly something we should validate.

Owner

jzaefferer commented Oct 22, 2012

Chrome on Android 4.1 works well. On the stock browser the phone number demo keeps switching back to the regular keyboard from the number keyboard, which is really annoying, but still usable (both with Swiftkey and stock keyboard).

For screenreaders it would probably be preferable to have the screenreader ignore the mask and read only the actual input. Could be hard to achieve, as we manipulate the input's value directly.

Maybe @hanshillen has some input on that?

Owner

gnarf commented Oct 22, 2012

Regarding a11y - I had done very little research here.

Owner

gnarf commented Oct 22, 2012

@zjs @artzstudio @jzaefferer - 👍 🍰 - spinnermask sounds pretty awesome.

Owner

scottgonzalez commented Apr 11, 2013

Closing until we're ready to do the full review after the 1.11 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment