paste operations can bypass the delimiter checks

[Pomax]

As the code analyses letters as they get typed, it fails to detect that it should slice up a string into tags when someone pastes in a string.

For example, with a delimiters array that contains 188 (the comma), normally people can type food, cake, chocolate and get three tags, but pasting the string "food, cake, chocolate" will bypass the detection and end up as a single tag with commas in it.

[Pomax]

Clicking outside of the input also bypasses creating a tag, so adding an onblur behaviour to turn the outstanding text into a tag might be a good idea, too

[i-like-robots]

Interesting thought, I can definitely see the flip side happening when somebody desperately needs commas!

[i-like-robots]

I've had a think and a chat and I reckon this functionality should be handled by the handleAddition callback of the implementation. The reason being that if you do not have the comma set as a delimiter key (which it isn't by default) then things can get funky, quickly!

We use the component in the office here in a few places to attach concepts to content, and a quick look at the 85,000 or so concepts in use shows commas are prevalent.

Fortunately it's pretty much a one-liner these days:

// before
handleAddition (tag) {
  const tags = [].concat(this.state.tags, tag)
  this.setState({ tags })
}

// after
handleAddition (tag) {
  const split = tag.name.split(/,\s*/).map((name) => ({ name }))
  const tags = [].concat(this.state.tags, split)
  this.setState({ tags })
}

[i-like-robots]

Before I close this issue, I did have a mess around with String.fromCharCode to see if this can be done automagically within the component but of course JS key codes and unicode points don't match up =[

CC #81

[Pomax]

where I say "commas", I'm really talking about "anything in delimiters". Our codebase disallows commas in tags, they are explicitly tag delimiters (think stackoverflow rules: spaces are fine, commas are not), so if I paste a string with commas that paste should be cut up into tags. Similarly if you paste a slew of words seprated by tabs, you want to split up into tags, not stored as one tag with actual \t in them =)

The issue really is more about "if text is pasted, and there are characters in that paste that are supposed to be delimiters, tag processing should kick in".

[i-like-robots]

I suppose this could have to be an extra option, or change the existing option to a map (keyCode => string)

[Pomax]

only as "option to turn it off" surely? If you've set up a tag input with delimiters, you've explicitly said "these characters are NOT allowed in tags", so pasting them in should not lead to them making it into tags, they should trigger the same tag parsing as typing that string manually letter by letter?

[i-like-robots]

I mean an extra option to specify the characters. AFAIK mapping the current delimiters option of key codes (or a KeyboardEvent.code 🙂) to a literal "," is not possible.

[Pomax]

Ah, fair point, why we decided that "implementation-dependent" codes were fine back when we invented keyboard events I will never know.

Would it make sense to add something like this?

handleAddition (tag) {
  let tags = this.state.tags;
  if (this.props.processAddition) {
    tags = this.props.processAddition(tag)
  } else {
    tags.push(tag);
  }
  this.setState({ tags })
}

Then people with custom delimiter lists can also add a custom handler that can do "the right thing" based on their own list of delimiters:

render() {
  return <ReactTags
    tags={ this.state.prefabTags }
    delimiters={ KNOWN_DELIMITERS }
    processAddition={ this.safifyTags }
  />;
}
safifyTags(input) {
  if (input.indexOf(blah) === -1)  return [input];
  return input.split(blah).map(v => v.trim());
}

[i-like-robots]

I can see some value advocating a pattern, but if the parent component is already in control of how tags are added (the handleAddition is not a method implemented inside this component!), would such an option add much extra?

[Pomax]

It's not? It's only in control of "tags we already know about in our system at this point in time" mostly for case folding purposes, e.g. we have a known tag IoT, a user types iot, we switch it for the known-case version instead. Whether they type that by hand (in which case it's easy to catch) or through a paste from some notepad or whatever they had open, or another webpage, etc, that functionality still needs to kick in, and the parent is not responsible for tag list creation, although it can be responsible for passing in a tag validation function like the one above (say, passing in some utils.tagvalidator, which the parent doesn't care about in the slightest other than "making sure it gets passed in, whatever it does")

[ajmas]

Based the pull-request submitted for issue #87, we can use the onPaste event. A basic handlePaste implementation:

  handlePaste (e) {
    const { delimiters, delimiterChars } = this.props

    e.preventDefault()

    const data = e.clipboardData.getData('Text')

    if (delimiterChars.length > 0) {
      // split the string based on the delimiterChars as a regex
      const tags = data.split(new RegExp(delimiterChars.join('|'), 'g'));
      for (let i=0; i < tags.length; i++) {
        if (tags[i].length > 0) {
          // look to see if the tag is already known
          const matchIdx = this.suggestions.state.options.findIndex((suggestion) => {
            return tags[i] === suggestion.name;
          })

          // if already known add it, otherwise add it only if we allow new
          if (matchIdx > -1) {
            this.addTag(this.suggestions.state.options[matchIdx])
          } else if (this.props.allowNew) {
            this.addTag({ name: tags[i] })
          }
        }
      }    
    }
  }

with the following code added to the <div>:

onPaste={this.handlePaste.bind(this)}

Edit: I have updated the example code to do the same checks that handleKeyDown() does to check whether the tag exist, before adding it. I'll present this as a pull-request if this looks good to everyone and if the fix for issue #87 is accepted. One other thing, if we want to accept a keyword list that already has a tab in it, then we need to add '\t' to the delimeterChars property.

[Pomax]

In terms of having users not reinvent the wheel, it might also be worth offering an onPaste implementation like this along with the component, so that we can show example code like:

import { ReactTags, processBuffer } from "react-tags";
...
  render() {
    return <ReactTags ... onPaste={processBuffer} ... />   
  } 

