Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Fix: BalloonPanelView should not be focusable. Closes #206.
Browse files Browse the repository at this point in the history
T/206a: BalloonPanelView should not be focusable
  • Loading branch information
oskarwrobel authored Apr 24, 2017
2 parents b8fbaf1 + 270a1a4 commit f9e3bb7
Show file tree
Hide file tree
Showing 8 changed files with 134 additions and 14 deletions.
23 changes: 23 additions & 0 deletions src/bindings/preventdefault.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module ui/bindings/preventdefault
*/

/**
* Returns a {module:ui/template~TemplateToBinding} resulting in a native `event#preventDefault`
* for the DOM event if `event#target` equals {@link module:ui/view~View#element}.
*
* @param {module:ui/view~View} view View instance that uses the template.
* @returns {module:ui/template~TemplateToBinding}
*/
export default function preventDefault( view ) {
return view.bindTemplate.to( evt => {
if ( evt.target === view.element ) {
evt.preventDefault();
}
} );
}
10 changes: 8 additions & 2 deletions src/focuscycler.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,13 +241,19 @@ export default class FocusCycler {
*/
_getFocusableItem( step ) {
// Cache for speed.
const current = this.current;
let current = this.current;
const collectionLength = this.focusables.length;

if ( !collectionLength || current === null ) {
if ( !collectionLength ) {
return null;
}

// Start from the beginning if no view is focused.
// https://github.com/ckeditor/ckeditor5-ui/issues/206
if ( current === null ) {
return this[ step === 1 ? 'first' : 'last' ];
}

// Cycle in both directions.
let index = ( current + collectionLength + step ) % collectionLength;

Expand Down
13 changes: 8 additions & 5 deletions src/panel/balloon/balloonpanelview.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { getOptimalPosition } from '@ckeditor/ckeditor5-utils/src/dom/position';
import isRange from '@ckeditor/ckeditor5-utils/src/dom/isrange';
import isElement from '@ckeditor/ckeditor5-utils/src/lib/lodash/isElement';
import toUnit from '@ckeditor/ckeditor5-utils/src/dom/tounit';
import preventDefault from '../../bindings/preventdefault.js';
import global from '@ckeditor/ckeditor5-utils/src/dom/global';

const toPx = toUnit( 'px' );
Expand Down Expand Up @@ -134,13 +135,15 @@ export default class BalloonPanelView extends View {
top: bind.to( 'top', toPx ),
left: bind.to( 'left', toPx ),
maxWidth: bind.to( 'maxWidth', toPx )
},

// Make this element `focusable` to be available for adding to FocusTracker.
tabindex: -1
}
},

children: this.content
children: this.content,

on: {
// https://github.com/ckeditor/ckeditor5-ui/issues/206
mousedown: preventDefault( this )
}
} );
}

Expand Down
8 changes: 7 additions & 1 deletion src/toolbar/toolbarview.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import FocusTracker from '@ckeditor/ckeditor5-utils/src/focustracker';
import FocusCycler from '../focuscycler';
import KeystrokeHandler from '@ckeditor/ckeditor5-utils/src/keystrokehandler';
import ToolbarSeparatorView from './toolbarseparatorview';
import preventDefault from '../bindings/preventdefault.js';

/**
* The toolbar view class.
Expand Down Expand Up @@ -78,7 +79,12 @@ export default class ToolbarView extends View {
]
},

children: this.items
children: this.items,

on: {
// https://github.com/ckeditor/ckeditor5-ui/issues/206
mousedown: preventDefault( this )
}
} );

this.items.on( 'add', ( evt, item ) => {
Expand Down
63 changes: 63 additions & 0 deletions tests/bindings/preventdefault.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* global Event */

import preventDefault from '../../src/bindings/preventdefault';
import View from '../../src/view';
import Template from '../../src/template';

describe( 'preventDefault', () => {
it( 'prevents default of a native DOM event', () => {
const view = new View();

view.template = new Template( {
tag: 'div',

on: {
foo: preventDefault( view )
}
} );

const evt = new Event( 'foo', { bubbles: true } );
const spy = sinon.spy( evt, 'preventDefault' );

// Render to enable bubbling.
view.element;

view.element.dispatchEvent( evt );
sinon.assert.calledOnce( spy );
} );

it( 'prevents only when target is view#element', () => {
const view = new View();
const child = new View();

child.template = new Template( {
tag: 'a'
} );

view.template = new Template( {
tag: 'div',

on: {
foo: preventDefault( view )
},

children: [
child
]
} );

const evt = new Event( 'foo', { bubbles: true } );
const spy = sinon.spy( evt, 'preventDefault' );

// Render to enable bubbling.
view.element;

child.element.dispatchEvent( evt );
sinon.assert.notCalled( spy );
} );
} );
8 changes: 4 additions & 4 deletions tests/focuscycler.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ describe( 'FocusCycler', () => {
expect( cycler.next ).to.equal( focusables.get( 2 ) );
} );

it( 'returns null when no view is focused', () => {
it( 'focuses the first focusable view when no view is focused', () => {
focusTracker.focusedElement = null;

expect( cycler.next ).to.be.null;
expect( cycler.next ).to.equal( focusables.get( 1 ) );
} );

it( 'returns null when no items', () => {
Expand Down Expand Up @@ -162,10 +162,10 @@ describe( 'FocusCycler', () => {
expect( cycler.previous ).to.equal( focusables.get( 2 ) );
} );

it( 'returns null when no view is focused', () => {
it( 'focuses the last focusable view when no view is focused', () => {
focusTracker.focusedElement = null;

expect( cycler.previous ).to.be.null;
expect( cycler.previous ).to.equal( focusables.get( 3 ) );
} );

it( 'returns null when no items', () => {
Expand Down
11 changes: 10 additions & 1 deletion tests/panel/balloon/balloonpanelview.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ describe( 'BalloonPanelView', () => {
it( 'should create element from template', () => {
expect( view.element.tagName ).to.equal( 'DIV' );
expect( view.element.classList.contains( 'ck-balloon-panel' ) ).to.true;
expect( view.element.getAttribute( 'tabindex' ) ).to.equal( '-1' );
} );

it( 'should set default values', () => {
Expand Down Expand Up @@ -129,6 +128,16 @@ describe( 'BalloonPanelView', () => {
} );
} );
} );

describe( 'event listeners', () => {
it( 'prevent default on #mousedown', () => {
const evt = new Event( 'mousedown', { bubbles: true } );
const spy = sinon.spy( evt, 'preventDefault' );

view.element.dispatchEvent( evt );
sinon.assert.calledOnce( spy );
} );
} );
} );

describe( 'show()', () => {
Expand Down
12 changes: 11 additions & 1 deletion tests/toolbar/toolbarview.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* For licensing, see LICENSE.md.
*/

/* global document */
/* global document, Event */

import ToolbarView from '../../src/toolbar/toolbarview';
import ToolbarSeparatorView from '../../src/toolbar/toolbarseparatorview';
Expand Down Expand Up @@ -64,6 +64,16 @@ describe( 'ToolbarView', () => {
it( 'should create element from template', () => {
expect( view.element.classList.contains( 'ck-toolbar' ) ).to.true;
} );

describe( 'event listeners', () => {
it( 'prevent default on #mousedown', () => {
const evt = new Event( 'mousedown', { bubbles: true } );
const spy = sinon.spy( evt, 'preventDefault' );

view.element.dispatchEvent( evt );
sinon.assert.calledOnce( spy );
} );
} );
} );

describe( 'init()', () => {
Expand Down

0 comments on commit f9e3bb7

Please sign in to comment.