fix: removing a double trigger due to the visual toggle already being surrounded by a label

[webark]

since the visual "toggle" already is a label, having a click handler was causing the sendToggle method to double fire. (one for the change event being triggered by clicking on the label, and one for the click on the div)

Could also remove the label, and leave the click event, and remove the "is input" stuff check.

[knownasilya]

@rwwagner90

[RobbieTheWagner]

@webark this stuff was in place in an attempt to support allowing doing like:

<label>
  {{toggle}}
</label>

But that never really panned out as working. I think without this click, it doesn't work in all browsers, especially IE, as for some reason, they thought it was a good idea to not fire click events on inputs with visibility:hidden when every other browser allows it.

[knownasilya]

@rwwagner90 I think we came to the conclusion that making <label>{{x-toggle}}</label> work is probably not going to pan out. Is that correct?

If so, we could remove this, if we found an alternative to fixing the browser support issue. @webark how were you implementing the toggle, when you came across this issue? Was that in the reproduction repo you sent me earlier?

[webark]

@rwwangner90 @knownasilya I tested it in IE, and it works as expected with this fix and the event isn't getting double triggered in ie as well. The current implementation wasn't working in safari due to the order of the events that where being triggered. In chrome, the click event on the div triggers first, then the on change event. But on safari, the onchange event triggers first then the click event. This order causes the toggle to just toggle back and forth in safari.

Are you saying that you don't want to use the label anymore, and keep the clock event? I can work on that if that's the case.

And not doing anything special. Just a standard toggle with the action defined is enough. {{x-toggle value=model.isPublic onToggle=(action (mut model.isPublic))}}

This is in the simplify branch.

[webark]

and i didn't know you could nest labels like that..

[webark]

ya.. you can't nest labels, so you wouldn't have been able to put the switch inside a label, since the switch is already using a label.

If that was desired, we could take the approach of removing the label in the switch, and triggering the change with js. Let me know if that's preferred..

[knownasilya]

I think we want to keep the labels in the toggle and not support the external label. Mainly because we want to support two labels (not nested).

[webark]

@knownasilya i was meaning the label in the switch component. https://github.com/knownasilya/ember-toggle/blob/simplify/addon/components/x-toggle-switch/template.hbs#L10

[knownasilya]

That's there because it triggers the checkbox when clicked since the checkbox is hidden (it can't be styled like we want). I'm open to an alternative.

[webark]

perhaps the label was from a "css only" attempted solution?

[webark]

@knownasilya yep! which is why with the click event it ends up being triggered twice.

[knownasilya]

Hum. I thought there was a check that prevented a double trigger. I.e. https://github.com/knownasilya/ember-toggle/blob/simplify/addon/components/x-toggle-switch/component.js#L22

[RobbieTheWagner]

@webark @knownasilya yeah, we have like 4 labels here sometimes, and that's definitely bad for accessibility, but it's what we have right now. I know for sure that cases with one label and cases with two labels and the simple toggle versus the contextual component toggle example all behaved inconsistently across various browsers. I think we need a rethink of the whole thing.

The root issue we need to support is 1 label or 2 labels or no labels and ensure that it works in all browsers.

[webark]

@rwwagner90 @knownasilya I put some logging in to show the issue... in chrome you have

click action from x-toggle-switch, current value is true, target is [object HTMLDivElement]
click action from x-toggle-switch, current value is true, target is [object HTMLInputElement]
sendToggle action from x-toggle. new value is false. current value is true. coming from click
sendToggle action from x-toggle. new value is false. current value is false. coming from onchange

in safari you have

click action from x-toggle-switch, current value is true, target is [object HTMLDivElement]
sendToggle action from x-toggle. new value is false. current value is true. coming from onchange
click action from x-toggle-switch, current value is false, target is [object HTMLInputElement]
sendToggle action from x-toggle. new value is true. current value is false. coming from click

[webark]

From my testing and experience. If you want to keep the label in the switch, you can remove the click handler (which was in there before the ie detection).

[webark]

if you take out the click action, all you have is what you need

sendToggle action from x-toggle. new value is false. current value is true. coming from onchange

[webark]

just looked at the styles, and it's currently setup as a "css only" solution with using the :checked + label styles.

[webark]

when the click action was introduced #67, the label had not yet been added to the switch https://github.com/knownasilya/ember-toggle/blob/283946f22f92ab6864ff59262f808e91ce7c08fc/addon/components/x-toggle-switch/template.hbs that was done later with https://github.com/knownasilya/ember-toggle/pull/69

[RobbieTheWagner]

@webark if you remove the click, IE does not work.

[RobbieTheWagner]

@webark so we can potentially make the check only send the click if IE, but IE blocks the default click a label to click and input behavior. I was pretty sure that either clicking the label or clicking the toggle, one or the other, also broke in other browsers without the click.

Perhaps we should create some more robust tests here.

[knownasilya]

Maybe setting up https://saucelabs.com/open-source with https://github.com/johanneswuerbach/ember-cli-sauce

[webark]

@rwwagner90 what version of ie? It worked in all the versions I tested.. 11, 10, and edge

[gtb104]

I've tested this on several browsers; all work as expected.

OSX Chrome 58.0.3029.110 Safari 9.1.1 FireFox 53.0.2

Windows 7 IE 11.0.9600.18638.

[gtb104]

BTW, you should also remove the ember-browser-checker service as that's not used anymore.

[RobbieTheWagner]

@gtb104 can you confirm what sort of examples you tested? Did you do the standard case, with no labels, the contextual component case with 1 label, the contextual component case with 2 labels, and the standard case with both 1 and 2 labels as well?

I believe these hacks were just for certain cases on certain browsers.

[gtb104]

I created a new Ember app, and installed v5.0.0-alpha.4. I then created a new component that has the following template:

{{!-- default --}}
{{x-toggle value=myValue onToggle=(action (mut myValue))}}

{{!-- with labels --}}
{{x-toggle
  value=myValue2
  showLabels=true
  onLabel='Enabled'
  offLabel='Disabled'
  onToggle=(action (mut myValue2))
}}

{{!-- custom --}}
{{#x-toggle
  onLabel='Custom Label'
  onToggle=(action 'toggleCustom')
  showLabels=true
  value=(readonly isCustomized)
  as |toggle|}}
  {{toggle.switch}}
  {{toggle.onLabel value=(readonly (not isCustomized))}}
{{/x-toggle}}

This worked as expected on the previously mentioned browsers, plus Firefox 53.0.2 on Windows 7.

[webark]

@rwwagner90 do you have a specific example that you are looking for? I've tried multiple different configurations and all have worked with this fix in ie.

The click was added before the label. Then the label was added which cause the "double click" behavior and why the accounting was needed to be done, which there was some ie oddness.

Safari (mobile and desktop) in the current beta version is broken though due to this double click behavior.

[RobbieTheWagner]

@webark I think @gtb104 covered the examples I was looking for. I am okay to merge this 👍 . Thanks for the PR!