html-next / ember-hammertime

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

ember 3.11: hammertime triggers syle binding warning #90

Open st-h opened 5 years ago

st-h commented 5 years ago

As already reported to the ember repo here, when ember > 3.11 is used with this addon, code like

<span class="trigger" {{action (toggle '_expanded' this)}}>

will trigger the following warning:

WARNING: Binding style attributes may introduce cross-site scripting vulnerabilities; please ensure that values being bound are properly escaped. For more information, including how to disable this warning, see https://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes. Style affected: "touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;"
eriktrom commented 5 years ago

@st-h - I believe we're going to deprecate this addon - discussion on that is pending here https://github.com/html-next/ember-hammertime/pull/86 (more context available here, @runspired or perhaps myself need to ensure that comments in https://github.com/html-next/ember-hammertime/issues/74 no longer hold true though, for background.)

And I agree with the issue u link to (https://github.com/emberjs/ember.js/issues/18193) - this warning probably should have begun happening before ember 3.11.

given ur using the {{action helper, as a modifier, u can get around this (w/o understanding the root cause in ember) by setting this line to false: https://github.com/html-next/ember-hammertime/blob/7c50bdfe483d83ff291063b649c0aa6036134e45/addon/mixins/touch-action.js#L37. U can also add onclick={{action as well, which is likely easier. The latter will run the ast transformer and prevent style binding at runtime (the former, would remove the touch-action: ... property altogether, but span is not a 'focusable input' element and ios will thus not add 300ms to create a fake hover state (on older iphones, if ur targeting newer iphones only, u could just remove the addon completely per discussion via the first link in this comment, fyi).

(don't quote me verbatim on any of this, we're talking a couple years here in terms of human memory, but I believe those statements are correct, to best of my knowledge, w/o looking again more closely)


leaving this for more context, although i edited the above, so this is mostly to re-iterate why it's like this and why {{on in ember octane is a better choice than {{action -- this addon doesn't support {{on atm though

--

whats happening is that for tags that are not focusable (aka, input tags) and that don't have onclick= (aka, only use a positional {{action modifier, we added this feature a while back for consistency due to confusion that consumers of this lib had due to the different ways {{action could be used.

st-h commented 5 years ago

@eriktrom thanks a lot for your reply. Honestly, I wasn't adding this addon because I was noticing delays but as a mere measure of preventing such things to show up unexpectedly. I have read through #74 back then and a few stackoverflow questions etc. but my conclusion pretty much was that there still are cases where hammertime would be helpful - but I never took the time to really look into this and its details.

One thing that just comes to my mind is that quite some time back, I stripped all css hover styles on mobile, because they were causing clicks to delay or seemed to introduce the need to click twice (and there is not really a good way to make use of hover at all using touch screens). Personally, I think I will remove this addon for now and see if I run into anything that brings up a need to be fixed. Thanks again for the clarifications.

eriktrom commented 5 years ago

@st-h - np -- also interesting about the hover css state findings u mention, thanks for sharing.

st-h commented 5 years ago

@eriktrom pretty simple thing: I just wrap :hover selectors in a media query:

@media (hover: hover) {
  h1:hover {
    color: red;
  }
}

Afaik (even though this was a few years ago) - when iOS safari notices that there is a hover style defined for an element, the first tap activates the hover style (not sure anymore if there just is a delay or a second tap is needed to trigger the actual click). At least removing hover selectors improved the mobile experience for my app. However, I do not really know anything if it still is needed today - I just kept it as it does not actually make much sense to me to define hover styles where hover is not supported. It just seems like a reasonable thing to do.

ref: https://docs.w3cub.com/css/@media/hover/