MessageListTopbar does not unregister click handler properly

RESOLVED FIXED in 1.2 C3(Oct25)

Status

Firefox OS
Gaia::E-Mail
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
1.2 C3(Oct25)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c= p=1 s= u=])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
While investigating the email error in bug 924233 I noticed that MessageListTopbar does not properly remove its click handler.  It currently does something like this:

  el.addEventListener('click', this._onclick.bind(this));

Then:

  this._el.removeEventListener('click', this._onclick.bind(this));

Unfortunately this does not work because bind() creates a new, unique function each time it is called.  Even though the same this object is passed to bind() the resulting functions are not equal.

Also, the test does not actually test if the click occurs because it effectively does this:

  subject.decorate(el);
  sinon.stub(subject, '_onclick');

By the time the stub is created the real click handler has already been bound and set as the click handler.  The stub needs to be created before decorate() in order to intercept the click event.
(Assignee)

Comment 1

5 years ago
Created attachment 816681 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/12832

This patch saves the bound click handler so that it can be passed to removeEventListener() later.  It also moves the sinon.stub() up before subject.decorate().
Attachment #816681 - Flags: review?(gaye)
(Assignee)

Updated

5 years ago
Whiteboard: [c= p= s= u=]
(Assignee)

Comment 2

5 years ago
Comment on attachment 816681 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/12832

Sorry, forgot to look at email module owner/peer list.
Attachment #816681 - Flags: review?(gaye) → review?(bugmail)
Comment on attachment 816681 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/12832

Gareth was a good call to review this; I formally delegate review to Gareth :)
Attachment #816681 - Flags: review?(bugmail) → review?(gaye)
Target Milestone: --- → 1.2 C3(Oct25)
(Assignee)

Updated

5 years ago
Whiteboard: [c= p= s= u=] → [c= p=1 s= u=]
Comment on attachment 816681 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/12832

r+ with nits on github... thanks for the patch!
Attachment #816681 - Flags: review?(gaye) → review+
(Assignee)

Comment 5

5 years ago
Nits address and travis is green.  Merged:

  https://github.com/mozilla-b2g/gaia/commit/5fcd6074ad67630c5ec35333657bbfa7ae64fc95

Thanks for the review!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.