Skip to content

Active target #1156

Merged
merged 21 commits into from Mar 28, 2013

3 participants

@mczepiel
MontageJS member
mczepiel commented Mar 7, 2013

Mainly posting here for sanity-checking, review, comments. I don't expect this to get pulled in without some discussion of naming.

All components probably need some review to accommodate this change, though even I'm surprised how painlessly it's slipped into the component prototype chain so far.

Plenty of specs are failing, but that seems to be the case atop edge right now, I don't think this makes matters worse but wouldn't mind some double checking. (3283 specs/ 26 failing) I'm also not sure how this wil all play with the matte/digit work that's going on right now but I'd like this to be considered ahead of any of that.

I also might squash some parts of this change set before it's accepted but have left my work open for illustrative purposes.

@kriskowal kriskowal commented on the diff Mar 8, 2013
core/core.js
@@ -627,13 +627,6 @@ Montage.defineProperty(Montage, "identifier", {
serializable: true
});
-// TODO @mczepiel Is the level of indirection of parentProperty necessary? Why
-// not just use `parent` instead of `[this.parentProperty]` throughout?
-// - @kriskowal
-Montage.defineProperty(Montage, "parentProperty", {
- value: null
-});
-
/**
@kriskowal
MontageJS member
kriskowal added a note Mar 8, 2013

YAY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kriskowal kriskowal commented on an outdated diff Mar 8, 2013
core/event/event-manager.js
- defaultEventManager.handleEvent(targettedEvent);
- },
- enumerable: false
-});
-
-/**
- @function external:Object#dispatchEventNamed
- */
-Montage.defineProperty(Object.prototype, "dispatchEventNamed", {
- value: function(type, canBubble, cancelable, detail) {
- var event = MutableEvent.fromType(type, canBubble, cancelable, detail);
- event.target = this;
- defaultEventManager.handleEvent(event);
- }
-});
-
var EventListenerDescriptor = Montage.create(Montage, {
@kriskowal
MontageJS member
kriskowal added a note Mar 8, 2013

YAY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kriskowal kriskowal commented on an outdated diff Mar 8, 2013
ui/native/button.reel/button.js
@@ -123,6 +123,18 @@ var Button = exports.Button = Montage.create(NativeControl, /** @lends module:"m
}
},
+ _acceptsFocus: {
+ value: true
+ },
@kriskowal
MontageJS member
kriskowal added a note Mar 8, 2013

With this pattern, I would be sure to double-check that any bit of code that is assigning to _acceptFocus directly (as opposed to indirectly through acceptFocus.set) manually dispatches an "acceptFocus" own property change.

@kriskowal
MontageJS member
kriskowal added a note Mar 8, 2013

Did the check myself and it looks fine at least assuming that there are no peers or heirs abusing the “private” property. Consider above just-for-future-reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kriskowal kriskowal commented on the diff Mar 8, 2013
core/target.js
@@ -0,0 +1,85 @@
@kriskowal
MontageJS member
kriskowal added a note Mar 8, 2013

YAY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kriskowal kriskowal commented on an outdated diff Mar 8, 2013
ui/component.js
@@ -82,8 +83,27 @@ var Component = exports.Component = Montage.create(Montage,/** @lends module:mon
value: null
},
- parentProperty: {
- value: "parentComponent"
+ _nextTarget: {
+ value: null
+ },
+
+ /**
+ * The next Target to consider in the event target chain
+ *
+ * Currently, components themselves do not allow this chain to be broken;
+ * setting a component's nextTarget to a falsy value will cause nextTarget
+ * to resolve as the parentComponent.
+ * *
@kriskowal
MontageJS member
kriskowal added a note Mar 8, 2013

Got an extra asterisk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kriskowal kriskowal commented on the diff Mar 8, 2013
test/events/object-hierarchy-spec.js
testApplication = testDocument.application;
eventManager = testApplication.eventManager;
eventManager.reset();
});
- it("should have a parentProperty on a Montage object", function() {
- expect((Montage.create()).parentProperty).toBeDefined();
- });
-
- // TODO @mczepiel Are these cases this necessary? - @kriskowal
@kriskowal
MontageJS member
kriskowal added a note Mar 8, 2013

YAY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
mczepiel added some commits Mar 6, 2013
@mczepiel mczepiel Add Target class to participate in events
ActiveTarget indicates the logical target the user is focused on much
as the document.activeElement indicates the element the user is
focused on.

activeElement may differ from activeTarget as the activeElement may
have no associated component.

All components are Targets.

This hopefully clarifies some of the half-baked attempts at articulating
this need as evidenced by the component's "acceptsDirectFocus",
and the "parentProperty" to facilitate pointing to the next target
candidate, in addition to a few one-off implementations manually
doing the work of finding the activeComponent.

Specifically, anywhere where we dispatched an event on a component the
user was focused on instead of where the event happened will now be
easier.

The eventManager also provides two new APIs that leverage this new
information, `dispatchFocusedEvent` and `dispatchFocusedEventNamed`
that match their less focused brethren in purpose while using the
activeTarget as the proximal target of the event.

Targets also provide a `nextTarget` property which is used exactly as
the `parentProperty` was to direct distribution of events through a tree
of target candidates.

The implementation thus far reuses the activationEvents concept which
the EventManager already manages. An alternative implementation
could derive the activeTarget from the activeElement and even
drive the prepareForActivationEvent system through focus alone.

That remains a possibility whereas this implementation minimizes
churn by using as much of the existing machinery as possible.
4029cba
@mczepiel mczepiel Move eventDispatching off Object.prototype
I didn't even know they were there, now they're on Target. Anybody that
ever plans on being able to be the proximal target or a target during
event distribution will need to be a Target now.

I've found a few places in my limited testing where this was necessary
to introduce in a few object's prototype chain.

The specs are a daunting work in progress.
4120c19
@mczepiel mczepiel Correct a few objects trying to dispatchEvents
Object.prototype no longer provides this functionality, it's moved
into Target.
901053b
@mczepiel mczepiel Rename as testMontage as testTarget for clarity
The intent being that the Target object used as the prototype has come
from the test environment, not the test suite environment.

This used to actually be a Montage object, not a Target.
71a6d2c
@mczepiel mczepiel Add specs for activeTarget concept
Always a worthwhile endeavor as I figured out I wasn't actually using
nextTarget to find the next target if the original didn't acceptFocus.

Anyway, now we have tests built on top of the hands-on playground so
there should be enough to get a feel for how this system works.
3a40851
@mczepiel mczepiel Ensure a component's nextTarget can be set
Of course, by default if there is non specified, use the
parentComponent. I'm purposefully going to prevent breaking the chain
if the nextTarget is falsy on components.

There is also now an example of redirecting the event target chain off
off of the componentTree
d0a23dd
@mczepiel mczepiel Differ event target path building strategies
Events originating from DOM targets remains DOM only in considering
target candidates.

Events originating from a Target uses the nextTarget facility in
considering target candidates.
99d9e1f
@mczepiel mczepiel Switch key-composer over to dispatchFocusdEvent
This is a half-baked transition; just enough to make sure it works in
theory. It's certainly not an exhaustive switch. I think the API may
warrant some changing with these results as usage of the keyComposers
is still confusing.
c92c050
@mczepiel mczepiel Visualize the application level event target chain
There ae better visualizations of course but this is a decent start.
fbc09f1
@mczepiel mczepiel Move eventListener API off of Object.prototype
More victories in consolidating dispatching responsibilities to
the Target; now the listener registration can be centralized here seeing
as these are the only objects that should be dispatching.

This does elevate Target to be a more vital concept, but it's sensible.

This may prove more disruptive than relocating the dispatching alone,
I haven't rigorously tested.
b5e1249
@mczepiel mczepiel Rename acceptsFocus as acceptsActiveTarget
I don't want to conflate the concept of focus and the concept of being
activeTarget.
0b01412
@mczepiel mczepiel Add API to Target around becoming activeTarget
In particular I've added a rough menu/toolbar example that uses these
new API hooks to preserve the original activeTarget while allowing
the menuItems themselves to become the activeTarget for handling
keyboard events.

While we may have some opinionated components that do otherwise,
this activeTarget system doesn't limit interesting multi-input
scenarios.
d9fb2ad
@mczepiel
MontageJS member

OK, I see a lot of positive response and no particular concerns. Having gotten pretty happy with the current implementation I'd like to get this into edge at this point and smooth any rough spots as they're encountered.

mczepiel added some commits Mar 13, 2013
@mczepiel mczepiel Merge branch 'edge' into active-target
Conflicts:
	composer/composer.js
	core/application.js
	test/run.js
	ui/check-input.js
	ui/native/button.reel/button.js
fd51dc9
@mczepiel mczepiel Merge branch 'edge' into active-target
Conflicts:
	test/events/object-hierarchy-spec.js
13773f6
@mczepiel mczepiel Merge branch 'edge' into active-target 841029a
@mczepiel mczepiel Merge branch 'edge' into active-target 8c438dc
@mczepiel mczepiel Merge branch 'edge' into active-target 32c1e7c
@mczepiel mczepiel Add labels to test for easier following
It may not be obvious what the identifier of each component is
791dd62
@mczepiel mczepiel Request a draw upon creation
Otherwise the component never draws as it has no template and really
has no reason to draw. This meant the listener was not being installed.
745c6a1
@mczepiel mczepiel Move target info panel out of the way
Helps that it doesn't jump the page around as it changes vertical
offset based on its content, and it's always visible.
bef7d0e
@mczepiel mczepiel Remove dispatchFocusedEvent APIs
It's simple enough to get a reference to the eventManager and access
the activeTarget  to dispatch from. No need for another set of
convenience API at the moment. Maybe later but it's still a relatively
rare occurrence.
cc6fded
@francoisfrisch francoisfrisch merged commit cc6fded into montagejs:edge Mar 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.