search

st-h / ember-content-editable

A contenteditable component for ember-cli that just works™
MIT License
66 stars 31 forks source link

Preserve placeholder on initial focus. Make opposite behavior a config option #21

Closed BrianSipple closed 8 years ago

BrianSipple commented 8 years ago

I just started using this add-on in a project (and great job, btw!), and I noticed that when using placeholders, the placeholder will be removed from the ::before pseudo element’s content as soon as the element gains focus. This is contrary to the native behavior for placeholders in input elements (which I made a quick demo for here), which only clears the field when the user begins typing.

With that in mind, I’d like to suggest keeping ember-content-editable consistent with standard placeholder behavior by default -- but then also providing a configuration option for the opposite behavior.

The former change is just a minor tweak to addon.css that accounts for [contenteditable=true]:focus:empty:before when applying the placeholder. The latter change would just be a classNameBinding on the config option with a corresponding CSS rule of its own:

    .ember-content-editable.clear-on-focus[contenteditable=true]:empty:focus:before {
      content: '';
    }

If these are acceptable changes, there’s one other addition I’d like to include here.

Along the way of adding tests, I discovered that PhantomJS doesn’t play too nicely with the :focused state of DOM elements (there’s actually an open bug here), Particularly, it doesn’t respond to the event being triggered on an in-memory Node.

This made headless tests for the ::before content of a focused element altogether impossible -- and it lead to the test for toggling focus when clearPlaceholderOnFocus was true passing in Chrome but failing in Phantom. (Side note: For what it’s worth, I can also confirm success with the behavior of toggling clearPlaceholderOnFocus in a project that's using my local branch).

With ember-content-editable’s primary concern being so DOM-centric, I think it would be a great improvement going forward to set up Travis to run tests in Chrome. I the made changes to travis.yml necessary for doing so — and currently, we’re all green.

chrissloey commented 8 years ago

Great, thanks for the detailed PR 😄. I agree with everything - updated on npm now.