From 0e3800b988ba529af58a188ea8b1b81747f93373 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 19 Jul 2017 16:34:19 +0200 Subject: [PATCH 1/7] Other: StickyToolbarView should support a configurable vertical offset from the top of the page. Closes #277. --- src/toolbar/sticky/stickytoolbarview.js | 54 ++++++++++++--- tests/toolbar/sticky/stickytoolbarview.js | 83 ++++++++++++++++++++++- 2 files changed, 124 insertions(+), 13 deletions(-) diff --git a/src/toolbar/sticky/stickytoolbarview.js b/src/toolbar/sticky/stickytoolbarview.js index 6fc91641..282c0e85 100644 --- a/src/toolbar/sticky/stickytoolbarview.js +++ b/src/toolbar/sticky/stickytoolbarview.js @@ -68,9 +68,25 @@ export default class StickyToolbarView extends ToolbarView { * @readonly * @observable * @default 50 - * @member {Number} #limiterOffset + * @member {Number} #limiterBottomOffset */ - this.set( 'limiterOffset', 50 ); + this.set( 'limiterBottomOffset', 50 ); + + /** + * The offset from the top edge of the web browser's viewport which makes the + * toolbar become sticky. The default value is `0`, which means the toolbar becomes + * sticky when it's upper edge touches the top of the page viewport. + * + * This attribute is useful when the web page has UI elements positioned to the top + * either using `position: fixed` or `position: sticky`, which would cover the + * sticky toolbar or vice–versa (depending on the `z-index` hierarchy). + * + * @readonly + * @observable + * @default 0 + * @member {Number} #viewportTopOffset + */ + this.set( 'viewportTopOffset', 0 ); /** * Controls the `margin-left` CSS style of the toolbar. @@ -93,6 +109,18 @@ export default class StickyToolbarView extends ToolbarView { */ this.set( '_isStickyToTheLimiter', false ); + /** + * Set `true` if the sticky toolbar uses the {@link #viewportTopOffset}, + * i.e. not {@link #_isStickyToTheLimiter} and the {@link #viewportTopOffset} + * is not `0`. + * + * @protected + * @readonly + * @observable + * @member {Boolean} #_hasViewportTopOffset + */ + this.set( '_hasViewportTopOffset', false ); + /** * The DOM bounding client rect of the {@link module:ui/view~View#element} of the toolbar. * @@ -121,10 +149,14 @@ export default class StickyToolbarView extends ToolbarView { } ), bottom: bind.to( '_isStickyToTheLimiter', _isStickyToTheLimiter => { - return _isStickyToTheLimiter ? toPx( this.limiterOffset ) : null; + return _isStickyToTheLimiter ? toPx( this.limiterBottomOffset ) : null; } ), - marginLeft: bind.to( '_marginLeft' ) + marginLeft: bind.to( '_marginLeft' ), + + top: bind.to( '_hasViewportTopOffset', _hasViewportTopOffset => { + return _hasViewportTopOffset ? toPx( this.viewportTopOffset ) : null; + } ) } } } ); @@ -191,23 +223,25 @@ export default class StickyToolbarView extends ToolbarView { // The toolbar must be active to become sticky. this.isSticky = this.isActive && - // The limiter's top edge must be beyond the upper edge of the visible viewport. - limiterRect.top < 0 && - // The model#limiterElement's height mustn't be smaller than the toolbar's height and model#limiterOffset. + // The limiter's top edge must be beyond the upper edge of the visible viewport (+the viewportTopOffset). + limiterRect.top < this.viewportTopOffset && + // The model#limiterElement's height mustn't be smaller than the toolbar's height and model#limiterBottomOffset. // There's no point in entering the sticky mode if the model#limiterElement is very, very small, because - // it would immediately set model#_isStickyToTheLimiter true and, given model#limiterOffset, the toolbar + // it would immediately set model#_isStickyToTheLimiter true and, given model#limiterBottomOffset, the toolbar // would be positioned before the model#limiterElement. - this._toolbarRect.height + this.limiterOffset < limiterRect.height; + this._toolbarRect.height + this.limiterBottomOffset < limiterRect.height; // Stick the toolbar to the top edge of the viewport simulating CSS position:sticky. // TODO: Possibly replaced by CSS in the future http://caniuse.com/#feat=css-sticky if ( this.isSticky ) { - this._isStickyToTheLimiter = limiterRect.bottom < toolbarRect.height + this.limiterOffset; + this._isStickyToTheLimiter = limiterRect.bottom < toolbarRect.height + this.limiterBottomOffset + this.viewportTopOffset; + this._hasViewportTopOffset = !this._isStickyToTheLimiter && !!this.viewportTopOffset; this._marginLeft = this._isStickyToTheLimiter ? null : toPx( -global.window.scrollX ); } // Detach the toolbar from the top edge of the viewport. else { this._isStickyToTheLimiter = false; + this._hasViewportTopOffset = false; this._marginLeft = null; } } diff --git a/tests/toolbar/sticky/stickytoolbarview.js b/tests/toolbar/sticky/stickytoolbarview.js index 449b2cf3..49877c66 100644 --- a/tests/toolbar/sticky/stickytoolbarview.js +++ b/tests/toolbar/sticky/stickytoolbarview.js @@ -47,9 +47,11 @@ describe( 'StickyToolbarView', () => { it( 'sets view attributes', () => { expect( view.isSticky ).to.be.false; expect( view.limiterElement ).to.be.null; - expect( view.limiterOffset ).to.equal( 50 ); + expect( view.limiterBottomOffset ).to.equal( 50 ); + expect( view.viewportTopOffset ).to.equal( 0 ); expect( view._isStickyToTheLimiter ).to.be.false; + expect( view._hasViewportTopOffset ).to.be.false; expect( view._marginLeft ).to.be.null; } ); @@ -83,6 +85,16 @@ describe( 'StickyToolbarView', () => { expect( element.classList.contains( 'ck-toolbar_sticky_bottom-limit' ) ).to.be.true; } ); + it( 'update the styles.top on view#_hasViewportTopOffset change', () => { + view.viewportTopOffset = 100; + + view._hasViewportTopOffset = false; + expect( element.style.top ).to.equal( '' ); + + view._hasViewportTopOffset = true; + expect( element.style.top ).to.equal( '100px' ); + } ); + it( 'update the styles.width on view#isSticky change', () => { testUtils.sinon.stub( view._elementPlaceholder, 'getBoundingClientRect' ).returns( { width: 100 } ); @@ -236,10 +248,10 @@ describe( 'StickyToolbarView', () => { expect( view.isSticky ).to.be.false; } ); - it( 'is false if view.limiterElement is smaller than the toolbar and view.limiterOffset (toolbar is active)', () => { + it( 'is false if view.limiterElement is smaller than the toolbar and view.limiterBottomOffset (toolbar is active)', () => { testUtils.sinon.stub( view.limiterElement, 'getBoundingClientRect' ).returns( { top: -10, height: 60 } ); view.isActive = true; - view.limiterOffset = 50; + view.limiterBottomOffset = 50; expect( view.isSticky ).to.be.false; @@ -307,6 +319,71 @@ describe( 'StickyToolbarView', () => { } ); } ); + describe( 'view._hasViewportTopOffset', () => { + it( 'is true if view._isStickyToTheLimiter is false and view.viewportTopOffset has been specified', () => { + view.viewportTopOffset = 100; + + testUtils.sinon.stub( view.limiterElement, 'getBoundingClientRect' ).returns( { + top: 90, + bottom: 190, + height: 100 + } ); + + testUtils.sinon.stub( view.element, 'getBoundingClientRect' ).returns( { + height: 20 + } ); + + view.isActive = true; + + view._checkIfShouldBeSticky(); + expect( view.isSticky ).to.be.true; + expect( view._isStickyToTheLimiter ).to.be.false; + expect( view._hasViewportTopOffset ).to.be.true; + } ); + + it( 'is false if view._isStickyToTheLimiter is true and view.viewportTopOffset has been specified', () => { + view.viewportTopOffset = 100; + + testUtils.sinon.stub( view.limiterElement, 'getBoundingClientRect' ).returns( { + top: 10, + bottom: 110, + height: 100 + } ); + + testUtils.sinon.stub( view.element, 'getBoundingClientRect' ).returns( { + height: 20 + } ); + + view.isActive = true; + + view._checkIfShouldBeSticky(); + expect( view.isSticky ).to.be.true; + expect( view._isStickyToTheLimiter ).to.be.true; + expect( view._hasViewportTopOffset ).to.be.false; + } ); + + it( 'is false if view._isStickyToTheLimiter is false and view.viewportTopOffset is 0', () => { + view.viewportTopOffset = 100; + + testUtils.sinon.stub( view.limiterElement, 'getBoundingClientRect' ).returns( { + top: 90, + bottom: 190, + height: 100 + } ); + + testUtils.sinon.stub( view.element, 'getBoundingClientRect' ).returns( { + height: 20 + } ); + + view.isActive = true; + + view._checkIfShouldBeSticky(); + expect( view.isSticky ).to.be.true; + expect( view._isStickyToTheLimiter ).to.be.false; + expect( view._hasViewportTopOffset ).to.be.true; + } ); + } ); + describe( 'view._marginLeft', () => { it( 'is set if view.isSticky is true view._isStickyToTheLimiter is false', () => { testUtils.sinon.stub( view.limiterElement, 'getBoundingClientRect' ).returns( { From 74fb01c09dae4a04ac6057aab7f40df6fc5d07cb Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 19 Jul 2017 16:38:11 +0200 Subject: [PATCH 2/7] Code style: Improvements in StickyToolbarView. --- src/toolbar/sticky/stickytoolbarview.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/toolbar/sticky/stickytoolbarview.js b/src/toolbar/sticky/stickytoolbarview.js index 282c0e85..84a440c9 100644 --- a/src/toolbar/sticky/stickytoolbarview.js +++ b/src/toolbar/sticky/stickytoolbarview.js @@ -148,15 +148,15 @@ export default class StickyToolbarView extends ToolbarView { return isSticky ? toPx( this._elementPlaceholder.getBoundingClientRect().width ) : null; } ), + top: bind.to( '_hasViewportTopOffset', _hasViewportTopOffset => { + return _hasViewportTopOffset ? toPx( this.viewportTopOffset ) : null; + } ), + bottom: bind.to( '_isStickyToTheLimiter', _isStickyToTheLimiter => { return _isStickyToTheLimiter ? toPx( this.limiterBottomOffset ) : null; } ), - marginLeft: bind.to( '_marginLeft' ), - - top: bind.to( '_hasViewportTopOffset', _hasViewportTopOffset => { - return _hasViewportTopOffset ? toPx( this.viewportTopOffset ) : null; - } ) + marginLeft: bind.to( '_marginLeft' ) } } } ); @@ -234,7 +234,8 @@ export default class StickyToolbarView extends ToolbarView { // Stick the toolbar to the top edge of the viewport simulating CSS position:sticky. // TODO: Possibly replaced by CSS in the future http://caniuse.com/#feat=css-sticky if ( this.isSticky ) { - this._isStickyToTheLimiter = limiterRect.bottom < toolbarRect.height + this.limiterBottomOffset + this.viewportTopOffset; + this._isStickyToTheLimiter = + limiterRect.bottom < toolbarRect.height + this.limiterBottomOffset + this.viewportTopOffset; this._hasViewportTopOffset = !this._isStickyToTheLimiter && !!this.viewportTopOffset; this._marginLeft = this._isStickyToTheLimiter ? null : toPx( -global.window.scrollX ); } From 2f4e0ad0db6ac43773a6f4c2dcc7ecd560f42f25 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 20 Jul 2017 12:30:14 +0200 Subject: [PATCH 3/7] Fix: StickyToolbarView does not become sticky on #init if the conditions are met. --- src/toolbar/sticky/stickytoolbarview.js | 3 +++ tests/toolbar/sticky/stickytoolbarview.js | 16 ++++++++++++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/toolbar/sticky/stickytoolbarview.js b/src/toolbar/sticky/stickytoolbarview.js index 84a440c9..42d85313 100644 --- a/src/toolbar/sticky/stickytoolbarview.js +++ b/src/toolbar/sticky/stickytoolbarview.js @@ -192,6 +192,9 @@ export default class StickyToolbarView extends ToolbarView { this.element.parentNode.insertBefore( this._elementPlaceholder, this.element ); + // Check if the toolbar should go into the sticky state immediately. + this._checkIfShouldBeSticky(); + // Update sticky state of the toolbar as the window is being scrolled. this.listenTo( global.window, 'scroll', () => { this._checkIfShouldBeSticky(); diff --git a/tests/toolbar/sticky/stickytoolbarview.js b/tests/toolbar/sticky/stickytoolbarview.js index 49877c66..18ee0eab 100644 --- a/tests/toolbar/sticky/stickytoolbarview.js +++ b/tests/toolbar/sticky/stickytoolbarview.js @@ -163,13 +163,21 @@ describe( 'StickyToolbarView', () => { expect( element.previousSibling ).to.equal( view._elementPlaceholder ); } ); + it( 'checks if the toolbar should be sticky', () => { + const spy = testUtils.sinon.spy( view, '_checkIfShouldBeSticky' ); + expect( spy.notCalled ).to.be.true; + + view.init(); + expect( spy.calledOnce ).to.be.true; + } ); + it( 'listens to window#scroll event and calls view._checkIfShouldBeSticky', () => { const spy = testUtils.sinon.spy( view, '_checkIfShouldBeSticky' ); + expect( spy.notCalled ).to.be.true; view.init(); global.window.fire( 'scroll' ); - - expect( spy.calledOnce ).to.be.true; + expect( spy.calledTwice ).to.be.true; } ); it( 'listens to view.isActive and calls view._checkIfShouldBeSticky', () => { @@ -178,10 +186,10 @@ describe( 'StickyToolbarView', () => { view.init(); view.isActive = true; - expect( spy.calledOnce ).to.be.true; + expect( spy.calledTwice ).to.be.true; view.isActive = false; - expect( spy.calledTwice ).to.be.true; + expect( spy.calledThrice ).to.be.true; } ); } ); From 2db39dd1305c043f70dd742aadf1934c80049ce3 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 20 Jul 2017 12:30:53 +0200 Subject: [PATCH 4/7] Tests: Extended StickyToolbarView manual test to consider the #viewportTopOffset attribute value. --- .../stickytoolbarview/stickytoolbarview.html | 85 ++++++++++++++----- .../stickytoolbarview/stickytoolbarview.js | 10 ++- .../stickytoolbarview/stickytoolbarview.md | 9 ++ 3 files changed, 83 insertions(+), 21 deletions(-) diff --git a/tests/manual/stickytoolbarview/stickytoolbarview.html b/tests/manual/stickytoolbarview/stickytoolbarview.html index a8810063..d2dff49d 100644 --- a/tests/manual/stickytoolbarview/stickytoolbarview.html +++ b/tests/manual/stickytoolbarview/stickytoolbarview.html @@ -1,31 +1,70 @@ -
-
+
+
+

