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

Add xslt to remove local formats from metadata. #357

Closed
wants to merge 27 commits into from

Conversation

nlwillia
Copy link

Includes utils.js with local formats removed built from the 847de27 libphonenumber commit.

@nlwillia
Copy link
Author

A couple of tests are going to fail because of local format assumptions. This is proof-of-concept for now.

@jackocnr
Copy link
Owner

Thank you so much for this great idea! Unfortunately I just tried it and for some reason when you have US selected, and you first type a digit, it doesn't add the initial opening bracket. Because of this, the lookahead feature we have to check if we should add a formatting char doesn't work, so we haven't solved the original problem of being able to type the first char of the US placeholder.

@jackocnr
Copy link
Owner

FYI I wrote a test for this case: it loops through all countries and tries typing the first char from the placeholder for that country and then checks if the input contains that char. It currently fails for 49 countries - all take an open bracket as the first char.

If you want to try it, insert this code in src/spec/tests/options/autoFormat.js on line 15:

describe("empty input, init plugin with autoFormat and nationalMode enabled", function() {

    beforeEach(function() {
      input = $("<input>");
      // must be in DOM for focus/keys to work
      input.appendTo($("body"));

      input.intlTelInput({
        autoFormat: true,
        initialCountry: "us",
        nationalMode: true,
        numberFormat: "FIXED_LINE" // for the sake of placeholders: not all countries have mobile numbers
      });
    });

    it("typing first char of placeholder adds that char", function() {
      var countryData = $.fn.intlTelInput.getCountryData();
      for (var i = 0; i < countryData.length; i++) {
        var placeholder = intlTelInputUtils.getExampleNumber(countryData[i].iso2, true, intlTelInputUtils.numberType.FIXED_LINE);
        triggerKeyOnInput(placeholder[0]);
        expect(getInputVal()).toEqual(placeholder[0]);
        input.val("");
      }
    });

  });

@nlwillia
Copy link
Author

Hmm. This may be a case of different expectations, but I think I see where you're coming from.

intl-tel-input currently doesn't accept typed grouping characters at all that I can tell. The isAllowedKey definition in the plugin only allows a digit or plus. So the value is only going to have brackets when the formatter decides it's time to add them, and it only does that once it's decided on a likely pattern which requires some input. Typing a 10-digit US number (with the local format removed from the metadata), I only see parens after I've typed the 3-digit area code. If I try to type (, it just flashes the error style and ignores the keystroke.

An interesting wrinkle is that if I type the country code prefix +1 then the formatter chooses a different ultimate format than it does if I omit it. +1 201-555-5555 instead of (201) 555-5555. That is probably technically correct since one is the international format and the other is the national format, but it does raise the point that if someone types the country code and then launches into a national format, the format may not be as expected even though the library can ultimately figure out what it is. An example "au" number typed as 0412345678 would format as 0412 345 678. But the E.164 version omits the leading 0 and formats as +61 412 345 67. If you just give it 412345678 then the library recognizes the number, but the formatter never figures it out. (This is based on their demo page.)

So, ignoring all of this for a moment, the point I think you're making is that someone seeing the default initial example format as placeholder text and trying to mimic that is going to end up having some of their keystrokes blocked, and not see the formatter start to pull its weight with their input until a few digits in. I've been focused on confusing alternative formats, but even if those are removed, there is still potentially a difference between what the user is trying to type, what they're allowed to type, what's suggested that they type and what the formatter thinks they are typing.

Depending on how strongly you feel about a formatting discrepancy relative to the placeholder, I don't know that the AsYouType formatter is ever going to produce a perfect result. You could write parsing code to temporarily allow grouping characters until the formatter caught up, but at a certain point you have to consider whether a conventional input mask (generated from the example) couldn't accomplish both the input structuring and formatting without needing any per-keystroke calls out to the formatter. Handling the country code edge case might still be tricky. And ultimately, a mask is only going to reflect the format you get back when you ask for an example. If a country has multiple possible formats depending on the number then a fixed mask could still end up being wrong for what the user is trying to type. Maybe I shouldn't give the libphonenumber folks such a hard time. :-)

I still think that the formatter does a pretty good job when it's not going off on rabbit trails of formats that will never produce a full number, but there could be a better compromise out there. I'll do some more prototyping as time permits and see if anything else looks promising.

@jackocnr
Copy link
Owner

@nlwillia thanks again for your thoughts. Yer the whole issue for me was that the user couldn't enter the first char from the placeholder, and there is a chance they might just give up at that point. Even if only 1 in 100 people drop out at the first char, I don't think it's worth losing that one user just so that the other 99 get a slightly prettier experience.

Nathan Williams added 3 commits February 5, 2016 11:33
If an otherwise disallowed character matches what the placeholder
suggested at that position and the formatter still doesn't know what the
value should look like, permit the user to type it.
@nlwillia
Copy link
Author

nlwillia commented Feb 5, 2016

What do you think about the following as a compromise? Acknowledging that the placeholder example (and this comes from within libphonenumber itself) is derived from the fixedLine metadata format which may not represent every possible format variant supported by the AsYouTypeFormatter, the placeholder is still what the user sees and where there's going to be the biggest impediment if they can't type to match it. So during the early typing phase when the AsYouTypeFormatter is characteristically still waiting on more digits before it chooses a format, we can compare a character that would otherwise be disallowed against the placeholder example at that position and permit it. This is only done while the formatter is still undecided (detected by comparing its output to cleaned input); after it starts doing its thing, we render that version exclusively.

The ideal for interactive input would probably be some kind of adaptive mask that would start with an example format, render grouping characters eagerly and switch between candidate mask formats as the user typed, but I don't think it's possible to do something like that with libphonenumber as it exists today. You'd need a way to project a format-specific example from the partial number the formatter thought it was parsing, and even after weeding out local formats, there could still be country-specific considerations over which examples were best to suggest for different inputs--which is something that if it ever existed would belong upstream.

There may still be some edge cases, but combined with the build step to remove local formats (which from my reading of their definition seems safe enough to do), I hope that the placeholder exception closes the loop enough to make automatic formatting an option that's still worth considering.

This still needs tests, but I'll wait to do that until I hear your thoughts.

@jackocnr
Copy link
Owner

jackocnr commented Feb 5, 2016

@nlwillia I admire your tenacity, but I'm sorry but this is going too far down the rabbit hole of trying to hack libphonenumber to do something it's not designed to do. First we're hacking AsYouTypeFormatter, then we're hacking the XML formats, and now you want to throw in different stages of formatting based on another source (the example numbers we use for placeholders) - we have to draw a line.

I have decided to remove the autoFormat feature until either libphonenumber decides to support our use case for their AsYouTypeFormatter (that would mean 1. formatting from the first digit instead of waiting until the third, and 2. allowing an option to specify the desired format e.g. full international format), or we find another lib that does the job.

@nlwillia thanks for all your effort and sorry we couldn't make this work. I hope you understand my decision.

@fruwe
Copy link

fruwe commented Feb 19, 2016

Hi there. I added a new issue #372 since removing bower caused the upgraded version to fail.

Sorry, but why is it, that bower needed to be removed in this issue/pull request?

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.

3 participants