Mask & Timepicker - Code Review

[gnarf]

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

[jzaefferer]

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

[gnarf]

@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.

[scottjehl]

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.

[scottjehl]

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]

@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]

@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.

[jzaefferer]

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?

[gnarf]

Regarding a11y - I had done very little research here.

[gnarf]

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

[scottgonzalez]

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