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

Top navigation in General Settings no longer working #18663

Closed
tsteur opened this issue Jan 20, 2022 · 17 comments
Closed

Top navigation in General Settings no longer working #18663

tsteur opened this issue Jan 20, 2022 · 17 comments
Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Jan 20, 2022

This seems to be broken for me on demo2 https://demo2.matomo.org/index.php?module=CoreAdminHome&action=generalSettings&idSite=1&period=day&date=yesterday and locally but it works on demo.matomo.org

Looks like a regression in a recent change maybe?

When I click on one of these links then it doesn't jump to the correct settings
image

@tsteur tsteur added Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Jan 20, 2022
@tsteur tsteur added this to the 4.7.0 milestone Jan 20, 2022
@sgiehl
Copy link
Member

sgiehl commented Jan 20, 2022

I'm not able to reproduce that locally. Also not with disabled developer mode and the same set of plugins. It seems somehow the locationchange doesn't trigger the anchorfix on demo2 as calling scrollToAnchorInUrl manually does the scrolling correctly there.
@diosmosis do you have an idea what this could be caused by?

@diosmosis
Copy link
Member

Unfortunately I don't have access to the settings page on demo2. I can try reproducing locally.

@sgiehl
Copy link
Member

sgiehl commented Jan 21, 2022

@diosmosis let us know if you need access to demo2 to check that there.

@diosmosis
Copy link
Member

@sgiehl
Copy link
Member

sgiehl commented Jan 21, 2022

@tsteur Seems neither me, nor @diosmosis are able to reproduce that locally. So might be hard to find the reason.

@diosmosis I can give you super user access on demo2 if that helps. If you need ssh access to check if some changes are helping, guess @tsteur can help. I don't have ssh access to demo2.

@diosmosis
Copy link
Member

I have ssh access to demo2 if I need to change anything.

@sgiehl
Copy link
Member

sgiehl commented Jan 21, 2022

@diosmosis I just gave you super user access. Feel free to check if you can find the reason...

@diosmosis
Copy link
Member

Looks like the $locationChangeStart event is not even triggered. I'm guessing somewhere in angularjs it's not noticing the update to the hash. I've seen this before but wasn't able to non-intrusively fix it.

@diosmosis
Copy link
Member

Comparing between local and demo2, this event handler appears to be missing on demo2:

image

Might be related.

@diosmosis
Copy link
Member

Something off w/ the angularjs bootstrap logic.

@diosmosis
Copy link
Member

@sgiehl This line is what fails in demo2 but works locally:

  forEach(runBlocks, function(fn) { if (fn) instanceInjector.invoke(fn); });

On demo2 fn is set to undefined, so nothing is run. Does not at first glance appear to be Vue related.

@diosmosis
Copy link
Member

Nevermind, that's not the reason... still looking.

@diosmosis
Copy link
Member

@sgiehl / @tsteur ok, I can reproduce: if you disable GoogleAnalyticsImporter locally (and potentially any other plugin that uses the $location service in a run/config), the behavior is reproducible. $location needs to be manually required in a run block so it will be initialized I guess. Or maybe required specifically some other way. (The file that makes it work locally for me is widget-events.run.js.) Do not believe this is Vue related.

@sgiehl
Copy link
Member

sgiehl commented Jan 21, 2022

@diosmosis good catch. Guess this was caused by migrating some stuff to vue then, as the $location might not be used anymore. Would it solve the issue to simply inject $location into e.g. PluginSettings.adapter.ts?

@diosmosis
Copy link
Member

@sgiehl I think you'd need a run block:

angular.module('piwikApp').run(['$location', function () { }]);

@diosmosis
Copy link
Member

If sticking it in Vue code would put it in MatomoUrl.adapter.ts since it's to get angularjs initialized properly there.

@diosmosis
Copy link
Member

@sgiehl here you go: #18671 since you didn't find time

sgiehl pushed a commit that referenced this issue Jan 26, 2022
)

Co-authored-by: dizzy <diosmosis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

No branches or pull requests

3 participants