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

bs-form components are getting cursor: pointer #31

Open
gustaff-weldon opened this issue Oct 13, 2016 · 24 comments
Open

bs-form components are getting cursor: pointer #31

gustaff-weldon opened this issue Oct 13, 2016 · 24 comments

Comments

@gustaff-weldon
Copy link

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
Copy link
Author

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

@kepek
Copy link

kepek commented Oct 25, 2016

👍 @gustaff-weldon
🔔 @runspired

@davearel
Copy link

davearel commented Nov 3, 2016

+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
Copy link

kepek commented Nov 3, 2016

@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
Copy link

kepek commented Nov 3, 2016

@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
Copy link
Collaborator

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
Copy link
Author

@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

const TouchActionProperties = 'touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;';

@davearel
Copy link

davearel commented Nov 4, 2016

@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
Copy link
Collaborator

runspired commented Nov 4, 2016

@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
Copy link

davearel commented Nov 4, 2016

@runspired fair.

@davearel
Copy link

davearel commented Nov 4, 2016

@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?

const TouchActionProperties = 'touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;';

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
Copy link

anulman commented Dec 20, 2016

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

@runspired
Copy link
Collaborator

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

@Techn1x
Copy link

Techn1x commented Sep 20, 2017

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
Copy link
Member

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

@Techn1x
Copy link

Techn1x commented Sep 20, 2017

@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
Copy link
Member

@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
Copy link

Techn1x commented Sep 22, 2017

@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
Copy link
Member

@Techn1x anytime you have some time is fine!

@Techn1x
Copy link

Techn1x commented Jan 6, 2018

@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)
Techn1x@6bf8814

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
Copy link
Member

@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
Copy link

adambedford commented Jan 12, 2018

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

@RobbieTheWagner
Copy link
Member

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? #44

@RobbieTheWagner
Copy link
Member

cc @Techn1x @adambedford ^

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

No branches or pull requests

8 participants