ibm-js / delite

HTML Custom Element / Widget infrastructure
http://ibm-js.github.io/delite/
Other
68 stars 28 forks source link

KeyNav: ensure event handlers are removed/added if focusDescendants changes. Fixes #375. #376

Closed AdrianVasiliu closed 9 years ago

AdrianVasiliu commented 9 years ago

Candidate fix for #375.

wkeese commented 9 years ago

This looks basically good but if possible I want to add a test for it before I check it in. I'll try to do that tomorrow.

The other approach is to set up the listeners unconditionally but then check if(this.focusDescendants) in the callbacks, like:

this.on("delite-deactivated", function () {
    if (this.focusDescendants) {
        this._keynavDeactivatedHandler();
    }
}.bind(this));

this.on("focusin", function (evt) {
    if (this.focusDescendants) {
        var target = this._getTargetElement(evt);
        if (target === this) {
            this._keynavFocusHandler(evt);
        } else {
            this._descendantNavigateHandler(target, evt);
        }
    }
}.bind(this));

I might do that instead since it's less code and perhaps doesn't need any special tests.

AdrianVasiliu commented 9 years ago

Okay, thanks Bill. I'll assume then that my PR can count on it being fixed a way or another in 0.6.0.

The other approach is to set up the listeners unconditionally but then check if(this.focusDescendants) in the callbacks

Yes, that's what I wrote in #375 ("Alternatively (but less optimal I guess), the handlers could be made no-op when this.focusDescendants is false."). I agree this might be good enough, although the other way spares unnecessary notifications when focusDescendants is false.

wkeese commented 9 years ago

Oh OK, I didn't notice that comment. I pushed that version.