@siva538 opened this Pull Request on May 28th 2019 Contributor

Issue with ShowModalDialog in IE browser is fixed.
fixes #11507

@siva538 commented on May 30th 2019 Contributor

Reviewer, also quick note on the piwik.js file being modified. It was assumed that the minified version of the file would be generated as part of build process, and hence explicitly not modified.

Please let me know if anything else needs to be done along with this JS file.

@diosmosis commented on June 25th 2019 Member

The change looks ok to me, if we can set a cookie properly, that should be enough. To be safe though, we could surround the test in a try-catch. What do you think @tsteur ?

@tsteur commented on June 25th 2019 Member

Sounds good @diosmosis
does it make sense to simply return '0' on any error as we probably can't rely on navigatorAlias.cookieEnabled in this case?

@diosmosis commented on June 25th 2019 Member

Yes, makes sense, if something bad happens when we try and set a cookie, we can't set a cookie.

@siva538 commented on June 29th 2019 Contributor

Thanks @diosmosis and @tsteur for your feedback. Is there any action item from my side or will you be able to help merge this change?

@tsteur commented on June 29th 2019 Member

There is nothing that needs to be done. We will include this in the 3.11 release. There's not enough testing time for 3.10 release unfotunately.

@tsteur commented on July 17th 2019 Member

image
Actually, I couldn't reproduce the warning @siva538

FYI: I always got "Access denied" when using showModalDialog until I realised you need to turn off the popup blocker in IE10/11 for it to test.

In general I was worried removing cookieEnabled check that hasCookies() might always return true cause in the end we're simply writing a property on document.cookie=... and reading the same property within the same request. I only tested latest Chrome and Firefox and IE11 but seems like browsers actually won't store any value on document.cookie when cookies are disabled. So it looks like merging the PR will be fine and the browser doesn't start treating this property like any regular property.

Couldn't find a blame why this check was added initially to see if it causes any problem maybe https://github.com/matomo-org/matomo/commit/c654e7382820fa5c1155eca30ee9617bd5b86d6d#diff-1279d666063b65e6d6777f902d11574f

@mattab not sure if there could be any regression or problem with it but suppose it should be fine.

At the same time popups are blocked by default in IE and not sure where this method showModalDialog actually works.

Assuming all browsers hopefully behave the same it should be fine to merge.

@ibril15 commented on July 25th 2019

Not sure if you're still having problems reproducing or not, but the file below should help. You can open it in IE11 straight from your desktop, just hit "Allow Blocked Content" at the bottom if you get prompted.

testmodalpopup.zip

@siva538 commented on August 5th 2019 Contributor

To re-iterate, this is an important fix that is causing lot of problems with the manual code modifications to piwik.js file and yet times it is getting overwritten and resulting in production critical issues.

Thanks for your help and understanding. Please let us know if you can merge this for 3.12.

@mattab commented on August 9th 2019 Member

@tsteur Reckon we can just merge it. In the worst case it would regress the accuracy of tracking how many users support Cookies. I'll set a reminder to check the reports in 2 months.

@tsteur commented on August 12th 2019 Member

Sweet, this PR is good to merge. We just need to update the minified tracker version afterwards.

@diosmosis commented on August 16th 2019 Member

Just merged, thanks @siva538 for the great first contribution!

This Pull Request was closed on August 16th 2019
Powered by GitHub Issue Mirror