MWPW-146129 App Launcher overlaps the menu in Unav in the devices #2123

sharmrj · 2024-04-09T07:23:20Z

Description

In non dekstop viewports, if the user clicks on one of the unav buttons while the menu is open, the popovers and the menu overlap. This PR brings the behavior in line with what happens on AEM.

Related Issue

Resolves: MWPW-146129

Testing instructions

See the JIRA ticket

URL for testing:

Before: https://main--milo--adobecom.hlx.live
After: https://unav-mobile--milo--sharmrj.hlx.live

…the user interacts with the universal nav (Notifications, app switcher, and profile/sign in button)

aem-code-sync · 2024-04-09T07:23:50Z

Page	Scores	Audits	Google
/

github-actions · 2024-04-10T01:08:15Z

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

mokimo · 2024-04-10T11:21:54Z

libs/blocks/global-navigation/global-navigation.js

@@ -280,6 +280,9 @@ class Gnav {
    if (this.useUniversalNav) {
      delete this.blocks.profile;
      this.blocks.universalNav = toFragment`<div class="feds-utilities"></div>`;
+      this.blocks.universalNav.addEventListener('click', () => {
+        document.querySelector('.feds-toggle[aria-expanded=true]')?.click();


There's a closeAllDropdowns method that you can call

mokimo · 2024-04-10T11:22:15Z

libs/blocks/global-navigation/global-navigation.js

@@ -280,6 +280,9 @@ class Gnav {
    if (this.useUniversalNav) {
      delete this.blocks.profile;
      this.blocks.universalNav = toFragment`<div class="feds-utilities"></div>`;
+      this.blocks.universalNav.addEventListener('click', () => {


Suggested change

this.blocks.universalNav.addEventListener('click', () => {

this.blocks.universalNav.addEventListener('click', () => closeAllDropdowns());

This seems not to work in adobe.com due to the following line (line 289 in utilities.js) in closeAllDropDowns()

if ('fedsPreventautoclose' in el.dataset) return;

Do you have a test link? Why does the element have the fedsPreventautoclose ?

IMO emulating a click on the open element is not the right path. You should investigate why fedsPreventautoclose exists to begin with (might be related to the keyboard navigation using arrow keys, escape, etc.) and otherwise look into adapting the closeAllDropdowns() method

I don't have a test link. The test links don't load the unav because the request for the JS file 403s. Since it's a one liner, I tested it by adding a breakpoint in the Gnav Constructor and adding in the line manually (you need to do cmd + s for it to work).

AFAICT it is, as you pointed out, related somewhat to the keyboard navigation files. It's difficult to say just from reading the code, but it seems that it makes somewhat of a distinction between specifically a dropdown and the menu as it opens in mobile and tablet viewports.
IMO closeAllDropdowns isn't what we're looking for here. What we're looking for specifically is inside the decorateToggle method:

const onToggleClick = async () => { const isExpanded = toggle.getAttribute('aria-expanded') === 'true'; toggle.setAttribute('aria-expanded', !isExpanded); this.elements.navWrapper.classList.toggle('feds-nav-wrapper--expanded', !isExpanded); closeAllDropdowns(); setCurtainState(!isExpanded); toggle.setAttribute('daa-ll', `hamburgermenu|${isExpanded ? 'open' : 'close'}`); if (this.blocks?.search?.instance) { this.blocks.search.instance.clearSearchForm(); } else { await this.loadSearch(); } if (isExpanded) setHamburgerPadding(); };

The most expedient way to call this function without a fairly significant refactor is by clicking on the element it's attached to. Furthermore it ties the behavior we want to the behavior of the actual html elements the end user interacts with, and not to the abstract way it is done in the code, which is liable to churn. In other words clicking on the toggle element decouples the implementation of the behavior (close the menu when we click on feds-utilities) from the implementation of how exactly clicking on the toggle hides/shows the menu.

I agree that we should generally avoid emulating click events, but in this scenario, it might be a reasonable approach. The closeAllDropdowns function is checking for open elements and whether they have the dataset fedsPreventautoclose before closing. However, in this particular case, we need to overlook that and ensure that all menus close when someone clicks on the navigation.
Another way would be for us to check open elements again, remove their dataset fedsPreventautoclose, and then call closeAllDropdowns, but in my opinion, that's not the right path to take. Therefore, using the click event seems fair in this scenario.

overmyheadandbody

I agree that this might not be the best approach. My hunch - this should actually work if it wasn't for the desktop viewport check here. We want to keep the behaviors between desktop and mobile separate, though, so I'd try adding an if/else. If the viewport is desktop, execute the current logic. If the viewport is mobile, close the hamburger menu. There's likely already logic that closes it (see the decorateToggle method), you might have to expose it to be able to simply re-use is where needed.

…bile

sharmrj · 2024-04-17T04:03:35Z

I'm not sure why unit tests are failing. The global-navigation tests seem to be passing (and global-navigation is the only modue I've touched).

overmyheadandbody · 2024-04-17T12:18:36Z

libs/blocks/global-navigation/utilities/utilities.js

+export const toggleMenuMobile = (toggle) => {
+  if (!toggle) return;
+  const isExpanded = toggle.getAttribute('aria-expanded') === 'true';
+  toggle.setAttribute('aria-expanded', !isExpanded);
+  document.querySelector(selectors.navWrapper)?.classList?.toggle('feds-nav-wrapper--expanded', !isExpanded);
+  closeAllDropdowns();
+  setCurtainState(!isExpanded);
+  toggle.setAttribute('daa-ll', `hamburgermenu|${isExpanded ? 'open' : 'close'}`);
+};
+


I'd argue that since this isn't used in multiple files, it can be left in the global-navigation.js one. You would then have direct access to this.elements.navWrapper and toggle and you'd keep the logic close to where it's being used.

…bile

sigadamvenkata · 2024-04-18T05:44:14Z

Verified the changes and we done see overlap issue now. PF below screenshots

overmyheadandbody

Let's add some relevant pages to the PR body where the hamburger menu interactions can actually be tested.

overmyheadandbody · 2024-04-19T09:27:32Z

libs/blocks/global-navigation/global-navigation.js

@@ -615,6 +619,16 @@ class Gnav {
    });
  };

+  toggleMenuMobile = () => {
+    const toggle = this.elements.mobileToggle;
+    const isExpanded = toggle?.getAttribute('aria-expanded') === 'true';


Same exact code is used on L284 & L656, might be worth turning it into a function

const isToggleExpanded = () => this.elements.mobileToggle?.getAttribute('aria-expanded') === 'true';

…bile

overmyheadandbody

You could trim down a couple more lines, added comments, otherwise looks good.

Personally, I would have still integrated this into the closeOnClickOutside logic, which would have also covered the use-case where a user clicks anywhere outside the hamburger menu area, but maybe that can happen in the future.

overmyheadandbody · 2024-04-22T14:09:51Z

libs/blocks/global-navigation/global-navigation.js

-      setCurtainState(!isExpanded);
-      toggle.setAttribute('daa-ll', `hamburgermenu|${isExpanded ? 'open' : 'close'}`);
+      this.toggleMenuMobile();
+      const isExpanded = this.isToggleExpanded();


You're just using the value of this variable once, you could simply use the function call on L666, where it's used.

overmyheadandbody · 2024-04-22T14:11:48Z

libs/blocks/global-navigation/global-navigation.js

+        const isExpanded = this.isToggleExpanded();
+        if (isExpanded) this.toggleMenuMobile();


This could be just one line, since isExpanded is not used anywhere else, if (this.isToggleExpanded()) this.toggleMenuMobile();

sharmrj · 2024-04-23T03:22:55Z

Personally, I would have still integrated this into the closeOnClickOutside logic, which would have also covered the use-case where a user clicks anywhere outside the hamburger menu area, but maybe that can happen in the future.

The issue with that is that it changes the current behavior in mobile viewports where tapping in the thin area outside the menu (when it's open) does nothing. I'm not sure if the current behavior should change.

…bile

…in the devices (#2184) * Closes the global nav expanded wrapper in non desktop viewports when the user interacts with the universal nav (Notifications, app switcher, and profile/sign in button) * We no longer use element.click * made toggleMenuMobile a method of the Gnav class * Extracted the toggle expanded check into its own function * a little cleanup

* stage: MWPW-146999: kodiak CVE 25883 (adobecom#2183) MWPW-144549 CTA alignment for Text, Icon, and Media Blocks (adobecom#2098) MWPW-146494-keep gnav sticky when branch banner is displayed (adobecom#2175) MWPW-135821 introduce mep custom action & use it for card collection (adobecom#2152) MWPW-146129 [Original: adobecom#2123] App Launcher overlaps the menu in Unav in the devices (adobecom#2184) Revert "MWPW-146129 App Launcher overlaps the menu in Unav in the devices" (adobecom#2182) MWPW-146129 App Launcher overlaps the menu in Unav in the devices (adobecom#2123) MWPW-139842 [Revert] Fill button style (adobecom#2181) Mwpw 142003: Mini Compare Chart card fixes (adobecom#2093) MWPW-146756] Add support for RTL in aside notifications, horizontal cards"" (adobecom#2178) Revert "[MWPW-146756] Add support for RTL in aside notifications, horizontal cards" (adobecom#2177) [MWPW-146756] Add support for RTL in aside notifications, horizontal cards (adobecom#2167) Revert "MWPW-142590 - [Share] enhancement - ability to edit text above icons" (adobecom#2172)

…obecom#2123) * Closes the global nav expanded wrapper in non desktop viewports when the user interacts with the universal nav (Notifications, app switcher, and profile/sign in button) * We no longer use element.click * made toggleMenuMobile a method of the Gnav class * Extracted the toggle expanded check into its own function * a little cleanup

…in Unav in the devices (adobecom#2184) * Closes the global nav expanded wrapper in non desktop viewports when the user interacts with the universal nav (Notifications, app switcher, and profile/sign in button) * We no longer use element.click * made toggleMenuMobile a method of the Gnav class * Extracted the toggle expanded check into its own function * a little cleanup

Closes the global nav expanded wrapper in non desktop viewports when …

920c33c

…the user interacts with the universal nav (Notifications, app switcher, and profile/sign in button)

sharmrj requested review from overmyheadandbody, mokimo and narcis-radu as code owners April 9, 2024 07:23

sharmrj added bug Something isn't working trivial PR doesn't require E2E testing by a reviewer labels Apr 9, 2024

sharmrj requested review from bandana147, Deva309 and nishantka April 10, 2024 06:03

Merge branch 'stage' into unav-mobile

beaf630

mokimo reviewed Apr 10, 2024

View reviewed changes

Merge branch 'stage' into unav-mobile

18e5aae

overmyheadandbody requested changes Apr 11, 2024

View reviewed changes

sharmrj added 3 commits April 12, 2024 08:25

Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…

bcf845d

…bile

Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…

f1ac544

…bile

We no longer use element.click

65c597f

sharmrj requested review from mokimo and overmyheadandbody April 17, 2024 03:58

overmyheadandbody reviewed Apr 17, 2024

View reviewed changes

sharmrj added 2 commits April 18, 2024 09:18

made toggleMenuMobile a method of the Gnav class

91ad1b9

Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…

1915b2a

…bile

sigadamvenkata added the verified PR has been E2E tested by a reviewer label Apr 18, 2024

sharmrj requested a review from overmyheadandbody April 19, 2024 04:16

salonijain3 approved these changes Apr 19, 2024

View reviewed changes

overmyheadandbody reviewed Apr 19, 2024

View reviewed changes

sharmrj added 2 commits April 22, 2024 09:24

Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…

9414bfc

…bile

Extracted the toggle expanded check into its own function

b8e6bd7

sharmrj requested a review from overmyheadandbody April 22, 2024 04:01

overmyheadandbody approved these changes Apr 22, 2024

View reviewed changes

sharmrj added 2 commits April 23, 2024 08:53

Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…

19d2886

…bile

a little cleanup

925972c

bandana147 added the Ready for Stage label Apr 23, 2024

drashti1712 merged commit 743ce09 into adobecom:stage Apr 23, 2024
12 checks passed

This was referenced Apr 23, 2024

[Release] Stage to Main #2179

Merged

Revert "MWPW-146129 App Launcher overlaps the menu in Unav in the devices" #2182

Merged

sharmrj mentioned this pull request Apr 23, 2024

MWPW-146129 [Original: #2123] App Launcher overlaps the menu in Unav in the devices #2184

Merged

github-actions bot mentioned this pull request Apr 30, 2024

[Release] Stage to Main mokimo/milo#57

Closed

mokimo mentioned this pull request May 7, 2024

[Release] Stage to Main mokimo/milo#59

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MWPW-146129 App Launcher overlaps the menu in Unav in the devices #2123

MWPW-146129 App Launcher overlaps the menu in Unav in the devices #2123

sharmrj commented Apr 9, 2024 •

edited

Loading

aem-code-sync bot commented Apr 9, 2024 •

edited

Loading

github-actions bot commented Apr 10, 2024

mokimo Apr 10, 2024

mokimo Apr 10, 2024 •

edited

Loading

sharmrj Apr 10, 2024

mokimo Apr 10, 2024

mokimo Apr 10, 2024

sharmrj Apr 10, 2024 •

edited

Loading

sharmrj Apr 10, 2024 •

edited

Loading

bandana147 Apr 11, 2024

overmyheadandbody left a comment

sharmrj commented Apr 17, 2024

overmyheadandbody Apr 17, 2024

sigadamvenkata commented Apr 18, 2024

overmyheadandbody left a comment •

edited

Loading

overmyheadandbody Apr 19, 2024

overmyheadandbody left a comment

overmyheadandbody Apr 22, 2024

overmyheadandbody Apr 22, 2024

sharmrj commented Apr 23, 2024

	this.blocks.universalNav.addEventListener('click', () => {
	this.blocks.universalNav.addEventListener('click', () => closeAllDropdowns());

		const isExpanded = this.isToggleExpanded();
		if (isExpanded) this.toggleMenuMobile();

MWPW-146129 App Launcher overlaps the menu in Unav in the devices #2123

MWPW-146129 App Launcher overlaps the menu in Unav in the devices #2123

Conversation

sharmrj commented Apr 9, 2024 • edited Loading

Description

Related Issue

Testing instructions

aem-code-sync bot commented Apr 9, 2024 • edited Loading

github-actions bot commented Apr 10, 2024

Choose a reason for hiding this comment

mokimo Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharmrj Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

sharmrj Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overmyheadandbody left a comment

Choose a reason for hiding this comment

sharmrj commented Apr 17, 2024

Choose a reason for hiding this comment

sigadamvenkata commented Apr 18, 2024

overmyheadandbody left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overmyheadandbody left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharmrj commented Apr 23, 2024

sharmrj commented Apr 9, 2024 •

edited

Loading

aem-code-sync bot commented Apr 9, 2024 •

edited

Loading

mokimo Apr 10, 2024 •

edited

Loading

sharmrj Apr 10, 2024 •

edited

Loading

sharmrj Apr 10, 2024 •

edited

Loading

overmyheadandbody left a comment •

edited

Loading