fix deprecated event.returnValue in Chrome

[zhiyelee]

As @hatched reported in #1417

chrome now issues a console warning
event.returnValue is deprecated. Please use the standard event.preventDefault() instead.

[juandopazo]

You don't need to check if preventDefault is there in DOMEventFacade. YUI loads a specific script for IE where it uses returnValue: https://github.com/yui/yui3/blob/master/src/event/js/event-facade-dom-ie.js#L109-L111.

And from the API Docs:

returnValue String
sets the returnValue of the event to this value (rather than the default false value). This can be used to add a customized confirmation query to the beforeunload event).

So maybe we need to avoid returnValue in all events except for onbeforeunload.

[ezequiel]

The logic here seems a bit flawed. Prior to your change, e.preventDefault was invoked without checking if said method actually existed. Therefore, your additional if statement will always evaluate to true.

preventDefault is manually attached to the Event object somewhere along the line...

[zhiyelee]

You are right.
Sorry, I misunderstanded the code here, I have doubted why we have returnValue and preventDefault at the same time. Seems we set returnValue property here for other use which I am not familiar, and the preventDefault is always exist.

I will reread relative code,thanks for explaning.

[zhiyelee]

@juandopazo I noticed the event-facade-dom-ie,but I didn't realized the usage relative to beforeunload in the comments far away from the function declare.

Thanks for explaning.

I also think avoid returnValue in all events except for onbeforeunload is a good idea. I will check it later.

[zhiyelee]

hi @juandopazo, I have made a change

[zhiyelee]

Ping..

[juandopazo]

I'll get a reviewer to look at it Monday.

Meanwhile, can you please add a test to avoid regressions and a notice in the API docs?

[zhiyelee]

@juandopazo Ok,I will make it later . Thx !

[ezequiel]

@juandopazo,

I don't see what value a notice to the API documentation would add in this case. It seems unusual and unnecessary to let the developer know about the deprecation of Event#returnValue, as they are not communicating directly with the DOM.

An additional unit test also seems unnecessary. All in all, what we we really care about is whether or not Event#preventDefault() still works properly after this change. I'm sure there are plenty of tests covering Event#preventDefault() already.

---

Ping @lsmith.

[lsmith]

The previous assignment was simpler because it was harmless. It is still harmless (a warning is a minor nuisance, but not harmful).

I would prefer to keep the logic simpler and go with

if (returnValue) {
    e.returnValue = returnValue;
}

And let the browser complain when a developer writes dubious code.

[zhiyelee]

Seems a much better way and also avoid hardcode.

[lsmith]

@ezequiel re: test coverage, sadly there are no tests of preventDefault(), let alone preventDefault(value) for DOM events.

It is worth adding a test for preventDefault(value).

[zhiyelee]

@lsmith Sorry, I don't quite understand your meaning by land in, it seems changes can be seen in this PR https://github.com/yui/yui3/pull/1429/files

[lsmith]

Weird. the commit wasn't showing on the PR for me, but now it is. My mistake.

[lsmith]

The || false is unnecessary.

[lsmith]

I'm ok with merging this even without new tests because the change in code functionality is obvious. The case where code is relying on e.returnValue in a non-IE environment is an edge case, and IMO does not need to be protected by the lib. That said, tests would be a nice bonus, given the currently limited coverage.

👍

[lsmith]

To the committer that merges the change in, if @zhiyelee hasn't already pruned the unnecessary || false, please do so.

@zhiyelee, In case no one else has mentioned, it's easier for us to merge in PRs that contain only changes to files in the 'src' directory.

[zhiyelee]

@lsmith Something wrong, I will close this and made another PR

[zhiyelee]

@lsmith I have pruned the unnecessary || false and made another PR that contain only changes to files in the srcdirectory #1460