html-next / ember-hammertime

TouchAction (aka "fastclick") Support for Ember Applications
MIT License
56 stars 23 forks source link

bs-form components are getting cursor: pointer #31

Open gustaff-weldon opened 8 years ago

gustaff-weldon commented 8 years ago

We are using Ember Bootstrap project forms (http://kaliber5.github.io/ember-bootstrap/api/classes/Components.FormElement.html) It defines action as a handler for submit. When combined with hammertime a whole form gets cursor: pointer

Is there a way to disable the {{action}} matching in hammertime?

gustaff-weldon commented 8 years ago

@runspired what do you suggest? I think some exclude pattern would be great to avoid those?

kepek commented 8 years ago

👍 @gustaff-weldon 🔔 @runspired

davearel commented 8 years ago

+1

cursor: pointer is a browser specific bug fix, but it's getting put on every component for me. There needs to be a way to disable this. At least as an inline-style. That is too heavy and difficult to override.

kepek commented 8 years ago

@davearel, you can ignore specific components if you want:

// your-app/app/components/komponent.js

import Ember from 'ember'

export default Ember.Component.extend({
    ignoreTouchAction: true
})
kepek commented 8 years ago

@davearel, small workaround for these who wants to ignore some specific selectors:

// your-app/app/initializers/customize-hammertime.js

import Ember from 'ember'
import ENV from 'your-app/config/environment'
import TouchActionMixin from 'ember-hammertime/mixins/touch-action'

const {
    defineProperty
} = Ember

const config = ENV['EmberHammertime']

// I could not add new config properties to `TouchActionSupport` htmlbars-plugin
// in `ember-hammertime`, so I have add this through the ENV config manually.
const TouchActionExcludedSelectors = config.touchActionExcludedSelectors || []

export function initialize() {
    TouchActionMixin.reopen({
        touchActionExcludedSelectors: TouchActionExcludedSelectors,

        init() {
            this._super(...arguments)

            let isExcluded = this.touchActionExcludedSelectors.indexOf(this.tagName) !== -1

            if (isExcluded) {
                defineProperty(this, 'touchActionStyle', null)
            }
        }
    })
}

export default {
    name: 'customize-hammertime',
    initialize
}
// your-app/config/environment.js

var ENV = {
    // ...
    EmberHammertime: {
        touchActionSelectors: ['button', 'input', 'a', 'textarea'],
        touchActionExcludedSelectors: ['form'],
        touchActionProperties: 'touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;'
    }
}
runspired commented 8 years ago

So there should be a story for this, but I suspect one thing I should do is simply special case form in the AST walker.

Sorry for taking so long to reply.

Hammer-time is in the process of being upgraded and the style rule is going to move to being a single class instead of the style string, should make overrides easier.

gustaff-weldon commented 8 years ago

@davearel This is not browser specific. The addon just adds inline style touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer; for matched components, see https://github.com/html-next/ember-hammertime/blob/6292fad841be5b89bbdc9ad0fa4315d7565f0411/addon/mixins/touch-action.js#L13

davearel commented 8 years ago

@gustaff-weldon Then the readme is misleading https://github.com/html-next/ember-hammertime

Regardless, it's only for mobile. Yet it's affecting the user experience on desktop.

runspired commented 8 years ago

@davearel it's not only for mobile. It's hard to remember sometimes but there is no longer a true distinction between phone / tablet / desktop. All can have a mouse or a touch screen.

Cursor:pointer has always been recommended for touch devices (which are largely impossible to detect) and since this operates at build time even if we could detect there wouldn't be any knowledge.

That this adds to an entire form for a submit action is a bug, which will get fixed. That's a kind of action that should have been filtered out already.

As for configuration, runtime configuration is pretty easy on a per component or component class basis (and since this is a component and not an HTML tag you could do it at runtime). But despite that, I am working to make it even easier.

Lastly, what this lib does is fairly simple, the code surface area is small, and PRs to fix bugs such as this are always welcome!

davearel commented 8 years ago

@runspired fair.

davearel commented 8 years ago

@gustaff-weldon @runspired Looking over this code, I see that it's only adding the inline styles for components with a root-level click handler; correct? https://github.com/html-next/ember-hammertime/blob/6292fad841be5b89bbdc9ad0fa4315d7565f0411/addon/mixins/touch-action.js#L13

I only have a click handler on two components, but these styles are being placed on every component for me.

Have you seen this before? Could it be a third party applying a click method on every component?

anulman commented 7 years ago

@runspired heads-up, this bug also affects elements with disabled styling, e.g. button:disabled { cursor: not-allowed; }

runspired commented 7 years ago

@anulman that would be a separate issue :P but one also fixed by an upcoming change :)

