knownasilya / ember-toggle

Checkbox based Toggle Switches for Ember
http://knownasilya.github.io/ember-toggle/
MIT License
112 stars 52 forks source link

Adds cursor: pointer; to all inputs #105

Open adambedford opened 6 years ago

adambedford commented 6 years ago

I'm seeing an inline style cursor: pointer; added to input elements (along with touch-action: manipulation; -ms-touch-action: manipulation;), which isn't desirable as it breaks convention for fields that accept user input.

I'm pretty sure the source of this inline style is ember-toggle and this seems like a bug.

knownasilya commented 6 years ago

What is the style that's introducing this in your app?

We don't use any generic selectors, everything is behind a specific toggle class. The css is here https://github.com/knownasilya/ember-toggle/tree/master/vendor/ember-toggle

lolmaus commented 6 years ago

@knownasilya, he's talking about inline styles, the ones set from JS.

knownasilya commented 6 years ago

I don't think we have any in v5.

lolmaus commented 6 years ago

@adambedford, can you repro?

adambedford commented 6 years ago

here's an example pulled from the Chrome inspector. It happens with input elements as well. I'm not 100% positive this is coming from ember-toggle but I can't think of where else it'd be coming from at the moment

<textarea 
  style="touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;" 
  placeholder="Add a private note..." 
  id="ember1260-field"
  class="form-control ember-view">
</textarea>
lolmaus commented 6 years ago

This addon has nothing to do with textareas. Try running your browser with all extensions disabled (there should be a CLI flag for that).

knownasilya commented 6 years ago

Maybe ember-gestures does something? This addon does use that addon..

danconnell commented 6 years ago

This is happening because ember-hammertime, a dependency of this addon, is applying those styles

lolmaus commented 6 years ago

@danconnell Thank you!

Closing this in favor of upstream issue: https://github.com/html-next/ember-hammertime/issues/31

danconnell commented 6 years ago

FYI @adambedford ember-hammertime doesn't have working configs, as documented in that bug, so you're best off downgrading to ember-toggle@5.0.2 until ember-hammertime is fixed.

knownasilya commented 6 years ago

@rwwagner90 just including you since you are active in the ember-hammertime repo.

adambedford commented 6 years ago

thanks @danconnell Unfortunately I need the material style so I can't downgrade. I'll have to wait this one out I suppose.

RobbieTheWagner commented 6 years ago

@knownasilya yeah, the issue in ember-hammertime was being worked on by @Techn1x but he said he was not having much success. What is the desired fix here? If we make it more configurable, can ember-toggle disable some of these? Just want to know exactly what we want before I look into a fix.

knownasilya commented 6 years ago

Basically, it makes all inputs have a pointer on hover. Not sure we should be doing anything from our side. I think the desired effect is that only items that need touch support via hammertime have the pointer. Maybe the API there should be opt-in for elements. I'd be fine with having to explicitly opt ember-toggle into touch.

RobbieTheWagner commented 6 years ago

@knownasilya do we need ember-gestures for this addon?

knownasilya commented 6 years ago

It might be overkill, we just need swipe/drag support (so toggles feel native), maybe there is a simpler way to do that.

lolmaus commented 6 years ago

Using ember-gestures was the simplest thing implementation-wise. Native drag events are pretty quirky.

knownasilya commented 6 years ago

