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

Unable to toggle oneway to yes on highways #3638

Closed
davidchiles opened this issue Dec 8, 2016 · 2 comments
Closed

Unable to toggle oneway to yes on highways #3638

davidchiles opened this issue Dec 8, 2016 · 2 comments
Labels
bug A bug - let's fix this!

Comments

@davidchiles
Copy link

On highways I'm unable to toggle away from oneway=no to oneway=yes using the checkboxes.

Screen recording

macOS 10.12.1
Chrome 55.0.2883.75 (64-bit)

@tyrasd tyrasd added the bug A bug - let's fix this! label Dec 8, 2016
@tyrasd
Copy link
Member

tyrasd commented Dec 8, 2016

confirming (chrome 53 / ubuntu 16.10).

@bhousel bhousel closed this as completed in a3de353 Dec 8, 2016
@bhousel
Copy link
Member

bhousel commented Dec 8, 2016

Thanks for the report! This is something that I accidentally introduced during the iD v2 refactor, and it's a weird side effect of how JavaScript and D3 works.

You could trigger the bug by switching directly from one preset with a oneway to another preset with a oneway. D3 does not re-enter the field in that situation, and so the existing click handler was getting re-used. Because that click handler uses a closure variable value, that variable would stay around from "the time that the function was first created", i.e. the original oneway field.

The fix is: any handlers that use closure variables need to be assigned on the "update" selection, not the "enter" selection.

bad:

// select the field, join to [0] to create singleton
var field = selection.selectAll('.foo')
  .data([0]);

// create dom elements that don't yet exist
var enter = field.enter()
  .append('input')
  .attr('class', 'foo')
  .on('click', function() { ... do something with value closure variable... });

// merge enter selection into update selection
var input = input
  .merge(enter);

good:

// select the field, join to [0] to create singleton
var field = selection.selectAll('.foo')
  .data([0]);

// create dom elements that don't yet exist
var enter = field.enter()
  .append('input')
  .attr('class', 'foo');

// merge enter selection into update selection
var input = input
  .merge(enter);

// update selection runs every time
input
  .on('click', function() { ... do something with value closure variable... });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug - let's fix this!
Projects
None yet
Development

No branches or pull requests

3 participants