Sticky to the top of the viewport

+ +
+
+

+ An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. +

+
+
-

- An editable content mock–up. - An editable content mock–up. - An editable content mock–up. - An editable content mock–up. - An editable content mock–up. - An editable content mock–up. - An editable content mock–up. - An editable content mock–up. -

-
+
+

Sticky to the green box

+ +
+
+

+ An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. + An editable content mock–up. +

+
+
+ +
The toolbar should stick to me instead of the viewport.
diff --git a/tests/manual/stickytoolbarview/stickytoolbarview.js b/tests/manual/stickytoolbarview/stickytoolbarview.js index 6655a691..202ae7d8 100644 --- a/tests/manual/stickytoolbarview/stickytoolbarview.js +++ b/tests/manual/stickytoolbarview/stickytoolbarview.js @@ -9,10 +9,14 @@ import StickyToolbarView from '../../../src/toolbar/sticky/stickytoolbarview'; import '@ckeditor/ckeditor5-theme-lark/theme/theme.scss'; const ui = testUtils.createTestUIView( { - top: '.ck-editor__top' + stickyToTheTop: '.ck-sticky_to-the-top .ck-editor__top', + stickyToTheBox: '.ck-sticky_to-the-box .ck-editor__top' } ); -createToolbar( ui.top ); +createToolbar( ui.stickyToTheTop ); +const stickyToTheBoxToolbar = createToolbar( ui.stickyToTheBox ); + +stickyToTheBoxToolbar.viewportTopOffset = 100; function createToolbar( collection ) { const toolbar = new StickyToolbarView(); @@ -21,4 +25,6 @@ function createToolbar( collection ) { collection.add( toolbar ); toolbar.isActive = true; + + return toolbar; } diff --git a/tests/manual/stickytoolbarview/stickytoolbarview.md b/tests/manual/stickytoolbarview/stickytoolbarview.md index 5b5a8f55..d0572edd 100644 --- a/tests/manual/stickytoolbarview/stickytoolbarview.md +++ b/tests/manual/stickytoolbarview/stickytoolbarview.md @@ -1,10 +1,19 @@ ## Vertical scrolling +### Sticky to the top of the viewport + 1. When the page is scrolled vertically, the toolbar should 1. stick to the top of the viewport first, 1. then disappear beyond the upper edge of the viewport as it touches the red area 1. but never cover the red area or go beyond the upper edge of editor mock–up. +### Sticky to the green box + +1. When the page is scrolled vertically, the toolbar should + 1. stick to the bottom of the green box first, + 1. then disappear beyond the bottom edge of the green box as it touches the red area + 1. but never cover the red area or go beyond the upper edge of editor mock–up. + ## Horizontal scrolling 1. The toolbar should always fit horizontally within the editor mock–up, regardless of the position of the h– and v–scrolls of the web page. From 4f96bbcdf3f76d83048c39a593c53a001d9b8228 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 20 Jul 2017 12:37:20 +0200 Subject: [PATCH 5/7] Implemented normalizeToolbarConfig() utility. --- src/toolbar/normalizetoolbarconfig.js | 35 +++++++++++++++++++++++++ tests/toolbar/normalizetoolbarconfig.js | 28 ++++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 src/toolbar/normalizetoolbarconfig.js create mode 100644 tests/toolbar/normalizetoolbarconfig.js diff --git a/src/toolbar/normalizetoolbarconfig.js b/src/toolbar/normalizetoolbarconfig.js new file mode 100644 index 00000000..f331f277 --- /dev/null +++ b/src/toolbar/normalizetoolbarconfig.js @@ -0,0 +1,35 @@ +/** + * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/** + * @module ui/toolbar/normalizetoolbarconfig + */ + +/** + * Normalizes the toolbar configuration (`config.toolbar`), which may be defined as an `Array` + * + * toolbar: [ 'headings', 'bold', 'italic', 'link', 'unlink', ... ] + * + * or an `Object` + * + * toolbar: { + * items: [ 'headings', 'bold', 'italic', 'link', 'unlink', ... ], + * ... + * } + * + * and returns it in the object form. + * + * @param {Array|Object} config The value of `config.toolbar`. + * @returns {Object} A normalized toolbar config object. + */ +export default function normalizeToolbarConfig( config ) { + if ( config instanceof Array ) { + config = { + items: config + }; + } + + return config; +} diff --git a/tests/toolbar/normalizetoolbarconfig.js b/tests/toolbar/normalizetoolbarconfig.js new file mode 100644 index 00000000..b7e53daf --- /dev/null +++ b/tests/toolbar/normalizetoolbarconfig.js @@ -0,0 +1,28 @@ +/** + * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +import normalizeToolbarConfig from '../../src/toolbar/normalizeToolbarConfig'; + +describe( 'normalizeToolbarConfig()', () => { + it( 'normalizes the config specified as an Array', () => { + const cfg = [ 'foo', 'bar' ]; + const normalized = normalizeToolbarConfig( cfg ); + + expect( normalized ).to.be.an( 'object' ); + expect( normalized.items ).to.equal( cfg ); + } ); + + it( 'passes through an already normalized config', () => { + const cfg = { + items: [ 'foo', 'bar' ], + foo: 'bar' + }; + const normalized = normalizeToolbarConfig( cfg ); + + expect( normalized ).to.equal( cfg ); + expect( normalized.items ).to.equal( cfg.items ); + expect( normalized.foo ).to.equal( cfg.foo ); + } ); +} ); From 04a726e7ff9435c3fb222222117fa02e4fecc91f Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 20 Jul 2017 13:35:40 +0200 Subject: [PATCH 6/7] Tests: Fixed broken dependency path. --- tests/toolbar/normalizetoolbarconfig.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/toolbar/normalizetoolbarconfig.js b/tests/toolbar/normalizetoolbarconfig.js index b7e53daf..dbd36f22 100644 --- a/tests/toolbar/normalizetoolbarconfig.js +++ b/tests/toolbar/normalizetoolbarconfig.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -import normalizeToolbarConfig from '../../src/toolbar/normalizeToolbarConfig'; +import normalizeToolbarConfig from '../../src/toolbar/normalizetoolbarconfig'; describe( 'normalizeToolbarConfig()', () => { it( 'normalizes the config specified as an Array', () => { From b3e608831c3192b00db773229796502719cfe348 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 24 Jul 2017 09:47:19 +0200 Subject: [PATCH 7/7] Code refactoring: Used Array.isArray instead of instanceof. --- src/toolbar/normalizetoolbarconfig.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/toolbar/normalizetoolbarconfig.js b/src/toolbar/normalizetoolbarconfig.js index f331f277..57f0da05 100644 --- a/src/toolbar/normalizetoolbarconfig.js +++ b/src/toolbar/normalizetoolbarconfig.js @@ -25,7 +25,7 @@ * @returns {Object} A normalized toolbar config object. */ export default function normalizeToolbarConfig( config ) { - if ( config instanceof Array ) { + if ( Array.isArray( config ) ) { config = { items: config };