jenkins-bot has submitted this change and it was merged.
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 enableFloatable, 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
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( , .. )"
Bug: 52014
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, 141 insertions(+), 121 deletions(-)
Approvals:
Esanders: Looks good to me, approved
jenkins-bot: Verified
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 bdb8c07..92d8c50 100644
--- a/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js
+++ b/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js
@@ -30,6 +30,8 @@
this.$document = null;
this.$spinner = $( '<div
class="ve-init-mw-viewPageTarget-loading"></div>' );
this.$toolbarTracker = $( '<div
class="ve-init-mw-viewPageTarget-toolbarTracker"></div>' );
+ this.toolbarTrackerFloating = null;
+ this.toolbarOffset = null;
this.toolbarCancelButton = null;
this.toolbarSaveButton = null;
this.saveDialogSlideHistory = [];
@@ -1169,56 +1171,47 @@
};
/**
- * The toolbar has updated its position.
+ * @see ve.ui.Surface#toolbarPosition
* @param {jQuery} $bar
+ * @param {Object} update
*/
-ve.init.mw.ViewPageTarget.prototype.onSurfaceToolbarPosition = function ( $bar
) {
- var css, offset, startProp, startOffset,
- dir = mw.config.get( 'wgVisualEditor' ).pageLanguageDir,
- type = $bar.css( 'position' );
-
- // 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' );
- }
-
- // 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
+ve.init.mw.ViewPageTarget.prototype.onSurfaceToolbarPosition = function (
$bar, update ) {
+ // It's important that the toolbar tracker always has 0 height,
otherwise 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() );
-
- }
-
- css[ startProp ] = startOffset;
-
- } else if ( type === 'absolute' || type === 'fixed' ) {
- css = {
- 'position': type,
- 'top': $bar.css( 'top' ),
- 'left': $bar.css( 'left' )
- };
- } else {
- return;
+ if ( !this.toolbarTrackerFloating && update.floating === true ) {
+ // When switching to floating, undo the 'top' position set
earlier
+ this.$toolbarTracker.css( 'top', '' );
}
- this.$toolbarTracker.css( css );
+
+ if ( update.offset ) {
+ this.toolbarOffset = update.offset;
+ }
+
+ if ( typeof update.floating === 'boolean' ) {
+ this.$toolbarTracker.toggleClass(
+ 've-init-mw-viewPageTarget-toolbarTracker-floating',
+ update.floating
+ );
+ this.toolbarTrackerFloating = update.floating;
+ }
+
+ // Switching to non-floating or offset update when already in
non-floating
+ if ( update.floating === false || this.toolbarTrackerFloating === 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': this.toolbarOffset.top,
+ 'left': this.toolbarOffset.left,
+ 'right': this.toolbarOffset.right
+ } );
+ } else if ( update.css ) {
+ this.$toolbarTracker.css( update.css );
+ }
};
/**
@@ -1899,7 +1892,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' )
@@ -1907,12 +1900,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..673cfb5 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: Do we need this? What does it 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..7112472 100644
--- a/modules/ve/ui/ve.ui.Surface.js
+++ b/modules/ve/ui/ve.ui.Surface.js
@@ -65,8 +65,15 @@
/* Events */
/**
+ * Whenever the toolbar $bar position is updated, the changes that took place.
+ *
* @event toolbarPosition
* @param {jQuery} $bar Toolbar bar
+ * @param {Object} update
+ * @param {boolean} [update.floating] Whether the toolbar is in floating mode
+ * @param {Object} [update.css] One or more css properties that changed
+ * @param {Object} [update.offset] Updated offset object (from
jQuery.fn.offset, though
+ * it also includes `offset.right`)
*/
/* Methods */
diff --git a/modules/ve/ui/ve.ui.Toolbar.js b/modules/ve/ui/ve.ui.Toolbar.js
index f122166..948e487 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,44 @@
/* Methods */
/**
+ * Sets up handles and preloads required information for the toolbar to work.
+ * This must be called immediately after it is attached to a visible document.
+ */
+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 +139,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 +169,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 +185,6 @@
/**
* Handle context changes on the surface.
*
- * @method
* @emits updateState
*/
ve.ui.Toolbar.prototype.onContextChange = function () {
@@ -173,8 +203,6 @@
/**
* Initialize all tools and groups.
- *
- * @method
*/
ve.ui.Toolbar.prototype.addTools = function ( tools ) {
var i, j, group, $group, tool;
@@ -208,11 +236,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 +247,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: merged
Gerrit-Change-Id: I9be855148962eee068a77fe83e98eb20bbdcfeec
Gerrit-PatchSet: 8
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Esanders <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Matmarex <[email protected]>
Gerrit-Reviewer: Robmoen <[email protected]>
Gerrit-Reviewer: Tim Starling <[email protected]>
Gerrit-Reviewer: Trevor Parscal <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
