Top menu entries for spaces are not highlighted when clicked

[marc-farre]

What steps will reproduce the problem?

Create a custom module.

Add this event:

    public static function onTopMenuInit($event)
    {
        /** @var TopMenu $menu */
        $menu = $event->sender;
        $space = Space::findOne(1);
        $currentContentContainer = ContentContainerHelper::getCurrent();

        $menu->addEntry(new MenuLink([
            'label' => "My space",
            'url' => $space->getUrl(),
            'icon' => 'inbox',
            'sortOrder' => 100,
            'isActive' => ($currentContentContainer->id ?? null) === $space->id,
            'isVisible' => true,
        ]));
    }

Click on the "My space" top menu entry

What is the expected result?

The "My space" top menu entry is highlighted (active class is added).

What do you get instead?

It is not highlighted.

Additional info

This is due to this JS on https://github.com/humhub/humhub/blob/master/static/js/humhub/humhub.ui.navigation.js#L26:

$('#top-menu-nav').find('li').removeClass('active');

Q	A
HumHub version	1.15.0

[marc-farre]

@luke- I don't really understand the purpose of this code:

        .on('humhub:space:changed', function () {
            $('#top-menu-nav').find('li').removeClass('active');
        })

I've tested without and everything seams to work well when changing space.

Would you accept a PR where it's removed?
Or any other idea?

[luke-]

@marc-farre Good point, I think the Javascript event is used, especially in conjunction with PJAX to change an active space.

@yurabakhtin Any idea?

[yurabakhtin]

@marc-farre I have tested your code but cannot reproduce the issue, I see the top menu is active in all cases:

• Click on the "My space" top menu entry,
• Click on the "Welcome Space" in space chooser .

Could you show please more details how to make the menu item is not highlighted?
Thanks.

[ArchBlood]

Isn't this why MenuLink::isActiveState() is used in most cases for TopMenu?
I can see the issue @marc-farre is having, the "My Spaces" only shows the active state on hover, but due to the reported lines of code it removes the active state of the dropdown li tag and all li tags within the dropdown menu.

[marc-farre]

@yurabakhtin thanks for your tests.

So once you have the "My space" top menu entry added with your event:

1. Click on any other menu entry (e.g. "Dashboard")
2. Click on "My space" only once
3. You'll see that it gets highlighted a fraction of second, and then it's not anymore

If you click a second time, refresh the page or click any link in the space, the menu entry is highlighted.
The bug is when clicking for the first time on the "My space" menu entry, coming from another page.

The reason is https://github.com/humhub/humhub/blob/master/static/js/humhub/humhub.ui.navigation.js#L25C2-L27

screen-2023-12-11_16.33.21.mp4

[ArchBlood]

@yurabakhtin thanks for your tests.

So once you have the "My space" top menu entry added with your event:

1. Click on any other menu entry (e.g. "Dashboard")
2. Click on "My space" only once
3. You'll see that it gets highlighted a fraction of second, and then it's not anymore

If you click a second time, refresh the page or click any link in the space, the menu entry is highlighted. The bug is when clicking for the first time on the "My space" menu entry, coming from another page.

The reason is https://github.com/humhub/humhub/blob/master/static/js/humhub/humhub.ui.navigation.js#L25C2-L27

screen-2023-12-11_16.33.21.mp4

Why not use something like this then? You're not implementing an active state to begin with in your onTopMenuInit().

    public static function onTopMenuInit($event)
    {
        /** @var TopMenu $menu */
        $menu = $event->sender;
        $space = Space::findOne(1);
        $currentContentContainer = ContentContainerHelper::getCurrent();

        $menu->addEntry(new MenuLink([
            'label' => "My space",
            'url' => $space->getUrl(),
            'icon' => 'inbox',
            'sortOrder' => 100,
            'isActive' => MenuLink::isActiveState(($currentContentContainer->id ?? null) === $space->id),
            'isVisible' => true,
        ]));
    }

[marc-farre]

@ArchBlood thanks, but MenuLink::isActiveState() returns a bool value by comparing the params with the current module, controller and action.
($currentContentContainer->id ?? null) === $space->id is already returning the desire bool value.
This issue is about a JS problem, PHP is OK.

[ArchBlood]

@ArchBlood thanks, but MenuLink::isActiveState() returns a bool value by comparing the params with the current module, controller and action. ($currentContentContainer->id ?? null) === $space->id is already returning the desire bool value. This issue is about a JS problem, PHP is OK.

In your example all I am seeing is that it uses the null coalescing operator (??) to handle the case where $currentContentContainer->id might be null. If $currentContentContainer->id exists and is equal to $space->id, isActive will be set to true. Otherwise, it will be set to false. Therefore, indirectly, the isActive property will be assigned a boolean value based on the comparison result of IDs.

As I stated #6721 (comment) the JS code is used in the dropdown menu for My Spaces to remove the active state, the JS isn't much of an issue, don't get me wrong I don't see the need for it, but I also don't see the need to remove it either.

[yurabakhtin]

@marc-farre Ok thank you for details. I could reproduce the issue only on Opera browser, but I cannot reproduce this on Chrome and FireFox, I don't know why, I even restored default browser settings. It looks like pjax doesn't work on the browsers because I have debugged the method humhub.space.setSpace() https://github.com/humhub/humhub/blob/master/protected/humhub/modules/space/resources/js/humhub.space.js#L20-L27 with alert:

var setSpace = function(spaceOptions, pjax) {
    if(!module.options || module.options.guid !== spaceOptions.guid) {
        module.options = spaceOptions;
        if(pjax) {
alert('!!!!PJAX');
            event.trigger('humhub:space:changed', $.extend({}, module.options));
        }
    }
};

I can see the alert only on Opera.

---

Yes, it seems the code https://github.com/humhub/humhub/blob/master/static/js/humhub/humhub.ui.navigation.js#L25C2-L27 can be deleted because I don't find a reason:

        .on('humhub:space:changed', function () {
            $('#top-menu-nav').find('li').removeClass('active');
        })

But I am not sure fully, what is the code is needed for some case or theme where it should be reset?

---

I can suggest another solution to fix this:

• Add htmlOptions in your php code like this:

        $menu->addEntry(new MenuLink([
            'label' => "My space",
            ...,
            'htmlOptions' => ['data-space-guid' => $space->guid]
        ]));

• Modify the JS code .on('humhub:space:changed' to:

}).on('humhub:space:changed', function (evt, options) {
    $('#top-menu-nav').find('li').removeClass('active');
    if (typeof options.guid === 'string') {
        $('#top-menu-nav').find('a[data-space-guid=' + options.guid + ']').parent().addClass('active');
    }
});

What do you think?

[yurabakhtin]

@ArchBlood Thank you for the help, but we cannot pass a bool value into the method MenuLink::isActiveState() as MenuLink::isActiveState(($currentContentContainer->id ?? null) === $space->id), because declaration of the method is:

public static function isActiveState($moduleId = null, $controllerIds = [], $actionIds = [])

Samples of using:

• MenuLink::isActiveState('dashboard')
• MenuLink::isActiveState('user', 'people')
• MenuLink::isActiveState('admin', 'setting', ['oembed', 'oembed-edit'])

[marc-farre]

Thanks @yurabakhtin
On my tests, the problem occurs on Brave (Chrome based). Not tested with others.

Anyway, I would be in favor to simplify JS code and remove specific things in it, because it adds complexity and make Humhub more difficult to maintain with future updates and adding modules.

Would you be OK if I remove this code in the dev branch:

        .on('humhub:space:changed', function () {
            $('#top-menu-nav').find('li').removeClass('active');
        })

and if we find a bug because of this removal, we'll see what to do?

[yurabakhtin]

Would you be OK if I remove this code in the dev branch:

        .on('humhub:space:changed', function () {
            $('#top-menu-nav').find('li').removeClass('active');
        })

and if we find a bug because of this removal, we'll see what to do?

@marc-farre Ok, please try to create a PR for develop branch with removing the JS code because I don't see too why we need the code.

[marc-farre]

@luke- @yurabakhtin PR #6727