Krinkle has uploaded a new change for review.
https://gerrit.wikimedia.org/r/76241
Change subject: ve.ui.Toolbar: Refactor floating logic for performance
......................................................................
ve.ui.Toolbar: Refactor floating logic for performance
== Renamed methods ==
* enableFloating -> enableFloatable
* disableFloating -> disableFloatable
* setPosition -> float
* resetPosition -> unfloat
== Scroll and resize event ==
Timeline for scroll event reduced from about half a dozen
"Recalculate style" and various forced "Paint" down to 0.
New timeline for scroll is clean (for me: from ~35 to ~59 fps):
* 1 Event (scroll)
* 1 Composite Layer
The composite layer action is the browser changing the viewport
to a different portion of the document drawing. Exactly the one
thing a simple scroll should do.
Timeline for resize event is still pretty crowded and low fps,
but it has improved. Further improvement would likely be around
using requestAnimation and going outside ve.ui.Toolbar.
== Changes ==
* New: ve.ui.Toolbar#initialize.
Similar to what surface has. Users of Toolbar should decide
whether to call enabeFloatable, append it to the DOM at some
point and then call initialize() once.
* Don't compute offset() every time.
Eliminated by doing it once in #initialize. These 'top' and
'left' offsets do not change.
* Don't compute outerWidth() and $window.width() every time.
Reduced by doing it once in #initialize to compute the 'right'
offset. Updating it only on resize.
* Don't set 'top' every time.
This is already in the stylesheet. It was never set to anything
else so the abstraction for it in #float has been removed.
This also made it obvious that code for "ve-ui-toolbar-bottom"
was unused and left behind. Tha class was only ever being
removed from something (never added).
The one addClass call for it was in a condition that is always
false ("if top > 0").
* Don't set 'left' every time.
Eliminated by doing it once in #float.
* Don't set 'right' every time.
Reduced by no longer doing it on scroll. Done once in #float,
and on resize after computing the new value for it.
* Remove no-op style operations.
Wrapped methods in if-floatable, if-floated etc. to reduce a
fair amount of easily avoided re-paint overhead.
* Avoid double re-paint in mw.ViewPageTarget.
Though we prevent a lot of redundant re-paints now, whenever
we do repaint we want to do it in 1 repaint instead of 2.
ve.ui.Toolbar emits #toolbarPosition, which tells
mw.ViewPageTarget to update toolbarTracker which would read
the new $bar style properties and copy them over to the
$toolbarTracker. However this read operation forces the browser
to do an immediate re-paint half-way just for $bar.
Browsers only repaint when style properties are changed and
JS has yielded. The exception to this is JS reading style
style properties, in that case the browser is forced to do
those deferred repaints directly and reflect the new values.
We can avoid this double repaint by passing the updated values
as data instead of requiring the receiver to read the DOM (and
thus a keep the deferred repaint). Now toolbarTracker can use
them directly(whilst the browser hasn't even repainted $bar yet.
== Clean up ==
* Redundant "border-radius: 0". This would reset something, but
it never does. None of the things it inherits from set a
border-radius. There is one subclass where toolbar is used
with a border-radius (".ve-ui-surfaceWidget .ve-ui-toolbar-bar"
sets a border-radius) which overrides this on purpose, so the
default of 0 is redundant.
* Pattern "if ( .. ) addClass() else removeClass()" changed to:
"toggleClass( , .. )"
Change-Id: I9be855148962eee068a77fe83e98eb20bbdcfeec
---
M modules/ve-mw/init/styles/ve.init.mw.ViewPageTarget.css
M modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js
M modules/ve/init/sa/ve.init.sa.Target.js
M modules/ve/ui/styles/ve.ui.Toolbar.css
M modules/ve/ui/ve.ui.Context.js
M modules/ve/ui/ve.ui.Surface.js
M modules/ve/ui/ve.ui.Toolbar.js
M modules/ve/ui/widgets/ve.ui.SurfaceWidget.js
M modules/ve/ve.Element.js
9 files changed, 139 insertions(+), 121 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/41/76241/1
diff --git a/modules/ve-mw/init/styles/ve.init.mw.ViewPageTarget.css
b/modules/ve-mw/init/styles/ve.init.mw.ViewPageTarget.css
index a260d25..8b0c317 100644
--- a/modules/ve-mw/init/styles/ve.init.mw.ViewPageTarget.css
+++ b/modules/ve-mw/init/styles/ve.init.mw.ViewPageTarget.css
@@ -110,12 +110,12 @@
.ve-init-mw-viewPageTarget-toolbarTracker {
position: absolute;
top: 0;
- right: 0;
height: 0;
overflow: visible;
}
.ve-init-mw-viewPageTarget-toolbarTracker-floating {
+ position: fixed;
z-index: 100;
}
diff --git a/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js
b/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js
index 29a4b97..44ad1c4 100644
--- a/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js
+++ b/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js
@@ -1162,55 +1162,51 @@
/**
* The toolbar has updated its position.
* @param {jQuery} $bar
+ * @param {Object} update
+ * @param {Object} [offset]
*/
-ve.init.mw.ViewPageTarget.prototype.onSurfaceToolbarPosition = function ( $bar
) {
- var css, offset, startProp, startOffset,
- dir = mw.config.get( 'wgVisualEditor' ).pageLanguageDir,
- type = $bar.css( 'position' );
+ve.init.mw.ViewPageTarget.prototype.onSurfaceToolbarPosition = ( function () {
+ var offset, floating;
- // HACK: If the toolbar is floating, also apply a floating class to the
toolbar tracker
- if ( $bar.parent().hasClass( 've-ui-toolbar-floating' ) ) {
- this.$toolbarTracker.addClass(
've-init-mw-viewPageTarget-toolbarTracker-floating' );
- } else {
- this.$toolbarTracker.removeClass(
've-init-mw-viewPageTarget-toolbarTracker-floating' );
- }
+ return function ( $bar, update ) {
+ // It's important that the toolbar tracker always has 0 height.
Else it will block events on
+ // the toolbar (e.g. clicking "Save page") as it would overlap
that space. The save dialog
+ // will remain visible for the same reason elsewhere: As long
as we don't have overflow:hidden,
+ // the save dialog will stick out of the tracker in the right
place without the tracker itself
+ // blocking the toolbar.
- // It's important that the toolbar tracker has 0 height. Else it will
block events on the
- // toolbar (e.g. clicking "Save page") as it would overlap that space.
The save dialog
- // will remain visible for the same reason elsewhere: As long as we
don't have overflow:hidden,
- // the save dialog will stick out of the tracker in the right place
without the tracker itself
- // blocking the toolbar.
-
- if ( type === 'relative' ) {
- offset = $bar.offset();
-
- css = {
- 'position': 'absolute',
- 'top': offset.top
- };
-
- if ( dir === 'ltr' ) {
- startProp = 'left';
- startOffset = offset.left;
- } else {
- startProp = 'right';
- startOffset = $( window ).width() - ( offset.left +
$bar.outerWidth() );
-
+ if ( !floating && update.floating === true ) {
+ // When switching to floating, undo the 'top' position
set earlier
+ this.$toolbarTracker.css( 'top', '' );
}
- css[ startProp ] = startOffset;
+ if ( update.offset ) {
+ offset = update.offset;
+ }
- } else if ( type === 'absolute' || type === 'fixed' ) {
- css = {
- 'position': type,
- 'top': $bar.css( 'top' ),
- 'left': $bar.css( 'left' )
- };
- } else {
- return;
- }
- this.$toolbarTracker.css( css );
-};
+ if ( typeof update.floating === 'boolean' ) {
+ floating = update.floating;
+ this.$toolbarTracker.toggleClass(
+
've-init-mw-viewPageTarget-toolbarTracker-floating',
+ floating
+ );
+ }
+
+ // Switching to non-floating or offset update when already in
non-floating
+ if ( update.floating === false || floating === false &&
update.offset ) {
+ // Don't use update.css in this case since the toolbar
is now in its non-floating
+ // position (static, in-flow). So make the tracker
absolutely postioned matching the
+ // offset of the toolbar.
+ this.$toolbarTracker.css( {
+ 'top': offset.top,
+ 'left': offset.left,
+ 'right': offset.right
+ } );
+ } else if ( update.css ) {
+ this.$toolbarTracker.css( update.css );
+ }
+ };
+} () );
/**
* Set up the logging of analytic edit events using EventLogging.
@@ -1890,7 +1886,7 @@
this.toolbar.addTools( this.constructor.static.toolbarTools );
this.surface.addCommands( this.constructor.static.surfaceCommands );
if ( !this.isMobileDevice ) {
- this.toolbar.enableFloating();
+ this.toolbar.enableFloatable();
}
this.toolbar.$
.addClass( 've-init-mw-viewPageTarget-toolbar' )
@@ -1898,12 +1894,8 @@
this.toolbar.$bar.slideDown( 'fast', ve.bind( function () {
// Check the surface wasn't torn down while the toolbar was
animating
if ( this.surface ) {
+ this.toolbar.initialize();
this.surface.getContext().update();
-
- // Emit event for initial position. Must be done here
after the
- // slide down instead of in ve.ui.Toolbar#constructor
because
- // back there it'll still be out of view.
- this.surface.emit( 'toolbarPosition', this.toolbar.$bar
);
}
}, this ) );
};
diff --git a/modules/ve/init/sa/ve.init.sa.Target.js
b/modules/ve/init/sa/ve.init.sa.Target.js
index d0870cd..e259f31 100644
--- a/modules/ve/init/sa/ve.init.sa.Target.js
+++ b/modules/ve/init/sa/ve.init.sa.Target.js
@@ -31,8 +31,11 @@
// Initialization
this.toolbar.$.addClass( 've-init-sa-target-toolbar' );
this.toolbar.addTools( this.constructor.static.toolbarTools );
- this.toolbar.enableFloating();
+ this.toolbar.enableFloatable();
+
this.$.append( this.toolbar.$, this.surface.$ );
+
+ this.toolbar.initialize();
this.surface.addCommands( this.constructor.static.surfaceCommands );
this.surface.initialize();
};
diff --git a/modules/ve/ui/styles/ve.ui.Toolbar.css
b/modules/ve/ui/styles/ve.ui.Toolbar.css
index 0a519ef..082d0fc 100644
--- a/modules/ve/ui/styles/ve.ui.Toolbar.css
+++ b/modules/ve/ui/styles/ve.ui.Toolbar.css
@@ -9,13 +9,8 @@
clear: both;
}
-.ve-ui-toolbar-bottom {
- position: static;
-}
-
.ve-ui-toolbar-bar {
border-bottom: solid 1px #ccc;
- position: relative;
background-color: white;
/* @embed */
background-image: url(images/fade-up.png);
@@ -26,11 +21,10 @@
}
.ve-ui-toolbar-floating .ve-ui-toolbar-bar {
- top: 0;
position: fixed;
- border-radius: 0;
+ top: 0;
z-index: 100;
- border-top: none;
+ border-top: none; /* TODO: What does this reset? */;
}
.ve-ui-toolbar-bottom .ve-ui-toolbar-bar {
diff --git a/modules/ve/ui/ve.ui.Context.js b/modules/ve/ui/ve.ui.Context.js
index 007fea8..1d15d3e 100644
--- a/modules/ve/ui/ve.ui.Context.js
+++ b/modules/ve/ui/ve.ui.Context.js
@@ -225,6 +225,7 @@
this.toolbar.addTools( [{ 'name': 'inspectors', 'items'
: tools }] );
this.$menu.append( this.toolbar.$ );
this.show();
+ this.toolbar.initialize();
} else if ( this.visible ) {
// Nothing to inspect
this.hide();
diff --git a/modules/ve/ui/ve.ui.Surface.js b/modules/ve/ui/ve.ui.Surface.js
index 200dd5e..9190dd9 100644
--- a/modules/ve/ui/ve.ui.Surface.js
+++ b/modules/ve/ui/ve.ui.Surface.js
@@ -67,6 +67,9 @@
/**
* @event toolbarPosition
* @param {jQuery} $bar Toolbar bar
+ * @param {Object} update
+ * @param {boolean} [update.floating]
+ * @param {Object} [update.css]
*/
/* Methods */
diff --git a/modules/ve/ui/ve.ui.Toolbar.js b/modules/ve/ui/ve.ui.Toolbar.js
index f122166..effd7a7 100644
--- a/modules/ve/ui/ve.ui.Toolbar.js
+++ b/modules/ve/ui/ve.ui.Toolbar.js
@@ -34,7 +34,11 @@
this.$tools = this.$$( '<div>' );
this.$actions = this.$$( '<div>' );
this.floating = false;
+ this.floatable = false;
+ this.initialized = false;
this.$window = null;
+ this.$surfaceView = null;
+ this.elementOffset = null;
this.windowEvents = {
'resize': ve.bind( this.onWindowResize, this ),
'scroll': ve.bind( this.onWindowScroll, this )
@@ -84,22 +88,43 @@
/* Methods */
/**
+ * @method
+ */
+ve.ui.Toolbar.prototype.initialize = function () {
+ this.initialized = true;
+ this.$window = $( this.getElementWindow() );
+ this.$surfaceView = this.surface.getView().$;
+ this.elementOffset = this.$.offset();
+ this.elementOffset.right = this.$window.width() - this.$.outerWidth() -
this.elementOffset.left;
+
+ // Initial position. Could be invalidated by the first
+ // call to onWindowScroll, but users of this event (e.g toolbarTracking)
+ // need to also now the non-floating position.
+ this.surface.emit( 'toolbarPosition', this.$bar, {
+ 'floating': false,
+ 'offset': this.elementOffset
+ } );
+
+ if ( this.floatable ) {
+ this.$window.on( this.windowEvents );
+ this.$surfaceView.on( this.surfaceViewEvents );
+ // The page may start with a non-zero scroll position
+ this.onWindowScroll();
+ }
+};
+
+/**
* Handle window resize events while toolbar floating is enabled.
*
* @returns {jQuery.Event} e Window resize event
*/
ve.ui.Toolbar.prototype.onWindowScroll = function () {
- var scrollTop = this.$window.scrollTop(),
- toolbarOffset = this.$.offset();
+ var scrollTop = this.$window.scrollTop();
- if ( scrollTop > toolbarOffset.top ) {
- this.setPosition(
- 0,
- toolbarOffset.left,
- this.$window.width() - this.$.outerWidth() -
toolbarOffset.left
- );
+ if ( scrollTop > this.elementOffset.top ) {
+ this.float();
} else if ( this.floating ) {
- this.resetPosition();
+ this.unfloat();
}
};
@@ -113,21 +138,27 @@
* @returns {jQuery.Event} e Window scroll event
*/
ve.ui.Toolbar.prototype.onWindowResize = function () {
- var offset = this.$.offset();
+ var update = {},
+ offset = this.elementOffset;
+
+ // Update right offset after resize (see #float)
+ offset.right = this.$window.width() - this.$.outerWidth() - offset.left;
+ update.offset = offset;
if ( this.floating ) {
- this.$bar.css( {
- 'left': offset.left,
- 'right': this.$window.width() - this.$.outerWidth() -
offset.left
- } );
- this.surface.emit( 'toolbarPosition', this.$bar );
+ update.css = { 'right': offset.right };
+ this.$bar.css( update.css );
}
+
+ // If we're not floating, toolbar position didn't change.
+ // But the dimensions did naturally change on resize, as did the right
offset.
+ // Which e.g. mw.ViewPageTarget's toolbarTracker needs.
+ this.surface.emit( 'toolbarPosition', this.$bar, update );
};
/**
* Method to scroll to the cursor position while toolbar is floating on keyup
only if
* the cursor is obscured by the toolbar.
- *
*/
ve.ui.Toolbar.prototype.onSurfaceViewKeyUp = function () {
var cursorPos = this.surface.view.getSelectionRect(),
@@ -137,14 +168,13 @@
// If toolbar is floating and cursor is obscured, scroll cursor into
view
if ( obscured && this.floating ) {
- $( 'html,body' ).animate( { scrollTop: scrollTo }, 0 );
+ $( 'html, body' ).animate( { scrollTop: scrollTo }, 0 );
}
};
/**
* Gets the surface the toolbar controls.
*
- * @method
* @returns {ve.ui.Surface} Surface being controlled
*/
ve.ui.Toolbar.prototype.getSurface = function () {
@@ -154,7 +184,6 @@
/**
* Handle context changes on the surface.
*
- * @method
* @emits updateState
*/
ve.ui.Toolbar.prototype.onContextChange = function () {
@@ -173,8 +202,6 @@
/**
* Initialize all tools and groups.
- *
- * @method
*/
ve.ui.Toolbar.prototype.addTools = function ( tools ) {
var i, j, group, $group, tool;
@@ -208,11 +235,9 @@
* Destroys toolbar, removing event handlers and DOM elements.
*
* Call this whenever you are done using a toolbar.
- *
- * @method
*/
ve.ui.Toolbar.prototype.destroy = function () {
- this.disableFloating();
+ this.disableFloatable();
this.surface.getModel().disconnect( this, { 'contextChange':
'onContextChange' } );
this.$.remove();
};
@@ -221,75 +246,74 @@
* Float the toolbar.
*
* @see ve.ui.Surface#event-toolbarPosition
- * @param {number} top Top position, in pixels
- * @param {number} left Left position, in pixels
- * @param {number} right Right position, in pixels
*/
-ve.ui.Toolbar.prototype.setPosition = function ( top, left, right ) {
- // When switching from default position, manually set the height of the
wrapper
+ve.ui.Toolbar.prototype.float = function () {
+ var update;
if ( !this.floating ) {
+ // When switching into floating mode, set the height of the
wrapper and
+ // move the bar to the same offset as the in-flow element
+ update = {
+ 'css': { 'left': this.elementOffset.left, 'right':
this.elementOffset.right },
+ 'floating': true
+ };
this.$
.css( 'height', this.$.height() )
.addClass( 've-ui-toolbar-floating' );
+ this.$bar.css( update.css );
this.floating = true;
+
+ this.surface.emit( 'toolbarPosition', this.$bar, update );
}
- this.$bar.css( { 'top': top, 'left': left, 'right': right } );
- if ( top > 0 ) {
- this.$.addClass( 've-ui-toolbar-bottom' );
- } else {
- this.$.removeClass( 've-ui-toolbar-bottom' );
- }
- this.surface.emit( 'toolbarPosition', this.$bar );
};
/**
- * Reset the toolbar to it's default position.
+ * Reset the toolbar to it's default non-floating position.
*
* @see ve.ui.Surface#event-toolbarPosition
*/
-ve.ui.Toolbar.prototype.resetPosition = function () {
- this.$
- .css( 'height', 'auto' )
- .removeClass( 've-ui-toolbar-floating ve-ui-toolbar-bottom' );
- this.$bar.css( { 'top': 0, 'left': 0, 'right': 0 } );
- this.floating = false;
- this.surface.emit( 'toolbarPosition', this.$bar );
+ve.ui.Toolbar.prototype.unfloat = function () {
+ if ( this.floating ) {
+ this.$
+ .css( 'height', '' )
+ .removeClass( 've-ui-toolbar-floating' );
+ this.$bar.css( { 'left': '', 'right': '' } );
+ this.floating = false;
+
+ this.surface.emit( 'toolbarPosition', this.$bar, { 'floating':
false } );
+ }
};
/**
- * Add automatic floating behavior to the toolbar.
+ * Set automatic floating behavior to the toolbar.
*
* Toolbar floating is not enabled by default, call this on setup to enable it.
- *
- * @method
+ * This will not make it float, but it will start listening for events that
+ * will result in it potentially being floated and defloated accordingly.
*/
-ve.ui.Toolbar.prototype.enableFloating = function () {
- this.$window = $( this.getElementWindow() ).on( this.windowEvents );
- this.$surfaceView = this.surface.getView().$.on( this.surfaceViewEvents
);
+ve.ui.Toolbar.prototype.enableFloatable = function () {
+ this.floatable = true;
- // TODO: Place this is a DOM attach event for this.$
- setTimeout( ve.bind( function () {
- // The page may load with a non-zero scroll without trigger the
scroll event
- this.onWindowScroll();
- }, this ), 0 );
+ if ( this.initialized ) {
+ this.$window.on( this.windowEvents );
+ this.$surfaceView.on( this.surfaceViewEvents );
+ }
};
/**
* Remove automatic floating behavior to the toolbar.
- *
- * @method
*/
-ve.ui.Toolbar.prototype.disableFloating = function () {
+ve.ui.Toolbar.prototype.disableFloatable = function () {
if ( this.$window ) {
this.$window.off( this.windowEvents );
- this.$window = null;
}
+
if ( this.$surfaceView ) {
this.$surfaceView.off( this.surfaceViewEvents );
- this.$surfaceView = null;
}
if ( this.floating ) {
- this.resetPosition();
+ this.unfloat();
}
+
+ this.floatable = false;
};
diff --git a/modules/ve/ui/widgets/ve.ui.SurfaceWidget.js
b/modules/ve/ui/widgets/ve.ui.SurfaceWidget.js
index 2f5ac6d..cc9177f 100644
--- a/modules/ve/ui/widgets/ve.ui.SurfaceWidget.js
+++ b/modules/ve/ui/widgets/ve.ui.SurfaceWidget.js
@@ -85,6 +85,7 @@
* @method
*/
ve.ui.SurfaceWidget.prototype.initialize = function () {
+ this.toolbar.initialize();
this.surface.initialize();
this.surface.view.documentView.documentNode.$.focus();
};
diff --git a/modules/ve/ve.Element.js b/modules/ve/ve.Element.js
index d9238d4..e8dd29d 100644
--- a/modules/ve/ve.Element.js
+++ b/modules/ve/ve.Element.js
@@ -156,7 +156,7 @@
/**
* Get the offset between two elements.
*
- * @param {jQuery} $from
+ * @param {jQuery} $from
* @param {jQuery} $to
* @returns {Object} Translated position coordinates, containing top and left
properties
*/
--
To view, visit https://gerrit.wikimedia.org/r/76241
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9be855148962eee068a77fe83e98eb20bbdcfeec
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