where the processBuffer function taps into the tag's delimiters and delimiterchars to make sure the conversion from string to array of tag strings works correctly.

[ajmas]

@Pomax If we include the code I added in my comment and then allow for an optional over-ride, would that be an acceptable solution?

[Pomax]

Yep: if there is a way to "enable" this using only what react-tags gives when installed, then that'll sort us right out.

[ajmas]

@Pomax I have made a PR. Let me know if the changes there are okay for you.

[Pomax]

I left a few comments, too, but overall this looks reasonable

[ajmas]

Note that PR #97 now resolves this, but without using the onPaste event.

[ajmas]

In doing the work in PR #97 I realised that we didn't really have a chart indicating what the behaviour expectations should be. The current implementation follows the 'good enough' approach, but there are obvious limitations. Let me know if the chart below, indicating pasted text and outcomes, for when we don't allowNew looks reasonable:

onPaste operation (or any other action which results in field value being updated in one shot)

Thailand, India       --> one tag, India in field
Thailand, India,      --> two tags
Thailand, India, xxx, --> two tags, xxx in field
Thailand, India, xxx  --> two tags, xxx in field
Thailand, xxx, India, --> two tags
Thailand, xxx, India  --> one tag, India in field
India,                --> one tag
India                 --> India in field
xxx,                  --> xxx in field
xxx                   --> xxx in field

The idea here is we drop any delimiter terminated values and carry on processing. The alternative is to stop processing as soon as we don't recognise a candidate tag? The current solution is a kind of hybrid, since it was only afterwards it occurred to me to consider all the possible variations? The perfectionist wants to address them all, the realist suggests it is already a big improvement as is :)

[i-like-robots]

Hmm, I cannot think of any precedent for this behaviour... but I think stopping processing when we find a tag that does not match makes most sense.

I can also see an argument for not enabling the split on delimiter behaviour at all when allowNew is false... the expectation being that options must be selected from the dropdown.

[Pomax]

I'm struggling to see when you would even use "allowNew" but also allow manual entry. If there is a predefined list of tags that must be used, freeform input is absolutely the wrong UI (that's not to say a textfield that accepts individual letters being typed to refine the list is wrong, but the kind of freeform input that allows paste would be a clear UX design mistake)

[ajmas]

@Pomax I am not sure what event you are referring to when you mention 'manual entry'? It is typing, pasting, selecting from the drop down, or something else? In the case of predefined tag list, the the typing acts as a way of filtering, so you can be presented with a list of 3 options, rather than 100, for example. Also, if you don't want to allow typing when there is a predefined lis of tags, then this is probably the wrong widget anyhow?

BTW if there is a case where we don't want to deal with paste, then we could have an option:

handleOnPaste(e) {
   if (props.disablePaste) {
      e.preventDefault();
   }
}

[Pomax]

I'm seeing a thread where two completely different ways of working with tags are seemingly treated as being the same thing, so that's what I'm calling out. To explain the two, incompatible, settings, let me write out the two conflicting interaction models that we get from allowNew semantics:

If there is a fixed list of tags that cannot be modified by user input, then in this users can only ever "pick tags", i.e. their interactions can only ever end up marking specific tags from the known set as being applicable. From the set of possible interactions with a text field, "typing" makes sense: every letter can reduce the set of tags to only those that start with what the user has typed so far, and any letter that does not lead to a tag refinement should simply get ignored: if a set {a,b} is used, and the user types c, that input shouldn't even make it into the text field (by not triggering a state update in the case of a React component), since it does not match any tags. In this setting, "pasting" is not a meaningful action (there is no clear, established, unambiguous way to filter when the input is arbitrary length, and arbitrary content). Important to note is that the authority in this context is the component: it dictates what the user can do.

In contrast to this is the setting where there exists a user-generated list of tags, where the system knows all the current tags in use but tags previously unknown simply get added to the global set of known tags. In this setting, user input is not a filter, and both "typing" and "pasting" are meaningful actions because input does not get filtered for applicability. The authority in this context is the user, their input determines set selection/expansion.

So there are two very different situations in which the user interactions carry vastly different meaning, with different behaviour and semantics (even if the final presentation is the same), and using a simple bool to flip between the two underestimates, or ignores, the fundamental differences. While certainly from a dev perspective having an allowNew is all that is needed, under the hood that property should really lead to something far more drastic:

import { ClosedTagSet, OpenTagSet } from 'tagsets';
const ReactTags extends Component {
  ...
  render() {
    if (this.props.allowNew === false) {
      return <ClosedTagSet {...this.props} />
    }
    return <OpenTagSet {...this.props} />
   }
}

where the two classes share as much code as possible (e.g. by extending the same component for basic functionality and presentation), but differ rather drastically in how they deal with actual user input, since they represent very different kinds of data, with very different input semantics, even if the terminology is virtually identical.

[i-like-robots]

@Pomax I agree with everything you mention. I'm positive however that the two may be reconciled and I believe with a good outcome... though this may require a more drastic rewrite.

Initially this component did not have an allowNew property (though the upstream component it is based did) for the reasons you mention, the "authority" is with the component and that is what suited our use case at the time.

With the allowNew flag on the authority in my opinion - as I mentioned at the top of this thread - should primarily fall to the parent component (the one implementing this). There is certainly work to do to make that easier and clearer and I think this does include some separation of the two behaviours.

I think that the work done so far has been very valuable, and I am hugely grateful to both of you for your time, knowledge and perseverance. If nothing else many difficulties and sticking points have been raised. I'm hoping to finally have some time this week to try and smooth everything together!

[Pomax]

awesome - I'm in the middle of moving (and kind of have been for the last 2 months because it's been a house hunt and lots of bank/lawyer/etc paperwork bs) but should finally have free time again as of next week.