r63566 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r63565‎ | r63566 | r63567 >
Date:19:20, 10 March 2010
Author:nimishg
Status:resolved (Comments)
Tags:
Comment:
restored previous functionality of hitting 'enter' inside a template
Modified paths:
  • /trunk/extensions/UsabilityInitiative/UsabilityInitiative.hooks.php (modified) (history)
  • /trunk/extensions/UsabilityInitiative/js/plugins.combined.js (modified) (history)
  • /trunk/extensions/UsabilityInitiative/js/plugins.combined.min.js (modified) (history)
  • /trunk/extensions/UsabilityInitiative/js/plugins/jquery.wikiEditor.highlight.js (modified) (history)

Diff [purge]

Index: trunk/extensions/UsabilityInitiative/UsabilityInitiative.hooks.php
——@@ -73,7 +73,7 @@
7474 array( 'src' => 'js/plugins/jquery.suggestions.js', 'version' => 7 ),
7575 array( 'src' => 'js/plugins/jquery.textSelection.js', 'version' => 27 ),
7676 array( 'src' => 'js/plugins/jquery.wikiEditor.js', 'version' => 162 ),
77 - array( 'src' => 'js/plugins/jquery.wikiEditor.highlight.js', 'version' => 38 ),
 77+ array( 'src' => 'js/plugins/jquery.wikiEditor.highlight.js', 'version' => 39 ),
