Opened 7 years ago

Closed 7 years ago

#16233 closed defect (fixed)

[patch][cla] dojox/analytics throws a leave page/stay on the same page error on chrome

Reported by: dylan Owned by: dylan
Priority: high Milestone: 1.8.2
Component: Dojox Version: 1.8.1
Keywords: Cc: Dustin Machi, Kenneth G. Franqueiro
Blocked By: Blocking:

Description

the dojox/analytics/tests/test_analytics.html page will throw the following dialog when clicking on the links:

[object Object]

Are you sure you want to leave this page?

This appears to be related to how we return a promise, and how we handle this promise in checkData and pushData, and finally the call to unload.addOnUnload(this, "pushData", true);

We can either change pushData to not return a promise, and use a reference to the promise, or we could bind something different to the unload handler that doesn't use a promise.

Attachments (2)

16233.patch (2.2 KB) - added by dylan 7 years ago.
Patch file that stops returning a promise to address Google Analytics issue
16233-simple.patch (454 bytes) - added by Kenneth G. Franqueiro 7 years ago.
Much simpler and more isolated patch

Download all attachments as: .zip

Change History (7)

Changed 7 years ago by dylan

Attachment: 16233.patch added

Patch file that stops returning a promise to address Google Analytics issue

comment:1 Changed 7 years ago by dylan

Status: newassigned
Summary: dojox/analytics throws a leave page/stay on the same page error on chrome[patch][cla] dojox/analytics throws a leave page/stay on the same page error on chrome

Added a patch against trunk. This needs a review. While this works, I'm not sure this is ideal.

comment:2 Changed 7 years ago by Kenneth G. Franqueiro

I'm no expert with this code, but I'm wondering why handleAs: "json" was added to the requests - seems like that could be changing behavior, and is unrelated to the fix? IIUC the focus of the fix is the introduction of promiseRef instead of returning a promise from pushData (which is what's causing the popup message, since pushData is hooked on onbeforeunload).

Also, you can remove the now-commented old return statements, no sense keeping them there.

Changed 7 years ago by Kenneth G. Franqueiro

Attachment: 16233-simple.patch added

Much simpler and more isolated patch

comment:3 Changed 7 years ago by dylan

In [29953]:

refs #16233, fixes analytics issue in chrome

comment:4 Changed 7 years ago by dylan

Ken's patch passes tests and is much simpler, I've committed to trunk. Will also backport to 1.8.x.

comment:5 Changed 7 years ago by dylan

Resolution: fixed
Status: assignedclosed

In [29954]:

fixes #16233

Note: See TracTickets for help on using tickets.