Merge branch '199211-fix-user-popover-glitch' into 'master'

Fix user popover glitch

Closes #199211

See merge request gitlab-org/gitlab!23904

app/assets/javascripts/notes/components/notes_app.vue

@@ -14,7 +14,7 @@ import placeholderSystemNote from '../../vue_shared/components/notes/placeholder
 import skeletonLoadingContainer from '../../vue_shared/components/notes/skeleton_note.vue';
 import highlightCurrentUser from '~/behaviors/markdown/highlight_current_user';
 import { __ } from '~/locale';
-import initUserPopovers from '../../user_popovers';
+import initUserPopovers from '~/user_popovers';
 
 export default {
   name: 'NotesApp',

app/assets/javascripts/user_popovers.js

@@ -3,108 +3,92 @@ import Vue from 'vue';
 import UsersCache from './lib/utils/users_cache';
 import UserPopover from './vue_shared/components/user_popover/user_popover.vue';
 
-let renderedPopover;
-let renderFn;
+const removeTitle = el => {
+  // Removing titles so its not showing tooltips also
 
-const handleUserPopoverMouseOut = event => {
-  const { target } = event;
-  target.removeEventListener('mouseleave', handleUserPopoverMouseOut);
+  el.dataset.originalTitle = '';
+  el.setAttribute('title', '');
+};
 
-  if (renderFn) {
-    clearTimeout(renderFn);
-  }
-  if (renderedPopover) {
-    renderedPopover.$destroy();
-    renderedPopover = null;
-  }
-  target.removeAttribute('aria-describedby');
+const getPreloadedUserInfo = dataset => {
+  const userId = dataset.user || dataset.userId;
+  const { username, name, avatarUrl } = dataset;
+
+  return {
+    userId,
+    username,
+    name,
+    avatarUrl,
+  };
 };
 
 /**
  * Adds a UserPopover component to the body, hands over as much data as the target element has in data attributes.
  * loads based on data-user-id more data about a user from the API and sets it on the popover
  */
-const handleUserPopoverMouseOver = event => {
-  const { target } = event;
-  // Add listener to actually remove it again
-  target.addEventListener('mouseleave', handleUserPopoverMouseOut);
-
-  renderFn = setTimeout(() => {
-    // Helps us to use current markdown setup without maybe breaking or duplicating for now
-    if (target.dataset.user) {
-      target.dataset.userId = target.dataset.user;
-      // Removing titles so its not showing tooltips also
-      target.dataset.originalTitle = '';
-      target.setAttribute('title', '');
+const populateUserInfo = user => {
+  const { userId } = user;
+
+  return Promise.all([UsersCache.retrieveById(userId), UsersCache.retrieveStatusById(userId)]).then(
+    ([userData, status]) => {
+      if (userData) {
+        Object.assign(user, {
+          avatarUrl: userData.avatar_url,
+          username: userData.username,
+          name: userData.name,
+          location: userData.location,
+          bio: userData.bio,
+          organization: userData.organization,
+          loaded: true,
+        });
       }
 
-    const { userId, username, name, avatarUrl } = target.dataset;
+      if (status) {
+        Object.assign(user, {
+          status,
+        });
+      }
+
+      return user;
+    },
+  );
+};
+
+export default (elements = document.querySelectorAll('.js-user-link')) => {
+  const userLinks = Array.from(elements);
+
+  return userLinks.map(el => {
+    const UserPopoverComponent = Vue.extend(UserPopover);
     const user = {
-      userId,
-      username,
-      name,
-      avatarUrl,
       location: null,
       bio: null,
       organization: null,
       status: null,
       loaded: false,
     };
-    if (userId || username) {
-      const UserPopoverComponent = Vue.extend(UserPopover);
-      renderedPopover = new UserPopoverComponent({
+    const renderedPopover = new UserPopoverComponent({
       propsData: {
-          target,
+        target: el,
         user,
       },
     });
 
     renderedPopover.$mount();
 
-      UsersCache.retrieveById(userId)
-        .then(userData => {
-          if (!userData) {
-            return undefined;
-          }
-
-          Object.assign(user, {
-            avatarUrl: userData.avatar_url,
-            username: userData.username,
-            name: userData.name,
-            location: userData.location,
-            bio: userData.bio,
-            organization: userData.organization,
-            status: userData.status,
-            loaded: true,
-          });
+    el.addEventListener('mouseenter', ({ target }) => {
+      removeTitle(target);
+      const preloadedUserInfo = getPreloadedUserInfo(target.dataset);
 
-          if (userData.status) {
-            return Promise.resolve();
-          }
+      Object.assign(user, preloadedUserInfo);
 
-          return UsersCache.retrieveStatusById(userId);
-        })
-        .then(status => {
-          if (!status) {
-            return;
+      if (preloadedUserInfo.userId) {
+        populateUserInfo(user);
       }
-
-          Object.assign(user, {
-            status,
     });
-        })
-        .catch(() => {
-          renderedPopover.$destroy();
-          renderedPopover = null;
+    el.addEventListener('mouseleave', ({ target }) => {
+      target.removeAttribute('aria-describedby');
     });
-    }
-  }, 200); // 200ms delay so not every mouseover triggers Popover + API Call
-};
-
-export default elements => {
-  const userLinks = elements || [...document.querySelectorAll('.js-user-link')];
 
-  userLinks.forEach(el => {
-    el.addEventListener('mouseenter', handleUserPopoverMouseOver);
+    return renderedPopover;
   });
 };

app/assets/javascripts/vue_shared/components/user_popover/user_popover.vue

@@ -56,19 +56,16 @@ export default {
 </script>
 
 <template>
-  <gl-popover :target="target" boundary="viewport" placement="top" offset="0, 1" show>
+  <!-- 200ms delay so not every mouseover triggers Popover -->
+  <gl-popover :target="target" :delay="200" boundary="viewport" triggers="hover" placement="top">
     <div class="user-popover d-flex">
       <div class="p-1 flex-shrink-1">
         <user-avatar-image :img-src="user.avatarUrl" :size="60" css-classes="mr-2" />
       </div>
       <div class="p-1 w-100">
         <h5 class="m-0">
-          {{ user.name }}
-          <gl-skeleton-loading
-            v-if="nameIsLoading"
-            :lines="1"
-            class="animation-container-small mb-1"
-          />
+          <span v-if="user.name">{{ user.name }}</span>
+          <gl-skeleton-loading v-else :lines="1" class="animation-container-small mb-1" />
         </h5>
         <div class="text-secondary mb-2">
           <span v-if="user.username">@{{ user.username }}</span>

changelogs/unreleased/199211-fix-user-popover-glitch.yml

+---
+title: Fix user popover glitch
+merge_request: 23904
+author:
+type: fixed

spec/features/projects/user_sees_user_popover_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'User sees user popover', :js do
+  set(:project) { create(:project, :repository) }
+
+  let(:user) { project.creator }
+  let(:merge_request) do
+    create(:merge_request, source_project: project, target_project: project)
+  end
+
+  before do
+    project.add_maintainer(user)
+    sign_in(user)
+  end
+
+  subject { page }
+
+  describe 'hovering over a user link in a merge request' do
+    before do
+      visit project_merge_request_path(project, merge_request)
+    end
+
+    it 'displays user popover' do
+      popover_selector = '.user-popover'
+
+      find('.js-user-link').hover
+
+      expect(page).to have_css(popover_selector, visible: true)
+
+      page.within(popover_selector) do
+        expect(page).to have_content(user.name)
+      end
+    end
+  end
+end

spec/frontend/notes/components/note_app_spec.js

@@ -12,6 +12,8 @@ import '~/behaviors/markdown/render_gfm';
 import * as mockData from '../../notes/mock_data';
 import * as urlUtility from '~/lib/utils/url_utility';
 
+jest.mock('~/user_popovers', () => jest.fn());
+
 setTestTimeout(1000);
 
 describe('note_app', () => {

spec/javascripts/user_popovers_spec.js

@@ -10,9 +10,14 @@ describe('User Popovers', () => {
   const dummyUser = { name: 'root' };
   const dummyUserStatus = { message: 'active' };
 
+  let popovers;
+
   const triggerEvent = (eventName, el) => {
-    const event = document.createEvent('MouseEvents');
-    event.initMouseEvent(eventName, true, true, window);
+    const event = new MouseEvent(eventName, {
+      bubbles: true,
+      cancelable: true,
+      view: window,
+    });
 
     el.dispatchEvent(event);
   };
@@ -26,46 +31,54 @@ describe('User Popovers', () => {
     const userStatusCacheSpy = () => Promise.resolve(dummyUserStatus);
     spyOn(UsersCache, 'retrieveStatusById').and.callFake(userId => userStatusCacheSpy(userId));
 
-    initUserPopovers(document.querySelectorAll('.js-user-link'));
+    popovers = initUserPopovers(document.querySelectorAll(selector));
   });
 
-  it('Should Show+Hide Popover on mouseenter and mouseleave', done => {
-    const targetLink = document.querySelector(selector);
-    const { userId } = targetLink.dataset;
-    triggerEvent('mouseenter', targetLink);
-
-    setTimeout(() => {
-      const shownPopover = document.querySelector('.popover');
+  it('initializes a popover for each js-user-link element found in the document', () => {
+    expect(document.querySelectorAll(selector).length).toBe(popovers.length);
+  });
 
-      expect(shownPopover).not.toBeNull();
-      expect(targetLink.getAttribute('aria-describedby')).not.toBeNull();
+  describe('when user link emits mouseenter event', () => {
+    let userLink;
 
-      expect(shownPopover.innerHTML).toContain(dummyUser.name);
-      expect(UsersCache.retrieveById).toHaveBeenCalledWith(userId.toString());
+    beforeEach(() => {
+      userLink = document.querySelector(selector);
 
-      triggerEvent('mouseleave', targetLink);
+      triggerEvent('mouseenter', userLink);
+    });
 
-      setTimeout(() => {
-        // After Mouse leave it should be hidden now
-        expect(document.querySelector('.popover')).toBeNull();
-        expect(targetLink.getAttribute('aria-describedby')).toBeNull();
-        done();
+    it('removes title attribute from user links', () => {
+      expect(userLink.getAttribute('title')).toBeFalsy();
+      expect(userLink.dataset.originalTitle).toBeFalsy();
     });
-    }, 210); // We need to wait until the 200ms mouseover delay is over, only then the popover will be visible
+
+    it('populates popovers with preloaded user data', () => {
+      const { name, userId, username } = userLink.dataset;
+      const [firstPopover] = popovers;
+
+      expect(firstPopover.$props.user).toEqual(
+        jasmine.objectContaining({
+          name,
+          userId,
+          username,
+        }),
+      );
     });
 
-  it('Should Not show a popover on short mouse over', done => {
-    const targetLink = document.querySelector(selector);
-    const { userId } = targetLink.dataset;
-    triggerEvent('mouseenter', targetLink);
+    it('fetches user info and status from the user cache', () => {
+      const { userId } = userLink.dataset;
 
-    setTimeout(() => {
-      expect(document.querySelector('.popover')).toBeNull();
-      expect(UsersCache.retrieveById).not.toHaveBeenCalledWith(userId.toString());
+      expect(UsersCache.retrieveById).toHaveBeenCalledWith(userId);
+      expect(UsersCache.retrieveStatusById).toHaveBeenCalledWith(userId);
+    });
+  });
 
-      triggerEvent('mouseleave', targetLink);
+  it('removes aria-describedby attribute from the user link on mouseleave', () => {
+    const userLink = document.querySelector(selector);
 
-      done();
-    });
+    userLink.setAttribute('aria-describedby', 'popover');
+    triggerEvent('mouseleave', userLink);
+
+    expect(userLink.getAttribute('aria-describedby')).toBe(null);
   });
 });