Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proof of concept fix for history plugin's scroll to top issue #7869

Merged
merged 6 commits into from May 13, 2015

Conversation

diosmosis
Copy link
Member

Fix involves using a new angular service that uses the $location singleton instead of jquery's history plugin.

If this is acceptable, we can remove the history plugin after this change.

Note: I haven't tested every hash change use case, but the following works:

  1. loading different pages in the main reporting UI works
  2. reloading after a change works
  3. loading different pages and going back works
  4. opening & closing a popover works (and there is no scroll change)

Refs #7798

@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Needs Review PRs that need a code review labels May 9, 2015
@diosmosis diosmosis added this to the 2.14.0 milestone May 9, 2015
@diosmosis diosmosis added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label May 9, 2015
@sgiehl
Copy link
Member

sgiehl commented May 9, 2015

I just had a quick look. Could you please have a look at the popovers again.
When I click on a visitor profile in the visitor log, it opens correctly but the hash isn't changed.
When clicking anywhere within the popover it is reloaded and the hash is adjusted (after that reload everything works as expected). When clicking outside the popover (after opening it) it closes but the hash is also adjusted to conatin the popover params.

@diosmosis
Copy link
Member Author

@sgiehl There was a $rootScope.$apply() missing. I've updated the code, and removed the jquery history plugin. I think it should still be tested, I'm not sure where to look for every possible problem.

@tsteur
Copy link
Member

tsteur commented May 10, 2015

Cool, have you tried it in all the browsers? Especially older ones like IE8?

I had a quick look in latest Chrome on Mac and somehow I got this error in the console:

Error: [$rootScope:inprog] http://errors.angularjs.org/1.2.28/$rootScope/inprog?p0=%24digest
    at Error (native)
    at http://apache.piwik/libs/bower_components/angular/angular.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:6:450
    at k (http://apache.piwik/libs/bower_components/angular/angular.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:106:37)
    at h.$apply (http://apache.piwik/libs/bower_components/angular/angular.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:113:293)
    at http://apache.piwik/plugins/CoreHome/javascripts/broadcast.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:386:24
    at Object.d [as invoke] (http://apache.piwik/libs/bower_components/angular/angular.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:35:36)
    at Object.broadcast.propagateNewPopoverParameter (http://apache.piwik/plugins/CoreHome/javascripts/broadcast.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:384:46)
    at HTMLDivElement.options.close (http://apache.piwik/plugins/CoreHome/javascripts/popover.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:47:27)
    at t.Widget._trigger (http://apache.piwik/libs/bower_components/jquery-ui/ui/minified/jquery-ui.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:6:10036)
    at HTMLDivElement.<anonymous> (http://apache.piwik/libs/bower_components/jquery-ui/ui/minified/jquery-ui.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:7:6106)
Error: [$rootScope:inprog] http://errors.angularjs.org/1.2.28/$rootScope/inprog?p0=%24digest
    at Error (native)
    at http://apache.piwik/libs/bower_components/angular/angular.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:6:450
    at k (http://apache.piwik/libs/bower_components/angular/angular.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:106:37)
    at h.$get.h.$apply (http://apache.piwik/libs/bower_components/angular/angular.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:113:293)
    at http://apache.piwik/plugins/CoreHome/javascripts/broadcast.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:386:24
    at Object.d [as invoke] (http://apache.piwik/libs/bower_components/angular/angular.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:35:36)
    at Object.broadcast.propagateNewPopoverParameter (http://apache.piwik/plugins/CoreHome/javascripts/broadcast.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:384:46)
    at HTMLDivElement.options.close (http://apache.piwik/plugins/CoreHome/javascripts/popover.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:47:27)
    at t.Widget._trigger (http://apache.piwik/libs/bower_components/jquery-ui/ui/minified/jquery-ui.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:6:10036)
    at HTMLDivElement.<anonymous> (http://apache.piwik/libs/bower_components/jquery-ui/ui/minified/jquery-ui.min.js?cb=9c723622fbdb8d60f8c3ae283dac44d8:7:6106)

What I did to reproduce was I went to the Admin -> Marketplace page. Opened the first plugin in a popover, pressed the browser back button and got this error. I'm not sure what is happening there. Maybe a $rootScope.digest is already running when we call the $rootScope.apply see eg http://stackoverflow.com/questions/22346990/why-is-using-ifscope-phase-scope-apply-an-anti-pattern ? I cannot always reproduce it so this seems to be possibly the case.

@diosmosis
Copy link
Member Author

It's possible, the problem is when invoking angular from broadcast we need $rootScope.$apply(), but from inside angular we shouldn't. Unfortunately, there's a lot interconnectedness w/ that code, so it's possible for strange things to happen.

I haven't checked every browser, but I'll look into it soon.

@tsteur
Copy link
Member

tsteur commented May 10, 2015

I think there are ways to avoid this error see eg the linked StackOverflow link. Would be great to avoid that JS error. If that solution works it would be cool and a first start to replace broadcast with Angular routing etc which would be awesome mid/long term.

@diosmosis diosmosis force-pushed the 7798_angular_history_change branch from c36a3cc to b6e564b Compare May 12, 2015 04:44
@diosmosis diosmosis removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label May 12, 2015
@diosmosis
Copy link
Member Author

Tested on the following browsers:

Ubuntu:

  • Firefox (latest)
  • Chrome (latest)
  • Opera (latest)

Windows:

  • IE8
  • IE9

OS X:

  • Safari (latest)

Appears to work in every case.

The PR is ready for another review and/or merge. Note: I moved the $rootScope.$apply to a setTimeout as described in the stack overflow link, and moved it to the history service. I'm hoping this won't create unnecessary digest cycles. Though if it does, shouldn't be a big issue unless the history service is used by an angular component rather than just broadcast.

@tsteur
Copy link
Member

tsteur commented May 12, 2015

Looks good to me code wise, not sure if I tested all cases.

diosmosis added 4 commits May 12, 2015 22:23
…ssue. Fix involves using angular service that uses the singleton instead of jquery's history plugin. Can remove the history plugin after this change.
…le after modifying (through the history service), hide error in anchorLinkFix code if hash is not valid element selector, and use .path to push new history items.
…ating new digest cycle to avoid digest already in progress error.
@diosmosis diosmosis force-pushed the 7798_angular_history_change branch from b6e564b to f2b5cb3 Compare May 13, 2015 05:37
diosmosis added a commit that referenced this pull request May 13, 2015
Fixes #7798 use angular.js $location object to change current page instead of jquery history plugin. Includes new angular service that manipulates $location object and invokes old broadcast object. jquery history plugin is removed.
@diosmosis diosmosis merged commit 7eb55f2 into master May 13, 2015
@diosmosis diosmosis deleted the 7798_angular_history_change branch May 13, 2015 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants