[WIP] Implement delegate change event for Internet Explorer < 9. Fixes #2531106 #375

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Contributor

ipeychev commented Dec 25, 2012

Here is a commit to fix #2531106.

This is only the source, no build files were added. shifter --walk in src will do the trick but it adds too much noise. Is there a better way to build only event module and update the loader info in the same time?

Thanks,

Owner

davglass commented Dec 25, 2012

You don't need to attach the build files, our Travis build will build
the modules when it tests them ;)

Owner

davglass commented Jan 16, 2013

@lsmith Could we arrange a time to do a code review on this?

Contributor

lsmith commented Jan 16, 2013

I'll review it tomorrow. If you want to review in hangout, catch me on IM and we'll schedule a date/time.

@lsmith lsmith commented on the diff Jan 18, 2013

src/loader/js/yui3.js
@@ -1316,6 +1316,20 @@ YUI.Env[Y.version].modules = YUI.Env[Y.version].modules || {
"node-base"
]
},
+ "event-delegate-change": {
+ "after": [
+ "event-base"
+ ],
+ "condition": {
+ "name": "event-delegate-change",
+ "trigger": "event-base-ie",
+ "ua": "ie"
@lsmith

lsmith Jan 18, 2013

Contributor

Isn't the trigger of "event-base-ie" sufficient?

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+
+ delete subscription._handles;
+ },
+
+ /**
+ * Retrieves the name of the native event which should be attached.
+ * For input elements of type "checkbox" and "radio", the event will be "click".
+ * For any others it will be "change"
+ *
+ * @method _getEventName
+ * @private
+ * @param activeElement {Y.Node} The element to check for the event
+ * @return {String} The name of the event
+ */
+ _getEventName: Y.cached(
+ function(activeElement) {
@lsmith

lsmith Jan 18, 2013

Contributor

Can you choose a different parameter name? This is shadowing the global activeElement, though it's not useful in this context. Because one might think to use activeElement in a module having to do with form fields and their events, this is potentially confusing.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ * @param filter {String|Function} Selector string or function that accepts an event object and returns null, a Node, or an array of Nodes matching the criteria for processing.
+ */
+ detachDelegate: function (node, subscription, notifier) {
+ this._detachEvents(node, subscription, notifier);
+ },
+
+ /**
+ * Implementation logic for subscriptions done via <code>node.on(type, fn)</code> or <code>Y.on(type, fn, target)</code>. <br>
+ *
+ * @method on
+ * @param node {Node} the node the subscription is being applied to
+ * @param subscription {Subscription} the object to track this subscription
+ * @param notifier {SyntheticEvent.Notifier} call notifier.fire(..) to trigger the execution of the subscribers
+ */
+ on: function (node, subscription, notifier) {
+ this._attachEvent(node, subscription, notifier);
@lsmith

lsmith Jan 18, 2013

Contributor

This should also handle Y.one('div').on('change', fn);, I would guess by relaying to delegate, though you'd have to supply the filter.

This is not critical because there's the workaround of using delegate directly, but it is likely to come up at some point, so if it's trivial to implement, please do so.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ while (result !== false && !event.stopped && tmpEl && tmpEl !== delegateEl);
+
+ result = ((result !== false) && (event.stopped !== 2));
+ }
+ }
+ else {
+ if (!event._stoppedWithFalse && notifier.fire(event) === false) {
+ event._stoppedWithFalse = true;
+ }
+ }
+
+ return result;
+ };
+
+ if (!YObject.owns(handles, type)) {
+ handles[type] = YEvent._attach([type, fireFn, node, notifier]);
@lsmith

lsmith Jan 18, 2013

Contributor

It's not necessary to pass notifier here. It's available by closure.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ }
+ while (result !== false && !event.stopped && tmpEl && tmpEl !== delegateEl);
+
+ result = ((result !== false) && (event.stopped !== 2));
+ }
+ }
+ else {
+ if (!event._stoppedWithFalse && notifier.fire(event) === false) {
+ event._stoppedWithFalse = true;
+ }
+ }
+
+ return result;
+ };
+
+ if (!YObject.owns(handles, type)) {
@lsmith

lsmith Jan 18, 2013

Contributor

I'm pretty sure you can use if (!handles[type]) here.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ type = this._getEventName(node),
+ self = this;
+
+ var fireFn = function(event) {
+ var result = true;
+
+ if (filter) {
+ if (!event.stopped) {
+ var delegateEl = YNode.getDOMNode(delegateNode);
+
+ var tmpEl = YNode.getDOMNode(node);
+
+ do {
+ if (tmpEl && Selector.test(tmpEl, filter)) {
+ event.currentTarget = Y.one(tmpEl);
+ event.container = node;
@lsmith

lsmith Jan 18, 2013

Contributor

event.container = delegateNode;?

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ if (!event.stopped) {
+ var delegateEl = YNode.getDOMNode(delegateNode);
+
+ var tmpEl = YNode.getDOMNode(node);
+
+ do {
+ if (tmpEl && Selector.test(tmpEl, filter)) {
+ event.currentTarget = Y.one(tmpEl);
+ event.container = node;
+
+ result = notifier.fire(event);
+ }
+
+ tmpEl = tmpEl.parentNode;
+ }
+ while (result !== false && !event.stopped && tmpEl && tmpEl !== delegateEl);
@lsmith

lsmith Jan 18, 2013

Contributor

node.delegate(eventName, fn, filterThatMatchesNode) has been supported since 3.3.0, so you'll need to test against delegateEl.parentNode.

Also, do/while? Who uses do/while, any more? In my entire career, I've used do/while maybe 5 times. :)
You're doing a truthy test against tmpEl at the start. It seems like you could get away with a plain old while.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ if (tmpEl && Selector.test(tmpEl, filter)) {
+ event.currentTarget = Y.one(tmpEl);
+ event.container = node;
+
+ result = notifier.fire(event);
+ }
+
+ tmpEl = tmpEl.parentNode;
+ }
+ while (result !== false && !event.stopped && tmpEl && tmpEl !== delegateEl);
+
+ result = ((result !== false) && (event.stopped !== 2));
+ }
+ }
+ else {
+ if (!event._stoppedWithFalse && notifier.fire(event) === false) {
@lsmith

lsmith Jan 18, 2013

Contributor

event._stoppedWithFalse? Isn't this trying to accomplish break?

Don't put actions in conditionals. Have the notifier.fire(event) in the block, not the condition.

I find inlined } else if (...) { easier to read than } else { if (...) { ... } }. I don't know if there's a style guide point on this, but the rest of the YUI code, so far as I'm aware, inlines else if.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ *
+ * @method _attachEvents
+ * @private
+ * @param node {Y.Node} The node to which events will be attached
+ * @param subscription {Subscription} The object tracking this subscription
+ * @param notifier {SyntheticEvent.Notifier} The triggering mechanism used by SyntheticEvents
+ * @param filter {String | Function} Selector string or function that accepts an event object and returns null, a Node, or an array of Nodes matching the criteria for processing.
+ */
+ _attachEvents: function(node, subscription, notifier, filter) {
+ var handles = this._prepareHandles(subscription, node),
+ self = this;
+
+ handles[EVENT_BEFOREACTIVATE] = node.delegate(
+ EVENT_BEFOREACTIVATE,
+ function(event) {
+ var activeElement = event.target;
@lsmith

lsmith Jan 18, 2013

Contributor

See below comment regarding the name activeElement.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ },
+ filter
+ );
+ },
+
+ /**
+ * Detaches all the nodes, registered to the supplied subscription
+ *
+ * @method _detachEvents
+ * @private
+ * @param node {Y.Node} The node the subscription was applied to
+ * @param subscription {Subscription} The object tracking this subscription
+ * @param notifier {SyntheticEvent.Notifier} The triggering mechanism used by SyntheticEvents
+ */
+ _detachEvents: function(node, subscription, notifier) {
+ Y.each(
@lsmith

lsmith Jan 18, 2013

Contributor

Store multiple events in an array, then detach with new Y.EventHandle(arrayOfHandles).detach();.

And in general, don't use Y.each. Prefer Y.Object.each and Y.Array.each unless you need to handle both types, or custom objects that host their own each method.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ ),
+
+ /**
+ * Creates _handles property to the subscription if not exists.
+ * Creates an empty object in _handles property which belongs to the node, passed as argument
+ *
+ * @method _prepareHandles
+ * @private
+ * @param subscription {Function} The callback function
+ * @param node {Y.Node} The node for which the handles will be registered
+ * @return {Object} The value of the created handle object for the passed node
+ */
+ _prepareHandles: function(subscription, node) {
+ var handles;
+
+ if (!YObject.owns(subscription, '_handles')) {
@lsmith

lsmith Jan 18, 2013

Contributor

Would this ever fail?

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+
+ return eventName;
+ }
+ ),
+
+ /**
+ * Creates _handles property to the subscription if not exists.
+ * Creates an empty object in _handles property which belongs to the node, passed as argument
+ *
+ * @method _prepareHandles
+ * @private
+ * @param subscription {Function} The callback function
+ * @param node {Y.Node} The node for which the handles will be registered
+ * @return {Object} The value of the created handle object for the passed node
+ */
+ _prepareHandles: function(subscription, node) {
@lsmith

lsmith Jan 18, 2013

Contributor

Another petition to explain the handle storage strategy. It looks like you're attempting to prevent duplicate subscriptions, when in fact you may want duplicates. We should chat about this in a more synchronous medium (irc or G+ hangout).

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/meta/event.json
@@ -104,6 +104,19 @@
"trigger": "node-base",
"test": "ie-base-test.js"
}
+ },
+ "event-delegate-change": {
+ "requires": [
+ "node-event-delegate",
@lsmith

lsmith Jan 18, 2013

Contributor

It only requires event-synthetic. If the implementer doesn't use a delegate module, that shouldn't impact them being able to write container.on('change', fn).

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/tests/manual/event-delegate-change.html
+ Y.log('Change event to checkbox, returning false. The second event listener to this checkbox should not execute.');
+
+ return false;
+ }
+ ),
+
+ Y.one('#returnFalseCheckbox').on(
+ 'change',
+ function(event) {
+ Y.log('Second change event to checkbox, returning false, this should never log');
+ }
+ )
+ ];
+
+ Y.one('#detachListeners').once('click', function(events) {
+ Y.Array.invoke(handles, 'detach');
@lsmith

lsmith Jan 18, 2013

Contributor

Here also, you can use new Y.EventHandle(handles).detach().

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/meta/event.json
@@ -104,6 +104,19 @@
"trigger": "node-base",
"test": "ie-base-test.js"
}
+ },
+ "event-delegate-change": {
@lsmith

lsmith Jan 18, 2013

Contributor

Name the module event-change similar to event-focus, which also patches event bubbling.

@lsmith lsmith commented on the diff Jan 18, 2013

src/event/js/event-delegate-change.js
+ * @private
+ * @param node {Y.Node} The node to which events will be attached
+ * @param subscription {Subscription} The object tracking this subscription
+ * @param notifier {SyntheticEvent.Notifier} The triggering mechanism used by SyntheticEvents
+ * @param filter {String | Function} Selector string or function that accepts an event object and returns null, a Node, or an array of Nodes matching the criteria for processing.
+ */
+ _attachEvents: function(node, subscription, notifier, filter) {
+ var handles = this._prepareHandles(subscription, node),
+ self = this;
+
+ handles[EVENT_BEFOREACTIVATE] = node.delegate(
+ EVENT_BEFOREACTIVATE,
+ function(event) {
+ var activeElement = event.target;
+
+ self._attachEvent(activeElement, subscription, notifier, node, filter);
@lsmith

lsmith Jan 18, 2013

Contributor

This will attempt to attach the individual event listeners to the, say, <input> once for every match of the delegate filter. I'm not following the event handle storage strategy you have going on right now (I'm sleepy). Can you elaborate? The potential duplicate subscriptions seems to compete with the manual pseudo-delegate parent axis walk starting around line 93.

@lsmith

lsmith Jan 18, 2013

Contributor

Oh, is the duplication protection to avoid resubscribing the events in response to each 'onbeforeactivate'? You might want to do this with node.once(), or since you have to use _attach, to set the 'once' property of the DOM event subscription (see https://github.com/yui/yui3/blob/master/src/event-custom/js/event-target.js#L139)

Owner

davglass commented Jan 18, 2013

@lsmith Thanks for the great review. @ipeychev would you like a Google Hangout with Luke and I to discuss this (after you digest them all)?

Contributor

ipeychev commented Jan 22, 2013

Thanks for the review.

I will try to get back on this soon.

Contributor

triptych commented Jun 13, 2013

Hi @ipeychev is there anything we can do to help you with this pull request? Feel free to close out and start fresh if you like, or post to the contributor mailing list if you'd like more feedback.

@ipeychev just wanted to let you know that, to get a fresh build of loader-yui3, you only need to build loader and yui.

Contributor

ipeychev commented Jun 20, 2013

Hey guys,

Currently I am not able to work on this issue. However, I'm going in vacation soon, so I hope will be able to continue working on it.

Thanks,

Contributor

triptych commented Aug 22, 2013

Adding [WIP] to title until this gets revisited.

I hope it is not rude to tickle this? Unfortunately IE8 is not irrelevant yet.
In addition, does this issue not apply to other form related events, such as submit (important), select, reset (perhaps less so)? That is, IE8 and earlier don't bubble these either.
View (and perhaps other higher level apis) relies on delegate to implement an events property, which of fails if used to manage a form through the DOM form events.

Contributor

clarle commented May 18, 2014

I'm closing this due to a bit of a lack of activity, but @ipeychev, if you'd like to work with me on getting this into YUI core, please definitely let me know!

We can always rebase this to try to get it back up to latest master, which has changed a lot since the initial PR.

clarle closed this May 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment