Bugfix: set active navigation node when smartmenus are enabled, resoves #620, #384 and #694.

[Menrath]

Another summer time theme_boost_union fix.

This targets at solving the issues #620, #384 and #694.

In the file classes/output/navigation/primary.php when smartmenus were enabled the parent:export_for_template does not get called ( parent class is core\navigation\output\primary).

Cause of the bug: The menu with the smartmenu nodes are just merged with the primary navigation and the custommenus via the PHP function array_merge.

Fix: Now the smartmenus and the custommenus are merged via array_merge and later merged with the primary menu nodes via the function merge_primary_and_custom. This function checks for the active node, the same way it happens when smartmenus are disabled.

Maybe this should also be adjusted when the variable $mobileprimarynav gets set, too.

To-Do:

• Write new acceptance tests that cover the flag

At reviewers:
There is still a bug when clicking on external static link in the smartmenu and using the browsers return function, that multiple nodes (the actual active one and the external one) are shown as active. But I don't have any idea how to fix this edge case.

[Menrath]

Something I am not completely happy with concerning the tests which is way I select the smartmenu top level menu items: .primary-navigation [data-orgposition='5'].

There seems to be no way to set the data-key for the smartmenus. Maybe one should be auto-generated by the title/text. Or is there a smarter more robust solution concerning the tests?

[wiebkemueller-hsh]

Hello @Menrath as discussed earlier, could you have a look at this issue while at it? There might be synergies. #694

[Menrath]

The behat tests now fail for the newest MOODLE_404_STABLE, but I think the issue is not related with this PR.

[abias]

Hi @Menrath ,

many thanks for working on this PR for this set of issues.

Unfortunately, I have to say that these issues are already addressed by #648 which adopts an upstream change from Moodle core (https://tracker.moodle.org/browse/MDL-77732) which was affected by the same highlighting issues (in the Moodle core custom menu).

#648 will be merged soon and the issues are solved as a first step. This means that your PR is not needed anymore in its full extend, I am really sorry for the time which you have spent.

However you raised two additional items:

There is still a bug when clicking on external static link in the smartmenu and using the browsers return function, that multiple nodes (the actual active one and the external one) are shown as active. But I don't have any idea how to fix this edge case.

I cannot reproduce that finding with the patch from #648.
I would appreciate if if you could re-test as soon as #648 is merged and submit a new issue if the problem persists for you.

Write new acceptance tests that cover the flag

Your PR does not only fix the bug, but also adds Behat tests to cover the fix. I really appreciate that.
May I ask you to change this PR in a way that just the Behat test is added and works with the patch from #648? That would be awesome.
And it would be even more awesome if you could write a second Behat test which adds a custom menu (on admin/settings.php?section=themesettingsadvanced) instead of a smart menu and verifies that Boost Union does not break the highlighting of the custom menu as well.

Many thanks in advance,
Alex

[abias]

Update: I went ahead and modified the Behat tests myself and will update this PR here shortly.

[abias]

I rebased and force-pushed the branch and will merge it as soon as the tests are green again.