Yeah, which is sad :(

lolmaus commented 6 years ago

We're using this as a workaround until https://github.com/html-next/ember-hammertime/issues/31 is resolved and included into ember-gestures:

input[type="text"], textarea {
  cursor: text !important;
}
RobbieTheWagner commented 6 years ago

I just released ember-hammertime 1.3.0, with configuration options that should allow us to disable these things. Is anyone interested in helping get this into ember-gestures and over the finish line, so we can fix this here?

adambedford commented 6 years ago

@rwwagner90 How do we get your fix into ember-toggle to resolve this issue?

RobbieTheWagner commented 6 years ago

@adambedford I don't think anything needs to be done here. I think ember-hammertime is installed directly in your app via a blueprint, so you should just have to configure it.

adambedford commented 6 years ago

Thank you @rwwagner90 Do you have guidance on what the ember-hammertime config should be to get rid of the inline styles for all other elements?

RobbieTheWagner commented 6 years ago

@adambedford the configuration is documented in the README https://github.com/html-next/ember-hammertime#configuration

If anything is unclear, please let me know.

knownasilya commented 6 years ago

So something like touchActionSelectors: ['.x-toggle-component']? We can probably document a good default for x-toggle.

RobbieTheWagner commented 6 years ago

@knownasilya that might work. All of the config options need to be set though. The default is to apply the styles to everything with an action all buttons, etc. and if we want to turn all that off, it needs to be something like:

EmberHammertime: {
    touchActionOnAction: false,
    touchActionAttributes: [''],
    touchActionSelectors: ['.x-toggle-component'],
    touchActionProperties: 'touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;'
  }

We don't want to mess up ember-hammertime for the consuming app by assuming any config, but we should document how to disable it.

adambedford commented 6 years ago

I haven't been able to get this working. I tried the config suggested by @rwwagner90 and even installed ember-hammertime in my app directly (it doesn't seem to be installed by ember-toggle via blueprint, but is instead a dependency)

RobbieTheWagner commented 6 years ago

@adambedford I thought we didn't have a direct dep on it here, but it appears I was wrong. We need to update the deps here to use ember-hammertime 1.3.0.

adambedford commented 6 years ago

@knownasilya @rwwagner90 Has there been any progress on this by chance?

RobbieTheWagner commented 6 years ago

@adambedford I just opened a PR to update ember-hammertime here.

adambedford commented 6 years ago

I've updated to the master branch of this library and have the following configuration in environment.js but I'm still seeing the unwanted styles set on almost every element in my application

EmberHammertime: {
      touchActionOnAction: false,
      touchActionAttributes: [''],
      touchActionSelectors: ['.x-toggle-component'],
      touchActionProperties: 'touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;'
    }

Any pointers on what I need to do to solve this?

knownasilya commented 6 years ago

We might have to set this config in ember-toggle.. I'll give it a try tomorrow.

RobbieTheWagner commented 6 years ago

@adambedford @knownasilya are we using the touch action mixin anywhere or just allowing the default behavior?

knownasilya commented 6 years ago

@adambedford give master another try. I merged another version bump change.

knownasilya commented 6 years ago

If that doesn't work, there might be some issue that doesn't propogate the config to the hammertime addon. We might make it an install "add npm package" feature instead of bundling together.

RobbieTheWagner commented 6 years ago

@knownasilya I don't think the config is passed down because it's a nested dep. Definitely would like to explore how we might fix this.

adambedford commented 6 years ago

Could the configuration simply be added to ember-toggle?

RobbieTheWagner commented 6 years ago

@adambedford I don't think we want that. We want users to be able to change the ember-hammertime config directly.

adambedford commented 6 years ago

Fair enough. Any idea how we fix this?

RobbieTheWagner commented 6 years ago

@adambedford I would need to play with it. The config is definitely overridable in ember-hammertime, itself, so we would need to investigate if we can pass it down from this addon or if we need to have ember-hammertime not be a dep and get installed via a blueprint. Would you be interested in working on a PR to explore these options?

adambedford commented 5 years ago

I wasn't able to dedicate any time to this unfortunately. Has anyone had a chance to make progress?

adambedford commented 5 years ago

Here's a quick fix derived from @lolmaus above:

[style^="touch-action: manipulation"]:not(.x-toggle-btn, button, a) {
  cursor: inherit !important;
}
knownasilya commented 5 years ago

I'll have a look soon. Thanks for bringing it up again.

RobbieTheWagner commented 5 years ago

I think we probably just don't need hammertime anymore, these days. What do you think @knownasilya?

knownasilya commented 5 years ago

@rwwagner90 yeah, seems a bit overkill. Would have to add slide support manually though.

RobbieTheWagner commented 5 years ago

@knownasilya is it not possible to use ember-gestures without ember-hammertime?

knownasilya commented 5 years ago

Oh, totally not sure. This is the only experience I have with either.

knownasilya commented 5 years ago

@adambedford give master a try. I've removed hammertime and it seems to have done the trick. cc @rwwagner90

If it works for you, then I'll publish a release.