Refactor scroll handling into directive

services/web/app/views/project/editor/editor.jade

@@ -40,8 +40,8 @@ div.full-size(
 			on-ctrl-enter="recompileViaKey",
 			syntax-validation="settings.syntaxValidation",
 			review-panel="reviewPanel",
-			on-scroll="onScroll",
+			on-scroll="scrollBindings.onAceScroll",
-			scroll-events="scrollEvents",
+			scroll-events="scrollBindings.reviewPanelEvents",
 			track-changes-enabled="trackChangesFeatureFlag",
 			track-new-changes= "reviewPanel.trackNewChanges",
 			changes-tracker="reviewPanel.changesTracker",

services/web/app/views/project/editor/review-panel.jade

@@ -2,11 +2,11 @@
 	.review-panel-toolbar
 		| Track Changes 
 		input(type="checkbox", ng-model="reviewPanel.trackNewChanges")
-	.review-panel-scroller
+	.rp-entry-list(
-		.rp-entry-list(
+		review-panel-sorted
-			review-panel-sorted
+		ng-if="reviewPanel.subView === SubViews.CUR_FILE"
-			ng-if="reviewPanel.subView === SubViews.CUR_FILE"
+	)
-		)
+		.rp-entry-list-inner
 			.rp-entry-wrapper(
 				ng-repeat="(entry_id, entry) in reviewPanel.entries[editor.open_doc_id]"
 			)

services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee

@@ -19,6 +19,10 @@ define [
 			adding: false
 			content: ""
 
+		# Used to communicate between Ace and reviewPanelSorted directive
+		$scope.scrollBindings =
+			reviewPanelEvents: new EventEmitter()
+
 		changesTrackers = {}
 
 		$scope.$watch "editor.open_doc_id", (open_doc_id) ->
@@ -29,59 +33,10 @@ define [
 		# TODO Just for prototyping purposes; remove afterwards.
 		mockedUserId = '12345abc'
 
-		scroller = $element.find(".review-panel-scroller")
+		#TODO: Doesn't work anymore now entries is first indexed by doc_id
-		list = $element.find(".rp-entry-list")
-
-		# Use these to avoid unnecessary updates. Scrolling one
-		# panel causes us to scroll the other panel, but there's no
-		# need to trigger the event back to the original panel.
-		ignoreNextPanelEvent = false
-		ignoreNextAceEvent = false
-
-		$scope.scrollEvents = new EventEmitter()
-	
-		scrollPanel = (scrollTop, height) ->
-			if ignoreNextAceEvent
-				ignoreNextAceEvent = false
-			else
-				ignoreNextPanelEvent = true
-				list.height(height)
-				scroller.scrollTop(scrollTop)
-	
-		scrollAce = (e) ->
-			if ignoreNextPanelEvent
-				ignoreNextPanelEvent = false
-			else
-				ignoreNextAceEvent = true
-				$scope.scrollEvents.emit "scroll", e.target.scrollTop
-				
-		$scope.$watch "ui.reviewPanelOpen", (reviewPanelOpen) ->
-			return if !reviewPanelOpen?
-			if reviewPanelOpen
-				scroller.on "scroll", scrollAce
-				$scope.onScroll = scrollPanel # Passed into the editor directive for it to call
-			else
-				scroller.off "scroll"
-				$scope.onScroll = null
-			$timeout () ->
-				$scope.$broadcast "review-panel:toggle"
-				$scope.$broadcast "review-panel:layout"
-
 		$scope.$watch (() -> Object.keys($scope.reviewPanel.entries).length), (nEntries) ->
 			$scope.reviewPanel.hasEntries = nEntries > 0
 		
-		# If we listen for scroll events in the review panel natively, then with a Mac trackpad
-		# the scroll is very smooth (natively done I'd guess), but we don't get polled regularly
-		# enough to keep Ace in step, and it noticeably lags. If instead, we borrow the manual
-		# mousewheel/trackpad scrolling behaviour from Ace, and turn mousewheel events into
-		# scroll events ourselves, then it makes the review panel slightly less smooth (barely)
-		# noticeable, but keeps it perfectly in step with Ace.
-		ace.require("ace/lib/event").addMouseWheelListener scroller[0], (e) ->
-			deltaY = e.wheelY
-			# console.log "mousewheel", deltaY
-			scroller.scrollTop(scroller.scrollTop() + deltaY * 4)
-			e.preventDefault()
-		
 		$scope.acceptChange = (entry_id) ->
 			$scope.$broadcast "change:accept", entry_id
 		

services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee

@@ -88,4 +88,51 @@ define [
 				scope.$on "review-panel:layout", () ->
 					scope.$evalAsync () ->
 						layout()
+
+				## Scroll lock with Ace
+				scroller = element
+				list = element.find(".rp-entry-list-inner")
+				
+				# If we listen for scroll events in the review panel natively, then with a Mac trackpad
+				# the scroll is very smooth (natively done I'd guess), but we don't get polled regularly
+				# enough to keep Ace in step, and it noticeably lags. If instead, we borrow the manual
+				# mousewheel/trackpad scrolling behaviour from Ace, and turn mousewheel events into
+				# scroll events ourselves, then it makes the review panel slightly less smooth (barely)
+				# noticeable, but keeps it perfectly in step with Ace.
+				ace.require("ace/lib/event").addMouseWheelListener scroller[0], (e) ->
+					deltaY = e.wheelY
+					# console.log "mousewheel", deltaY
+					scroller.scrollTop(scroller.scrollTop() + deltaY * 4)
+					e.preventDefault()
+
+				# Use these to avoid unnecessary updates. Scrolling one
+				# panel causes us to scroll the other panel, but there's no
+				# need to trigger the event back to the original panel.
+				ignoreNextPanelEvent = false
+				ignoreNextAceEvent = false
+			
+				scrollPanel = (scrollTop, height) ->
+					console.log "Scrolling panel", scrollTop, height, list, scroller
+					if ignoreNextAceEvent
+						ignoreNextAceEvent = false
+					else
+						ignoreNextPanelEvent = true
+						list.height(height)
+						scroller.scrollTop(scrollTop)
+			
+				scrollAce = (e) ->
+					if ignoreNextPanelEvent
+						ignoreNextPanelEvent = false
+					else
+						ignoreNextAceEvent = true
+						scope.scrollBindings.reviewPanelEvents.emit "scroll", e.target.scrollTop
+
+				scroller.on "scroll", scrollAce
+				scope.scrollBindings.onAceScroll = scrollPanel
+
+				scope.$watch "ui.reviewPanelOpen", (reviewPanelOpen) ->
+					return if !reviewPanelOpen?
+					$timeout () ->
+						scope.$broadcast "review-panel:toggle"
+						scope.$broadcast "review-panel:layout"
 		}

services/web/public/stylesheets/app/editor/review-panel.less

@@ -70,6 +70,7 @@
 	border-left: solid 1px @rp-border-grey;
 	font-size: @rp-base-font-size;
 	color: @rp-type-blue;
+	overflow: hidden; // Stop inner hidden scroll bar showing
 }
 
 .review-panel-toolbar {
@@ -86,18 +87,18 @@
 	z-index: 2;
 }
 
-.review-panel-scroller {
+.rp-entry-list {
+	// TODO: Use a more cross-browser method of hiding the scroll bar
+	width: @review-off-width + @rp-scroller-offset;
+	padding-right: @rp-scroller-offset;
+	overflow-y: scroll;
 	position: absolute;
 	top: 0;
 	bottom: 0;
-	left: 0;
-	// TODO: Use a more cross-browser method of hiding the scroll bar
-	right: -@rp-scroller-offset; // Hide scroll bar
 }
 
-.rp-entry-list {
+.rp-entry-list-inner {
 	position: relative;
-	width: @review-off-width;
 }
 
 .rp-entry-indicator {
@@ -438,10 +439,6 @@
 		}
 	}
 
-	.review-panel-scroller {
-		overflow-y: scroll;
-	}
-
 	#review-panel {
 		display: block;
 		width: @review-panel-width;
@@ -453,7 +450,7 @@
 		}
 
 		.rp-entry-list {
-			width: @review-panel-width;
+			width: @review-panel-width + @rp-scroller-offset;
 		}
 
 			.rp-entry {