label editor: TypeError: e is undefined

[miketaylr]

STR:

1. login, go to any issue
2. click label editor icon to open
3. click it again to close

expected: editor closes
actual: doesn't close

Not sure when, but this regressed. We'll need a test as well.

[miketaylr]

@brizental if you get bored with tests, wanna investigate?

[karlcow]

@miketaylr not sure it's related, but setting a label and clicking outside of the menu doesn't work anymore too.
It seems now that you need to explicitly click on save and close.

[zoepage]

@karlcow This was intentional. The main reason was different UX paths that needed to be covered, but it's changeable. How much does it bother you?

[miketaylr]

I prefer the older "click outside to save" behavior (b/c it matches GitHub's).

[miketaylr]

@miketaylr not sure it's related, but setting a label and clicking outside of the menu doesn't work anymore too.

Not sure when the change was introduced, can you file a new bug for that?

[miketaylr]

@brizental seems like the bug is here:

d11ea5e#diff-4e6286167906e6713e9772fa9eae96eaR189

closeEditor: function(e) {
    if (e.keyCode === 27 || e.keyCode === undefined) {
      var checked = $("input[type=checkbox]:checked");
      var labelsArray = _.pluck(checked, "name");
      this.issueView.editorButton.removeClass("is-active");
      this.issueView.model.updateLabels(labelsArray);
      // detach() (vs remove()) here because we don't want to lose events if the
      // user reopens the editor.
      this.$el.children().detach();
    }
  }

(I didn't follow this PR or do review... but the first line is trying to handle escape key, right? isn't that covered by the following already?

d11ea5e#diff-4e6286167906e6713e9772fa9eae96eaR103

  keyboardEvents: {
    esc: "closeEditor"
  },

I've got 30 mins to kill so I'm gonna steal this issue from you @brizental. Hope that's OK!

[miketaylr]

but setting a label and clicking outside of the menu doesn't work anymore too.

Hmm... looking more at this code, did we ever actually support that? Looking at the v2.6.0 version of labels.js (about 1 year ago), https://github.com/webcompat/webcompat.com/blob/8850d9f743602988b85bf3d8697f54943460e11d/webcompat/static/js/lib/labels.js

I only see esc and clicking on button (the "save and close" text) as ways to close and save the editor. We could add that behavior easily tho... /me might just be confused!

edit: OK, I checked out that code and we def. did support that behavior.

[miketaylr]

OK... down a tiny rabbit hole, the code we used to handle this with was in the MainView, so if body got a click event we would call closeLabelEditor:

webcompat.com/webcompat/static/js/lib/issues.js

Lines 378 to 390 in 8850d9f

	closeLabelEditor: function(e) {
	var target = $(e.target);
	// early return if the editor is closed,
	if (!this.$el.find('.js-LabelEditor').is(':visible') ||
	// or we've clicked on the button to open it,
	(target[0].nodeName === 'BUTTON' && target.hasClass('js-LabelEditorLauncher')) ||
	// or clicked anywhere inside the label editor
	target.parents('.js-LabelEditor').length) {
	return;
	} else {
	this.labels.closeEditor();
	}
	},

[miketaylr]

can you file a new bug for that?

@karlcow nevermind, I can fix it in this bug since it's related-ish. Thanks for noticing.

[karlcow]

@zoepage to "How much does it bother you?" well I had to fix yesterday, all the bugs I had edited through webcompat last week, because the labels didn't get set :) and I used yesterday github instead of webcompat because of that. Changing labels is one of the main thing I do on the project. :)
@miketaylr Thanks a lot