7878 array( 'src' => 'js/plugins/jquery.wikiEditor.toolbar.js', 'version' => 52 ),
7979 array( 'src' => 'js/plugins/jquery.wikiEditor.dialogs.js', 'version' => 19 ),
8080 array( 'src' => 'js/plugins/jquery.wikiEditor.toc.js', 'version' => 97 ),
——@@ -82,10 +82,10 @@
8383 array( 'src' => 'js/plugins/jquery.wikiEditor.publish.js', 'version' => 3 ),
8484 ),
8585 'combined' => array(
86 - array( 'src' => 'js/plugins.combined.js', 'version' => 317 ),
 86+ array( 'src' => 'js/plugins.combined.js', 'version' => 318 ),
8787 ),
8888 'minified' => array(
89 - array( 'src' => 'js/plugins.combined.min.js', 'version' => 317 ),
 89+ array( 'src' => 'js/plugins.combined.min.js', 'version' => 318 ),
9090 ),
9191 ),
9292 );
Index: trunk/extensions/UsabilityInitiative/js/plugins/jquery.wikiEditor.highlight.js
——@@ -185,7 +185,7 @@
186186 //start same
187187 oldStarts[markers[i].start+","+markers[i].type] = true;
188188 if(division == 'realchange'){
189 - cBreak = true;
 189+ cBreak = true; //already existed, offsets fine
190190 }
191191 }
192192 else{
——@@ -194,7 +194,7 @@
195195 context.modules.highlight.markersOldStarts = oldStarts;
196196 purgeStarts = true;
197197 if(division == 'realchange'){
198 - cBreak = true;
 198+ cBreak = true; //user is currently typing in something whose begin exists
199199 }
200200 }
201201 context.modules.highlight.markersOldStarts[markers[i].start+","+markers[i].type] = true;
——@@ -312,14 +312,12 @@
313313 var ca1 = startNode, ca2 = endNode;
314314 if ( ca1 && ca2 && ca1.parentNode ) {
315315 var anchor = markers[i].getAnchor( ca1, ca2 );
316 - if(cBreak){
317 - return;
318 - }
319 - if ( !anchor ) {
 316+ if ( !anchor && !cBreak ) { //cBreak prevents a new span from being created which stops cursor jumps
320317 var commonAncestor = ca1.parentNode;
321 - if ( markers[i].anchor == 'wrap' ) {
 318+ if ( markers[i].anchor == 'wrap') {
322319 // We have to store things like .parentNode and .nextSibling because
323320 // appendChild() changes these properties
 321+
324322 var newNode = ca1.ownerDocument.createElement( 'span' );
325323
326324 var nextNode = ca2.nextSibling;
——@@ -331,6 +329,7 @@
332330 n = ns;
333331 }
334332 // Insert newNode in the right place
 333+
335334 if ( nextNode ) {
336335 commonAncestor.insertBefore( newNode, nextNode );
337336 } else {
——@@ -343,7 +342,6 @@
344343 }
345344 $( anchor ).data( 'marker', markers[i] )
346345 .addClass( 'wikiEditor-highlight' );
347 -
348346 // Allow the module adding this marker to manipulate it
349347 markers[i].afterWrap( anchor, markers[i] );
350348
Index: trunk/extensions/UsabilityInitiative/js/plugins.combined.js
——@@ -8646,7 +8646,7 @@
86478647 //start same
86488648 oldStarts[markers[i].start+","+markers[i].type] = true;
86498649 if(division == 'realchange'){
8650 - cBreak = true;
 8650+ cBreak = true; //already existed, offsets fine
86518651 }
86528652 }
86538653 else{
——@@ -8655,7 +8655,7 @@
86568656 context.modules.highlight.markersOldStarts = oldStarts;
86578657 purgeStarts = true;
86588658 if(division == 'realchange'){
8659 - cBreak = true;
 8659+ cBreak = true; //user is currently typing in something whose begin exists
86608660 }
86618661 }
86628662 context.modules.highlight.markersOldStarts[markers[i].start+","+markers[i].type] = true;
——@@ -8773,14 +8773,12 @@
87748774 var ca1 = startNode, ca2 = endNode;
87758775 if ( ca1 && ca2 && ca1.parentNode ) {
87768776 var anchor = markers[i].getAnchor( ca1, ca2 );
8777 - if(cBreak){
8778 - return;
8779 - }
8780 - if ( !anchor ) {
 8777+ if ( !anchor && !cBreak ) { //cBreak prevents a new span from being created which stops cursor jumps
87818778 var commonAncestor = ca1.parentNode;
8782 - if ( markers[i].anchor == 'wrap' ) {
 8779+ if ( markers[i].anchor == 'wrap') {
87838780 // We have to store things like .parentNode and .nextSibling because
87848781 // appendChild() changes these properties
 8782+
87858783 var newNode = ca1.ownerDocument.createElement( 'span' );
87868784
87878785 var nextNode = ca2.nextSibling;
——@@ -8792,6 +8790,7 @@
87938791 n = ns;
87948792 }
87958793 // Insert newNode in the right place
 8794+
87968795 if ( nextNode ) {
87978796 commonAncestor.insertBefore( newNode, nextNode );
87988797 } else {
——@@ -8804,7 +8803,6 @@
88058804 }
88068805 $( anchor ).data( 'marker', markers[i] )
88078806 .addClass( 'wikiEditor-highlight' );
8808 -
88098807 // Allow the module adding this marker to manipulate it
88108808 markers[i].afterWrap( anchor, markers[i] );
88118809
Index: trunk/extensions/UsabilityInitiative/js/plugins.combined.min.js
——@@ -603,8 +603,7 @@
604604 oldParent.parentNode.removeChild(oldParent);}
605605 lastP=t.inP;}
606606 context.fn.purgeOffsets();}
607 -var ca1=startNode,ca2=endNode;if(ca1&&ca2&&ca1.parentNode){var anchor=markers[i].getAnchor(ca1,ca2);if(cBreak){return;}
608 -if(!anchor){var commonAncestor=ca1.parentNode;if(markers[i].anchor=='wrap'){var newNode=ca1.ownerDocument.createElement('span');var nextNode=ca2.nextSibling;var n=ca1;while(n!=nextNode){var ns=n.nextSibling;newNode.appendChild(n);n=ns;}
 607+var ca1=startNode,ca2=endNode;if(ca1&&ca2&&ca1.parentNode){var anchor=markers[i].getAnchor(ca1,ca2);if(!anchor&&!cBreak){var commonAncestor=ca1.parentNode;if(markers[i].anchor=='wrap'){var newNode=ca1.ownerDocument.createElement('span');var nextNode=ca2.nextSibling;var n=ca1;while(n!=nextNode){var ns=n.nextSibling;newNode.appendChild(n);n=ns;}
609608 if(nextNode){commonAncestor.insertBefore(newNode,nextNode);}else{commonAncestor.appendChild(newNode);}
610609 anchor=newNode;}else if(markers[i].anchor=='tag'){anchor=commonAncestor;}
611610 $(anchor).data('marker',markers[i]).addClass('wikiEditor-highlight');markers[i].afterWrap(anchor,markers[i]);}else{$(anchor).data('marker',markers[i]);markers[i].onSkip(anchor);}

Comments

#Comment by Pdhanda (talk | contribs)   22:02, 10 March 2010 
-				if(cBreak){
-					return;
-				}
-				if ( !anchor ) {
+				if ( !anchor && !cBreak ) { //cBreak prevents a new span from being created which stops cursor jumps
 					var commonAncestor = ca1.parentNode;

is still causing issues. I get javascript errors on template wrap. markers[i].onSkip( anchor ); gets called with a null.

#Comment by Catrope (talk | contribs)   15:23, 11 March 2010 
 				var anchor = markers[i].getAnchor( ca1, ca2 );
-				if ( !anchor ) {
+				if ( !anchor && !cBreak ) { //cBreak prevents a new span from being created which stops cursor jumps
... (create anchor)
				} else {
... (do something with anchor)
				}

This obviously fails when anchor is null but cBreak is true. The else { should become else if ( anchor ) { so the code inside is only executed if anchor isn't null, and an else { block handling the case where !anchor && cBreak (what's even supposed to happen here?) should be added.

However, as I've already said in an e-mail, I don't believe this is the right way to fix these cursor moving issues. Not calling onSkip() for them is not the right way, as onSkip()'s duty is to update the model when the HTML inside the template container changes. Instead, the code in onSkip() causing the cursor to move needs to be fixed. I'll take a closer look at doing the latter later.

#Comment by Nimish Gautam (talk | contribs)   22:30, 16 March 2010

This code has been removed

Status & tagging log