Techn1x commented 7 years ago

I've just encountered this issue today, any progress towards this? or should I override cursor: pointer for my bs-form elements? (specifically I want the default cursor:text to appear for input elements, which I have had to add an action for to do e.preventDefault)

Some kind of exclusion filter could work well too

Since it actually adds style attributes that are harder to override to a lot of elements, it does seem like this addon is a bit invasive at times, a class definition would certainly help

RobbieTheWagner commented 7 years ago

@Techn1x you can configure this addon https://github.com/html-next/ember-hammertime#configuration

Techn1x commented 7 years ago

@rwwagner90 Awesome! Except now that I look at that and configure it, I've noticed that the configuration doesn't seem to be applied correctly? I've set the following;

var ENV = {
  // ...
  EmberHammertime: {
    touchActionSelectors: ['button', 'a'],
    touchActionProperties: 'touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;'
  }
}

And while that has stopped the properties from being applied to my input elements, it's still being set to th, tr and some div elements (within the same table)... is this a bug? Specifically, I'm using ember-light-table

As far as I can tell, ember-light-table doesn't use ember-hammertime. Are there any other conditions that cause the properties to be applied to an element?

RobbieTheWagner commented 7 years ago

@Techn1x I believe it also automatically adds it to any elements with onclick or action on them. https://github.com/html-next/ember-hammertime/blob/master/htmlbars-plugins/touch-action.js#L68

We should likely also make this configurable. I would be open to PRs for it, if you are interested 😄

Techn1x commented 7 years ago

@rwwagner90 ah! thanks. Normally I would jump at the opportunity to do a PR but I'm going on holidays for the next 10 days, I'll take a look when I get back though :)

RobbieTheWagner commented 7 years ago

@Techn1x anytime you have some time is fine!

Techn1x commented 6 years ago

@rwwagner90 3 months later and I found some time to give this a go, to provide the ability to configure if action or onclick elements get the hammertime style applied.

This is my attempt at it (didn't make a PR because it's not really working at the moment) https://github.com/Techn1x/ember-hammertime/commit/6bf881413c7a379772e2b4d7545ec4a920e2b7bb

At first it seemed pretty simple, I added an extra config option to the code in htmlbars-plugins/touch-action.js called touchActionAttrSelectors but then I noticed there's also some very similar code in addon/mixins/touch-action.js, so I added the logic there too.

Then came testing the changes, but I couldn't seem to get the hammertime configuration to work at all. As a test I tried blanking everything (so theoretically, ember-hammertime shouldn't make any changes to my elements)

var ENV = {
  // ...
  EmberHammertime: {
    touchActionSelectors: [],
    touchActionAttrSelectors: [],
    touchActionProperties: ''
  }
}

But still the standard ember-hammertime style was being applied to my elements. Am I doing something wrong?

RobbieTheWagner commented 6 years ago

@Techn1x I would need to know more about how you were testing things. I would suggest making a PR, so that I can look at your changes and we can work through everything together.

adambedford commented 6 years ago

@kepek Does your initializer work for ignoring specific elements, even if they have a click handler?

RobbieTheWagner commented 6 years ago

I currently have a PR up that allows some additional configuration. It allows disabling adding the touch-action styles to elements that had an action or an onclick which you could not disable before. Can you guys please try it out and see if it works for your cases? If not, can you please tell me which cases are still broken? https://github.com/html-next/ember-hammertime/pull/44

RobbieTheWagner commented 6 years ago

cc @Techn1x @adambedford ^