Fix: Remove focus trap on AnnotationPopover on mobile (#279)

* Allow clicking on other highlights when createHighlightDialog is open * Exiting annotations modes properly re-binds handlers
box · Nov 4, 2018 · a14fe6b · a14fe6b
1 parent 0c7fe0f
commit a14fe6b
Show file tree

Hide file tree

Showing 21 changed files with 378 additions and 52 deletions.
diff --git a/src/Annotator.scss b/src/Annotator.scss
@@ -168,7 +168,8 @@
 
 // Quad point positioning - the helper divs are positioned relative to the Rangy-created element
 .bp-doc .rangy-highlight {
-    background-color: #ff0;
+    background-color: lighten($highlight-yellow, 10%);
+    opacity: .25;
     position: relative;
 }
 

diff --git a/src/components/ActionControls/ActionControls.scss b/src/components/ActionControls/ActionControls.scss
@@ -21,14 +21,14 @@
         margin: 0;
     }
 
-    .ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw,
-    .ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight {
+    .ba-annotator-label ~ .ba-action-controls .ba-action-controls-draw,
+    .ba-annotator-label ~ .ba-action-controls .ba-action-controls-highlight {
         border-left: 1px solid $see-see;
         margin-left: 5px;
     }
 
     // TABLET-SPECIFIC CSS
-    @media #{$tablet} {
+    @media #{$mobile}, #{$tablet} {
         .ba-create-popover .ba-action-controls {
             border: none;
             box-shadow: none;
@@ -38,9 +38,10 @@
             width: 100%;
         }
 
-        .ba-annotation-label ~ .ba-action-controls .ba-action-controls-draw,
-        .ba-annotation-label ~ .ba-action-controls .ba-action-controls-highlight {
+        .ba-annotator-label ~ .ba-action-controls .ba-action-controls-draw,
+        .ba-annotator-label ~ .ba-action-controls .ba-action-controls-highlight {
             border-left: 0;
+            margin: 0;
         }
     }
 

diff --git a/src/components/AnnotationPopover/AnnotationPopover.js b/src/components/AnnotationPopover/AnnotationPopover.js
@@ -2,11 +2,11 @@
 import React from 'react';
 import classNames from 'classnames';
 import noop from 'lodash/noop';
-import Overlay from 'box-react-ui/lib/components/flyout/Overlay';
 import PlainButton from 'box-react-ui/lib/components/plain-button';
 import IconClose from 'box-react-ui/lib/icons/general/IconClose';
 
 import Internationalize from '../Internationalize';
+import Overlay from './Overlay';
 import CommentList from '../CommentList';
 import { TYPES, CLASS_ANNOTATION_POPOVER } from '../../constants';
 
@@ -93,8 +93,7 @@ class AnnotationPopover extends React.PureComponent<Props> {
                     ) : (
                         <span className='ba-popover-caret' />
                     )}
-
-                    <Overlay className='ba-popover-overlay'>
+                    <Overlay shouldDefaultFocus={!isMobile}>
                         {hasComments ? (
                             <CommentList comments={comments} onDelete={onDelete} />
                         ) : (

diff --git a/src/components/AnnotationPopover/AnnotatorLabel.js b/src/components/AnnotationPopover/AnnotatorLabel.js
@@ -42,7 +42,7 @@ class AnnotatorLabel extends React.PureComponent<Props> {
         const { id, isPending } = this.props;
         return (
             !isPending && (
-                <span className='ba-annotation-label'>
+                <span className='ba-annotator-label'>
                     <CommentText id={id} tagged_message={this.getAnnotatorLabelMessage()} translationEnabled={false} />
                 </span>
             )

diff --git a/src/components/AnnotationPopover/AnnotatorLabel.scss b/src/components/AnnotationPopover/AnnotatorLabel.scss
@@ -1,15 +1,17 @@
 @import '../../variables';
 
 .ba {
-    .ba-annotation-label {
+    .ba-annotator-label {
         line-height: 23px;
     }
 
     // MOBILE & TABLET CSS
     @media #{$mobile}, #{$tablet} {
-        .ba-annotation-label {
+        .ba-annotator-label {
             background: white;
             border-bottom: 1px solid #ccc;
+            display: flex;
+            justify-content: center;
             left: 0;
             padding: 20px;
             position: fixed;

diff --git a/src/components/AnnotationPopover/Overlay.js b/src/components/AnnotationPopover/Overlay.js
@@ -0,0 +1,23 @@
+// @flow
+import * as React from 'react';
+
+import OverlayComponent from 'box-react-ui/lib/components/flyout/Overlay';
+
+type Props = {
+    shouldDefaultFocus: boolean,
+    children: React.Node
+};
+
+const Overlay = ({ shouldDefaultFocus, children }: Props) => {
+    if (shouldDefaultFocus) {
+        return (
+            <OverlayComponent className='ba-popover-overlay' shouldOutlineFocus={false}>
+                {children}
+            </OverlayComponent>
+        );
+    }
+
+    return <div className='ba-popover-overlay'>{children}</div>;
+};
+
+export default Overlay;
diff --git a/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js b/src/components/AnnotationPopover/__tests__/AnnotationPopover-test.js
@@ -73,6 +73,26 @@ describe('components/AnnotationPopover', () => {
         expect(wrapper.find('.ba-inline').length).toEqual(1);
     });
 
+    test('should correctly render a BRUI Overlay if not on mobile', () => {
+        const wrapper = render({
+            canAnnotate: true,
+            comments,
+            isMobile: false
+        });
+        expect(wrapper).toMatchSnapshot();
+        expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeTruthy();
+    });
+
+    test('should correctly render a div without a Focus Trap if on mobile', () => {
+        const wrapper = render({
+            canAnnotate: true,
+            comments,
+            isMobile: true
+        });
+        expect(wrapper).toMatchSnapshot();
+        expect(wrapper.find('Overlay').prop('shouldDefaultFocus')).toBeFalsy();
+    });
+
     test('should correctly render a popover with comments and reply textarea', () => {
         const wrapper = render({
             canAnnotate: true,

diff --git a/src/components/AnnotationPopover/__tests__/Overlay-test.js b/src/components/AnnotationPopover/__tests__/Overlay-test.js
@@ -0,0 +1,18 @@
+import * as React from 'react';
+import { shallow } from 'enzyme';
+
+import Overlay from '../Overlay';
+
+describe('components/Overlay', () => {
+    test('should correctly render a Focus Trap if not on mobile', () => {
+        const wrapper = shallow(<Overlay shouldDefaultFocus={true} />).dive();
+        expect(wrapper).toMatchSnapshot();
+        expect(wrapper.find('FocusTrap').length).toEqual(1);
+    });
+
+    test('should correctly render a div without a Focus Trap if on mobile', () => {
+        const wrapper = shallow(<Overlay shouldDefaultFocus={false} />);
+        expect(wrapper).toMatchSnapshot();
+        expect(wrapper.find('FocusTrap').length).toEqual(0);
+    });
+});