Do not assume that revIds increase in time

This patch fixes issues with the RevisonSlider assuming that the higher
revision id belongs to the newer revision. Min/Max methods to decide what
the diff and what the oldid is are removed and the usage of methods is
adjusted accordingly.

Also the test for switchover pointers is removed since this will not work
with the patch.

Bug: T164455
Change-Id: If5d9cbb8ebd872aee376d249942e6881c8edb984

modules/ext.RevisionSlider.DiffPage.js

@@ -12,16 +12,14 @@
 		/**
 		 * Refreshes the diff view with two given revision IDs
 		 *
-		 * @param {number} revId1
-		 * @param {number} revId2
+		 * @param {number} diff
+		 * @param {number} oldid
 		 * @param {SliderView} sliderView
 		 * @param {number} [retryAttempt=0]
 		 */
-		refresh: function ( revId1, revId2, sliderView, retryAttempt ) {
+		refresh: function ( diff, oldid, sliderView, retryAttempt ) {
 			var self = this,
 				retryLimit = 2,
-				diff = Math.max( revId1, revId2 ),
-				oldid = Math.min( revId1, revId2 ),
 				data = {
 					diff: diff,
 					oldid: oldid
@@ -86,7 +84,7 @@
 					this.tryCount++;
 					mw.track( 'counter.MediaWiki.RevisionSlider.error.refresh' );
 					if ( retryAttempt <= retryLimit ) {
-						self.refresh( revId1, revId2, sliderView, retryAttempt + 1 );
+						self.refresh( diff, oldid, sliderView, retryAttempt + 1 );
 					}
 					// TODO notify the user that we failed to update the diff?
 					// This could also attempt to reload the page with the correct diff loaded without ajax?
@@ -97,17 +95,17 @@
 		/**
 		 * Replaces the current state in the history stack
 		 *
-		 * @param {number} revId1
-		 * @param {number} revId2
+		 * @param {number} diff
+		 * @param {number} oldid
 		 * @param {SliderView} sliderView
 		 */
-		replaceState: function ( revId1, revId2, sliderView ) {
+		replaceState: function ( diff, oldid, sliderView ) {
 			// IE9 does not have history.replaceState()
 			if ( typeof history.replaceState === 'function' ) {
 				history.replaceState(
-					this.getStateObject( revId1, revId2, sliderView ),
+					this.getStateObject( diff, oldid, sliderView ),
 					$( document ).find( 'title' ).text(),
-					this.getStateUrl( revId1, revId2 )
+					this.getStateUrl( diff, oldid )
 				);
 			}
 		},
@@ -115,17 +113,17 @@
 		/**
 		 * Pushes the current state onto the history stack
 		 *
-		 * @param {number} revId1
-		 * @param {number} revId2
+		 * @param {number} diff
+		 * @param {number} oldid
 		 * @param {SliderView} sliderView
 		 */
-		pushState: function ( revId1, revId2, sliderView ) {
+		pushState: function ( diff, oldid, sliderView ) {
 			// IE9 does not have history.pushState()
 			if ( typeof history.pushState === 'function' ) {
 				history.pushState(
-					this.getStateObject( revId1, revId2, sliderView ),
+					this.getStateObject( diff, oldid, sliderView ),
 					$( document ).find( 'title' ).text(),
-					this.getStateUrl( revId1, revId2 )
+					this.getStateUrl( diff, oldid )
 				);
 			}
 		},
@@ -133,15 +131,15 @@
 		/**
 		 * Gets a state object to be used with history.replaceState and history.pushState
 		 *
-		 * @param {number} revId1
-		 * @param {number} revId2
+		 * @param {number} diff
+		 * @param {number} oldid
 		 * @param {SliderView} sliderView
 		 * @return {Object}
 		 */
-		getStateObject: function ( revId1, revId2, sliderView ) {
+		getStateObject: function ( diff, oldid, sliderView ) {
 			return {
-				revid1: revId1,
-				revid2: revId2,
+				diff: diff,
+				oldid: oldid,
 				pointerOlderPos: sliderView.pointerOlder.getPosition(),
 				pointerNewerPos: sliderView.pointerNewer.getPosition(),
 				sliderPos: sliderView.slider.getOldestVisibleRevisionIndex()
@@ -151,12 +149,12 @@
 		/**
 		 * Gets a URL to be used with history.replaceState and history.pushState
 		 *
-		 * @param {number} revId1
-		 * @param {number} revId2
+		 * @param {number} diff
+		 * @param {number} oldid
 		 * @return {string}
 		 */
-		getStateUrl: function ( revId1, revId2 ) {
-			var url = mw.util.wikiScript( 'index' ) + '?diff=' + Math.max( revId1, revId2 ) + '&oldid=' + Math.min( revId1, revId2 ),
+		getStateUrl: function ( diff, oldid ) {
+			var url = mw.util.wikiScript( 'index' ) + '?diff=' + diff + '&oldid=' + oldid,
 				params = this.getExtraDiffPageParams();
 			if ( Object.keys( params ).length > 0 ) {
 				Object.keys( params ).forEach( function ( key ) {
@@ -203,7 +201,7 @@
 					sliderView.$element.find( 'div.mw-revslider-revisions' )
 				);
 				sliderView.updatePointerPositionAttributes();
-				self.refresh( event.state.revid1, event.state.revid2 );
+				self.refresh( event.state.diff, event.state.oldid );
 			} );
 		},
 

modules/ext.RevisionSlider.SliderView.js

@@ -132,7 +132,7 @@
 
 			this.slide( Math.floor( ( this.pointerNewer.getPosition() - 1 ) / this.slider.getRevisionsPerWindow() ), 0 );
 			this.diffPage.addHandlersToCoreLinks( this );
-			this.diffPage.replaceState( mw.config.get( 'extRevisionSliderOldRev' ), mw.config.get( 'extRevisionSliderNewRev' ), this );
+			this.diffPage.replaceState( mw.config.get( 'extRevisionSliderNewRev' ), mw.config.get( 'extRevisionSliderOldRev' ), this );
 			this.diffPage.initOnPopState( this );
 		},
 
@@ -229,7 +229,7 @@
 					var $p = $( this ),
 						relativeIndex = self.getRelativePointerIndex( $p ),
 						pointer = self.whichPointer( $p ),
-						revId1, revId2;
+						diff, oldid;
 
 					self.isDragged = false;
 					self.removePointerDragCursor();
@@ -246,15 +246,15 @@
 					self.resetPointerStylesBasedOnPosition();
 					self.resetRevisionStylesBasedOnPointerPosition( $revisions );
 
-					revId1 = self.getRevElementAtPosition(
-						$revisions, self.pointerOlder.getPosition()
-					).data( 'revid' );
-
-					revId2 = self.getRevElementAtPosition(
+					diff = self.getRevElementAtPosition(
 						$revisions, self.pointerNewer.getPosition()
 					).data( 'revid' );
 
-					self.refreshRevisions( revId1, revId2 );
+					oldid = self.getRevElementAtPosition(
+						$revisions, self.pointerOlder.getPosition()
+					).data( 'revid' );
+
+					self.refreshRevisions( diff, oldid );
 
 					self.redrawPointerLines();
 				},
@@ -347,10 +347,19 @@
 			}
 			pClicked.setPosition( targetPos );
 			view.updatePointerPositionAttributes();
-			view.refreshRevisions(
-				+view.getRevElementAtPosition( $revisions, pOther.getPosition() ).data( 'revid' ),
-				+$clickedRev.data( 'revid' )
-			);
+
+			if ( hasClickedTop ) {
+				view.refreshRevisions(
+					+$clickedRev.data( 'revid' ),
+					+view.getRevElementAtPosition( $revisions, pOther.getPosition() ).data( 'revid' )
+				);
+			} else {
+				view.refreshRevisions(
+					+view.getRevElementAtPosition( $revisions, pOther.getPosition() ).data( 'revid' ),
+					+$clickedRev.data( 'revid' )
+				);
+			}
+
 			view.resetPointerColorsBasedOnValues( view.pointerOlder.getPosition(), view.pointerNewer.getPosition() );
 			view.resetRevisionStylesBasedOnPointerPosition( $revisions );
 			view.alignPointers();
@@ -377,14 +386,12 @@
 		/**
 		 * Refreshes the diff page to show the diff for the specified revisions
 		 *
-		 * @param {number} revId1
-		 * @param {number} revId2
+		 * @param {number} diff
+		 * @param {number} oldid
 		 */
-		refreshRevisions: function ( revId1, revId2 ) {
-			var oldRev = Math.min( revId1, revId2 ),
-				newRev = Math.max( revId1, revId2 );
-			this.diffPage.refresh( oldRev, newRev, this );
-			this.diffPage.pushState( oldRev, newRev, this );
+		refreshRevisions: function ( diff, oldid ) {
+			this.diffPage.refresh( diff, oldid, this );
+			this.diffPage.pushState( diff, oldid, this );
 		},
 
 		showNextDiff: function () {
@@ -407,8 +414,8 @@
 			);
 			this.updatePointerPositionAttributes();
 			this.refreshRevisions(
-				$( '.mw-revslider-revision[data-pos="' + this.pointerOlder.getPosition() + '"]' ).attr( 'data-revid' ),
-				$( '.mw-revslider-revision[data-pos="' + this.pointerNewer.getPosition() + '"]' ).attr( 'data-revid' )
+				$( '.mw-revslider-revision[data-pos="' + this.pointerNewer.getPosition() + '"]' ).attr( 'data-revid' ),
+				$( '.mw-revslider-revision[data-pos="' + this.pointerOlder.getPosition() + '"]' ).attr( 'data-revid' )
 			);
 		},
 
@@ -846,7 +853,7 @@
 
 			revIdOld = self.getRevElementAtPosition( $revisions, pOld.getPosition() ).data( 'revid' );
 			revIdNew = self.getRevElementAtPosition( $revisions, pNew.getPosition() ).data( 'revid' );
-			this.diffPage.replaceState( revIdOld, revIdNew, this );
+			this.diffPage.replaceState( revIdNew, revIdOld, this );
 
 			scrollLeft = this.slider.getOldestVisibleRevisionIndex() * this.revisionWidth;
 			$revisionContainer.scrollLeft( scrollLeft );

modules/ext.RevisionSlider.SliderViewTwo.js

@@ -188,10 +188,17 @@
 
 			pointerMoved.setPosition( pos );
 			this.updatePointerPositionAttributes();
-			this.refreshRevisions(
-				this.getRevElementAtPosition( $revisions, pointerOther.getPosition() ).attr( 'data-revid' ),
-				$clickedRev.attr( 'data-revid' )
-			);
+			if ( $line.hasClass( 'mw-revslider-pointer-container-newer' ) ) {
+				this.refreshRevisions(
+					$clickedRev.attr( 'data-revid' ),
+					this.getRevElementAtPosition( $revisions, pointerOther.getPosition() ).attr( 'data-revid' )
+				);
+			} else {
+				this.refreshRevisions(
+					this.getRevElementAtPosition( $revisions, pointerOther.getPosition() ).attr( 'data-revid' ),
+					$clickedRev.attr( 'data-revid' )
+				);
+			}
 			this.resetRevisionStylesBasedOnPointerPosition( $revisions );
 			this.alignPointers();
 		},

tests/browser/features/pointers.feature

@@ -26,16 +26,3 @@ Feature: RevisionSlider pointers
     And the upper pointer should be on revision 4
     And revision 3 should be loaded on the left of the diff
     And revision 4 should be loaded on the right of the diff
-
-  Scenario: RevisionSlider pointers switch when crossed over
-    Given I am on the diff page
-    When I have loaded the RevisionSlider and dismissed the help dialog
-    And I drag the lower pointer to revision 3
-    And I wait until the diff has loaded
-    And I click on revision 1 to move the upper pointer
-    And I wait until the diff has loaded
-    And I wait until the pointers stopped moving
-    Then the lower pointer should be on revision 1
-    And the upper pointer should be on revision 3
-    And revision 1 should be loaded on the left of the diff
-    And revision 3 should be loaded on the right of the diff

tests/qunit/RevisionSlider.DiffPage.test.js

@@ -32,8 +32,8 @@
 		assert.propEqual(
 			history.state,
 			{
-				revid1: 3,
-				revid2: 37,
+				diff: 3,
+				oldid: 37,
 				pointerOlderPos: 1,
 				pointerNewerPos: 3,
 				sliderPos: NaN