From 7b8fa9de865aa7a22eeb25d00fd92d28222a3b71 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Mon, 24 Oct 2022 09:57:54 +0200 Subject: [PATCH] MDL-75908 navigation: fix active flag in navigation for custom menus --- lib/amd/build/menu_navigation.min.js | Bin 4112 -> 4605 bytes lib/amd/build/menu_navigation.min.js.map | Bin 12969 -> 14854 bytes lib/amd/src/menu_navigation.js | 28 ++++ lib/classes/navigation/output/primary.php | 109 +++++++++++++- lib/tests/navigation/output/primary_test.php | 136 +++++++++++++++++- .../templates/primary-drawer-mobile.mustache | 6 +- 6 files changed, 272 insertions(+), 7 deletions(-) diff --git a/lib/amd/build/menu_navigation.min.js b/lib/amd/build/menu_navigation.min.js index 4871c55c3049436b6724028ddc1ed62879578641..30055e4cb5ddd3d13db748775dc2e7049d66d290 100644 GIT binary patch delta 481 zcmaJ;O-sWt7>3DY=-u;-Qd8DNJdcV)dK(DNn+J*MyS0!eHF?u1BKr%1e?-_n^C$== z!>ZuNDdhQjpS*eOzwJG5cTSRNHpxye&d#S-xzTm57pg?4xWKZ3^nkVCFrb&*lzb?h zF`zIdXdJA1otNG+S(M6K8PD^q@!X`e2nq}(HJ^hm^nxUG=9Ev>ipKXsD~tUQY13Q_ zS9uS+abWJIAVHuuV=3u>)#8zIQtqZ{2wB=FT8A+SYL3P6QG1_hlW|edui?;6FAU65 zU~|ARqA9N(0wnqtJkfa?mG&2t-JsL delta 17 ZcmeyXJV9XtH`8W6ruQ71pYw>Z0sua}21x(_ diff --git a/lib/amd/build/menu_navigation.min.js.map b/lib/amd/build/menu_navigation.min.js.map index d6e015c932f5c13b475c52059398c42fc66c4949..cea72ddf5759d06dc07458fe11e66da844ca0cbe 100644 GIT binary patch delta 2254 zcmaJ>QI8W<6ppxwCTJi=V~D{VHimZD%apEws43*$nQmEODN9)^fyCU-+;)bY-dkqw zv~-Q>8;?HFXP=FUF(&KZ@F)0S;=AuYdCr}dWkG1#>79Ge`ObH~^UeIMp4i`hU1WnI-akSY(;fw((%qW`#QVY31zOGe1`09wdFR zvC0%k{N+UtC@F_TgM=jz43#cCxa;CVsiCL9Xay=&cw(Wfja%uIqNIf+haxXznoc1! z+GZwARrEl^Ip|HP9^`74jaBBsbu&@8U@(c5hN&qaOp|brxhSnBgg_fAa?D_$Z0^n$ zMRO}HlFwnH(v+%WY5Zyfqs;V>U|Cg#vdEQ7#QzLZHIrjiB8}t#5p;wdV?l7YsgV)H zax_vp_n=GMh_=g(lLdvIL;-t+wMLhGO&XQyktypUlT78-WJtfCnV|y-Wf&_qgGpjz z=o!3_z6;^YwmjHc&IKXzB8-t-D0786DP)Oo)oBrMQoz62?QVuma8>_W1OR!oE0#i!k`XM8+LuVr6Au34S zY3Dn%scUzkRSAMdu9kexG#a3bG=;NgOHsX4=>f`o73$9#fEjk-;=R7fnmCf`vo6(P z6!nT9WY|*HQA`t!ipva*csSBHQ+<%uW(nG%G*f7XBYLJ1TMn9rw0I1m$WoZ$BSw3@ z%uLXcUfD%iZmF+>ZGiHy7+<1SE3{o%inq+5{#`t^KS?yc7`)VklIG^Q1C-;rfZ5TA zk+iXvLv@q}fgepfn&t4;p;E(o>ajP9N0rycFRQ>h9IqVa%>R1t!}*irYfnE|c|*bkO-`{!b-B}H~OT&|OQQ^S=UssWP+mtVj*FKo2>p~0}Ma?mq^k$MR6V^8VWV+Ue@SpQo;!xaz#J%2r>rIBu2p!Uf4e_Y2u^xomiTYVD8tKWmNm zHw3>P3(hVJzGVcv;PaL}sGnOob}aD^F0&uroaJ~@_iwW{=j~mV2+kA1?(v7$cj(5q z!e`r@?>zQdhx5+VUxbVUc17@JT~OlJc|d=ASO{acn{?A*E$%<`H{H(Xg0goU1YMiG zsbDv-;{HsqCg;sqFv+=OJKTRPum}M}bZsk_6gU$6rG206a(^6bzH|^ccouR#Kvplp zxn{a}>PO?vPfH3`AQs;px{lg}t3}I^+hmA=IHclw>yki$kgQ@p=e|U(xI`I-DAvB% zAnkY<(Dfc_h861K!u;P_`+VGFC=iNtF`)GDl3-ols`-hJx4}gpQv&VZa+&+{Z|i5a z_nJgpQBvP4=b4DHbBO#r6ZgJNQN{Uu$^)CfZ;nGRCEZx&?p&(s;GZ7Enl(%W7 zd^ZW$7Ux^F`9~XPPGIjyavi<6gEp$oZ)|uQs92ZtZXbv1f2G_SE!X@8l_Bx_Y$|vv LVxblsR=@ublF-!r delta 503 zcmZ{gO-n*i5QeD?Bm$wmh}I^Y@*_!wC}!r$P4Sb9n9=6?m8M-aMN4Sgvd#GqL7NEf zFKFAMpVFdD+s-*HT7;WnUYPftXXau0+HFcc6Atf%4(S;Xu@%+f)hyjmiLiJ^j6 mS%|0fessdpg|)1V;9O4kU}EO1rB`;~j&ML-YX1_1?tcIU#g*6q diff --git a/lib/amd/src/menu_navigation.js b/lib/amd/src/menu_navigation.js index 7b74016e9aa..696c950e6f7 100644 --- a/lib/amd/src/menu_navigation.js +++ b/lib/amd/src/menu_navigation.js @@ -26,6 +26,7 @@ const SELECTORS = { 'menuitem': '[role="menuitem"]', 'tab': '[role="tab"]', 'dropdowntoggle': '[data-toggle="dropdown"]', + 'dropdownitemactive': '.dropdown-item[aria-current="true"]', }; let openDropdownNode = null; @@ -84,6 +85,30 @@ const menuItemHelper = src => { } }; +/** + * Check if there are sub items in a dropdown menu. There can be one element active only. That is usually controlled + * by the server. However, when you click, the newly clicked item gets the active state as well. This is no problem + * because the user leaves the page and a new page load happens. When the user hits the back button, the old page dom + * is restored from the cache, with both menu items active. If there is such a case, we need to uncheck the item that + * was clicked when leaving this page. + * + */ +const dropDownMenuActiveCheck = function() { + const items = document.querySelectorAll(SELECTORS.dropdownitemactive); + // Do the check only, if there is more than one subitem active. + if (items !== null && items.length > 1) { + items.forEach(function(e) { + // Get the link target from the href attribute and compare it with the current url in the browser. + const href = e.getAttribute('href'); + if (href !== window.location.href && href !== window.location.pathname + && href !== window.location.href + '/index.php' && href !== window.location.pathname + 'index.php') { + e.classList.remove('active'); + e.removeAttribute('aria-current'); + } + }); + } +}; + /** * Defined keyboard event handling so we can remove listeners on nodes on resize etc. * @@ -180,6 +205,9 @@ export default elementRoot => { elementRoot.addEventListener('click', clickListenerEvents); }; +// We need this triggered only when the user hits the back button. +window.addEventListener('pageshow', dropDownMenuActiveCheck); + /** * Handle the focusing to the next element in the dropdown. * diff --git a/lib/classes/navigation/output/primary.php b/lib/classes/navigation/output/primary.php index 26aa8bfd723..d6e2812d156 100644 --- a/lib/classes/navigation/output/primary.php +++ b/lib/classes/navigation/output/primary.php @@ -55,9 +55,9 @@ class primary implements renderable, templatable { $output = $this->page->get_renderer('core'); } - $menudata = (object) array_merge($this->get_primary_nav(), $this->get_custom_menu($output)); + $menudata = (object) $this->merge_primary_and_custom($this->get_primary_nav(), $this->get_custom_menu($output)); $moremenu = new \core\navigation\output\more_menu($menudata, 'navbar-nav', false); - $mobileprimarynav = array_merge($this->get_primary_nav(), $this->get_custom_menu($output)); + $mobileprimarynav = $this->merge_primary_and_custom($this->get_primary_nav(), $this->get_custom_menu($output), true); $languagemenu = new \core\output\language_menu($this->page); @@ -116,6 +116,111 @@ class primary implements renderable, templatable { return $nodes; } + /** + * When defining custom menu items, the active flag is not obvserved correctly. Therefore, the merge of the primary + * and custom navigation must be handled a bit smarter. Change the "isactive" flag of the nodes (this may set by + * default in the primary nav nodes but is entirely missing in the custom nav nodes). + * Set the $expandedmenu argument to true when the menu for the mobile template is build. + * + * @param array $primary + * @param array $custom + * @param bool $expandedmenu + * @return array + */ + protected function merge_primary_and_custom(array $primary, array $custom, bool $expandedmenu = false): array { + if (empty($custom)) { + return $primary; // No custom nav, nothing to merge. + } + // Remember the amount of primary nodes and whether we changed the active flag in the custom menu nodes. + $primarylen = count($primary); + $changed = false; + foreach (array_keys($custom) as $i) { + if (!$changed) { + if ($this->flag_active_nodes($custom[$i], $expandedmenu)) { + $changed = true; + } + } + $primary[] = $custom[$i]; + } + // In case some custom node is active, mark all primary nav elements as inactive. + if ($changed) { + for ($i = 0; $i < $primarylen; $i++) { + $primary[$i]['isactive'] = false; + } + } + return $primary; + } + + /** + * Recursive checks if any of the children is active. If that's the case this node (the parent) is active as + * well. If the node has no children, check if the node itself is active. Use pass by reference for the node + * object because we actively change/set the "isactive" flag inside the method and this needs to be kept at the + * callers side. + * Set $expandedmenu to true, if the mobile menu is done, in this case the active flag gets the node that is + * actually active, while the parent hierarchy of the active node gets the flag isopen. + * + * @param object $node + * @param bool $expandedmenu + * @return bool + */ + protected function flag_active_nodes(object $node, bool $expandedmenu = false): bool { + global $FULLME; + $active = false; + foreach (array_keys($node->children ?? []) as $c) { + if ($this->flag_active_nodes($node->children[$c], $expandedmenu)) { + $active = true; + } + } + // One of the children is active, so this node (the parent) is active as well. + if ($active) { + if ($expandedmenu) { + $node->isopen = true; + } else { + $node->isactive = true; + } + return true; + } + + // By default, the menu item node to check is not active. + $node->isactive = false; + + // Check if the node url matches the called url. The node url may omit the trailing index.php, therefore check + // this as well. + if (empty($node->url)) { + // Current menu node has no url set, so it can't be active. + return false; + } + $nodeurl = parse_url($node->url); + $current = parse_url($FULLME ?? ''); + + $pathmatches = false; + // Exact match of the path of node and current url. + if ($nodeurl['path'] === $current['path']) { + $pathmatches = true; + } + // The current url may be trailed by a index.php, otherwise it's the same as the node path. + if (!$pathmatches && $nodeurl['path'] . 'index.php' === $current['path']) { + $pathmatches = true; + } + // No path did match, so the node can't be active. + if (!$pathmatches) { + return false; + } + // We are here because the path matches, so now look at the query string. + $nodequery = $nodeurl['query'] ?? ''; + $currentquery = $current['query'] ?? ''; + // If the node has no query string defined, then the patch match is sufficient. + if (empty($nodeurl['query'])) { + $node->isactive = true; + return true; + } + // If the node contains a query string then also the current url must match this query. + if ($nodequery === $currentquery) { + $node->isactive = true; + } + return $node->isactive; + } + /** * Get/Generate the user menu. * diff --git a/lib/tests/navigation/output/primary_test.php b/lib/tests/navigation/output/primary_test.php index 33b82783dff..649ff0cefe9 100644 --- a/lib/tests/navigation/output/primary_test.php +++ b/lib/tests/navigation/output/primary_test.php @@ -153,6 +153,17 @@ class primary_test extends \advanced_testcase { * @param array $expected */ public function test_get_custom_menu(string $config, array $expected) { + $actual = $this->get_custom_menu($config); + $this->assertEquals($expected, $actual); + } + + /** + * Helper method to get the template data for the custommenuitem that is set here via parameter. + * @param string $config + * @return array + * @throws \ReflectionException + */ + protected function get_custom_menu(string $config): array { global $CFG, $PAGE; $CFG->custommenuitems = $config; $output = new primary($PAGE); @@ -171,8 +182,7 @@ class primary_test extends \advanced_testcase { $actual = $method->invoke($output, $renderer); $custommenufilter($actual); - - $this->assertEquals($expected, $actual); + return $actual; } /** @@ -301,4 +311,126 @@ class primary_test extends \advanced_testcase { ] ]; } + + /** + * Test the merge_primary_and_custom and the eval_is_active method. Merge primary and custom menu with different + * page urls and check that the correct nodes are active and open, depending on the data for each menu. + * + * @covers \core\navigation\output\primary::merge_primary_and_custom + * @covers \core\navigation\output\primary::flag_active_nodes + * @return void + * @throws \ReflectionException + * @throws \moodle_exception + */ + public function test_merge_primary_and_custom() { + global $PAGE; + + $menu = $this->merge_and_render_menus(); + + $this->assertEquals(4, count(\array_keys($menu))); + $msg = 'No active nodes for page ' . $PAGE->url; + $this->assertEmpty($this->get_menu_item_names_by_type($menu, 'isactive'), $msg); + $this->assertEmpty($this->get_menu_item_names_by_type($menu, 'isopen'), str_replace('active', 'open', $msg)); + + $msg = 'Active nodes desktop for /course/search.php'; + $menu = $this->merge_and_render_menus('/course/search.php'); + $isactive = $this->get_menu_item_names_by_type($menu, 'isactive'); + $this->assertEquals(['Courses', 'Course search'], $isactive, $msg); + $this->assertEmpty($this->get_menu_item_names_by_type($menu, 'isopem'), str_replace('Active', 'Open', $msg)); + + $msg = 'Active nodes mobile for /course/search.php'; + $menu = $this->merge_and_render_menus('/course/search.php', true); + $isactive = $this->get_menu_item_names_by_type($menu, 'isactive'); + $this->assertEquals(['Course search'], $isactive, $msg); + $isopen = $this->get_menu_item_names_by_type($menu, 'isopen'); + $this->assertEquals(['Courses'], $isopen, str_replace('Active', 'Open', $msg)); + + $msg = 'Active nodes desktop for /course/search.php?areaids=core_course-course&q=test'; + $menu = $this->merge_and_render_menus('/course/search.php?areaids=core_course-course&q=test'); + $isactive = $this->get_menu_item_names_by_type($menu, 'isactive'); + $this->assertEquals(['Courses', 'Course search'], $isactive, $msg); + + $msg = 'Active nodes desktop for /?theme=boost'; + $menu = $this->merge_and_render_menus('/?theme=boost'); + $isactive = $this->get_menu_item_names_by_type($menu, 'isactive'); + $this->assertEquals(['Theme', 'Boost'], $isactive, $msg); + } + + /** + * Internal function to get an array of top menu items from the primary and the custom menu. The latter is defined + * in this function. + * @param string|null $url + * @param bool|null $ismobile + * @return array + * @throws \ReflectionException + * @throws \coding_exception + */ + protected function merge_and_render_menus(?string $url = null, ?bool $ismobile = false): array { + global $PAGE, $FULLME; + + if ($url !== null) { + $PAGE->set_url($url); + $FULLME = $PAGE->url->out(); + } + $primary = new primary($PAGE); + + $method = new ReflectionMethod('core\navigation\output\primary', 'get_primary_nav'); + $method->setAccessible(true); + $dataprimary = $method->invoke($primary); + + // Take this custom menu that would come from the setting custommenitems. + $custommenuitems = <<< ENDMENU + Theme + -Boost|/?theme=boost + -Classic|/?theme=classic + -Purge Cache|/admin/purgecaches.php + Courses + -All courses|/course/ + -Course search|/course/search.php + -### + -FAQ|https://example.org/faq + -My Important Course|/course/view.php?id=4 + Mobile app|https://example.org/app|Download our app + ENDMENU; + + $datacustom = $this->get_custom_menu($custommenuitems); + $method = new ReflectionMethod('core\navigation\output\primary', 'merge_primary_and_custom'); + $method->setAccessible(true); + $menucomplete = $method->invoke($primary, $dataprimary, $datacustom, $ismobile); + return $menucomplete; + } + + /** + * Traverse the menu array structure (all nodes recursively) and fetch the node texts from the menu nodes that are + * active/open (determined via param $nodetype that can be "inactive" or "isopen"). The returned array contains a + * list of nade names that match this criterion. + * @param array $menu + * @param string $nodetype + * @return array + */ + protected function get_menu_item_names_by_type(array $menu, string $nodetype): array { + $matchednodes = []; + foreach ($menu as $menuitem) { + // Either the node is an array. + if (is_array($menuitem)) { + if ($menuitem[$nodetype] ?? false) { + $matchednodes[] = $menuitem['text']; + } + // Recursively move through child items. + if (array_key_exists('children', $menuitem) && count($menuitem['children'])) { + $matchednodes = array_merge($matchednodes, $this->get_menu_item_names_by_type($menuitem['children'], $nodetype)); + } + } else { + // Otherwise the node is a standard object. + if (isset($menuitem->{$nodetype}) && $menuitem->{$nodetype} === true) { + $matchednodes[] = $menuitem->text; + } + // Recursively move through child items. + if (isset($menuitem->children) && is_array($menuitem->children) && !empty($menuitem->children)) { + $matchednodes = array_merge($matchednodes, $this->get_menu_item_names_by_type($menuitem->children, $nodetype)); + } + } + } + return $matchednodes; + } } diff --git a/theme/boost/templates/primary-drawer-mobile.mustache b/theme/boost/templates/primary-drawer-mobile.mustache index 22bc68f0c24..e15ac7eae6b 100644 --- a/theme/boost/templates/primary-drawer-mobile.mustache +++ b/theme/boost/templates/primary-drawer-mobile.mustache @@ -62,7 +62,7 @@
{{#mobileprimarynav}} {{#haschildren}} - {{{text}}} {{#pix}} t/expanded, core {{/pix}} @@ -77,10 +77,10 @@ -