r113775 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r113774‎ | r113775 | r113776 >
Date:22:38, 13 March 2012
Author:brion
Status:fixme (Comments)
Tags:needs-js-test 
Comment:
* (bug 35201) Edit buttons no longer cause jump in IE 9

Changed order of feature-detection checks so we prefer the IE method over the Mozilla method in encapsulateSelection. Fixes a problem where IE9 triggered the Mozilla mode, then mysteriously scrolled to the top of the page.

(IE9 and IE10 add a lot of standard and semi-standard HTML APIs, so this sort of thing happens from time to time. :)
Modified paths:
  • /trunk/phase3/resources/jquery/jquery.textSelection.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.textSelection.js
@@ -131,8 +131,58 @@
132132 var isSample = false;
133133 if ( this.style.display == 'none' ) {
134134 // Do nothing
 135+ } else if ( document.selection && document.selection.createRange ) {
 136+ // IE
 137+
 138+ // Note that IE9 will trigger the next section unless we check this first.
 139+ // See bug 35201.
 140+
 141+ activateElementOnIE( this );
 142+ if ( context ) {
 143+ context.fn.restoreCursorAndScrollTop();
 144+ }
 145+ if ( options.selectionStart !== undefined ) {
 146+ $(this).textSelection( 'setSelection', { 'start': options.selectionStart, 'end': options.selectionEnd } );
 147+ }
 148+
 149+ var selText = $(this).textSelection( 'getSelection' );
 150+ var scrollTop = this.scrollTop;
 151+ var range = document.selection.createRange();
 152+
 153+ checkSelectedText();
 154+ var insertText = pre + selText + post;
 155+ if ( options.splitlines ) {
 156+ insertText = doSplitLines( selText, pre, post );
 157+ }
 158+ if ( options.ownline && range.moveStart ) {
 159+ var range2 = document.selection.createRange();
 160+ range2.collapse();
 161+ range2.moveStart( 'character', -1 );
 162+ // FIXME: Which check is correct?
 163+ if ( range2.text != "\r" && range2.text != "\n" && range2.text != "" ) {
 164+ insertText = "\n" + insertText;
 165+ pre += "\n";
 166+ }
 167+ var range3 = document.selection.createRange();
 168+ range3.collapse( false );
 169+ range3.moveEnd( 'character', 1 );
 170+ if ( range3.text != "\r" && range3.text != "\n" && range3.text != "" ) {
 171+ insertText += "\n";
 172+ post += "\n";
 173+ }
 174+ }
 175+
 176+ range.text = insertText;
 177+ if ( isSample && options.selectPeri && range.moveStart ) {
 178+ range.moveStart( 'character', - post.length - selText.length );
 179+ range.moveEnd( 'character', - post.length );
 180+ }
 181+ range.select();
 182+ // Restore the scroll position
 183+ this.scrollTop = scrollTop;
135184 } else if ( this.selectionStart || this.selectionStart == '0' ) {
136185 // Mozilla/Opera
 186+
137187 $(this).focus();
138188 if ( options.selectionStart !== undefined ) {
139189 $(this).textSelection( 'setSelection', { 'start': options.selectionStart, 'end': options.selectionEnd } );
@@ -182,51 +232,6 @@
183233 this.selectionStart = startPos + insertText.length;
184234 this.selectionEnd = this.selectionStart;
185235 }
186 - } else if ( document.selection && document.selection.createRange ) {
187 - // IE
188 - activateElementOnIE( this );
189 - if ( context ) {
190 - context.fn.restoreCursorAndScrollTop();
191 - }
192 - if ( options.selectionStart !== undefined ) {
193 - $(this).textSelection( 'setSelection', { 'start': options.selectionStart, 'end': options.selectionEnd } );
194 - }
195 -
196 - var selText = $(this).textSelection( 'getSelection' );
197 - var scrollTop = this.scrollTop;
198 - var range = document.selection.createRange();
199 -
200 - checkSelectedText();
201 - var insertText = pre + selText + post;
202 - if ( options.splitlines ) {
203 - insertText = doSplitLines( selText, pre, post );
204 - }
205 - if ( options.ownline && range.moveStart ) {
206 - var range2 = document.selection.createRange();
207 - range2.collapse();
208 - range2.moveStart( 'character', -1 );
209 - // FIXME: Which check is correct?
210 - if ( range2.text != "\r" && range2.text != "\n" && range2.text != "" ) {
211 - insertText = "\n" + insertText;
212 - pre += "\n";
213 - }
214 - var range3 = document.selection.createRange();
215 - range3.collapse( false );
216 - range3.moveEnd( 'character', 1 );
217 - if ( range3.text != "\r" && range3.text != "\n" && range3.text != "" ) {
218 - insertText += "\n";
219 - post += "\n";
220 - }
221 - }
222 -
223 - range.text = insertText;
224 - if ( isSample && options.selectPeri && range.moveStart ) {
225 - range.moveStart( 'character', - post.length - selText.length );
226 - range.moveEnd( 'character', - post.length );
227 - }
228 - range.select();
229 - // Restore the scroll position
230 - this.scrollTop = scrollTop;
231236 }
232237 $(this).trigger( 'encapsulateSelection', [ options.pre, options.peri, options.post, options.ownline,
233238 options.replace, options.spitlines ] );

Sign-offs

UserFlagDate
Laberkisteinspected22:42, 13 March 2012
Krinkleinspected03:43, 15 March 2012

Follow-up revisions

RevisionCommit summaryAuthorDate
r113786MFT r113775reedy23:40, 13 March 2012
r114015MFT r112918, r113214, r113268, r113277, r113312, r113415, r113454, r113737, r...reedy15:18, 16 March 2012

Comments

#Comment by Catrope (talk | contribs)   23:01, 14 March 2012

Looks fine, marking OK. I would recommend you double-check this in Opera though, because I seem to recall it having the reverse problem (implementing the Mozilla API well and the IE API poorly).

#Comment by Brion VIBBER (talk | contribs)   23:03, 14 March 2012

I tested briefly with a current version of Opera, but it wouldn't hurt to do a more thorough check.

#Comment by Krinkle (talk | contribs)   07:04, 19 March 2012
#Comment by Krinkle (talk | contribs)   07:04, 19 March 2012
#Comment by Brion VIBBER (talk | contribs)   17:51, 26 March 2012

Runs clean in IE9 in my Windows 7 VM; might be one of those issues where being in background screws it up...

#Comment by Krinkle (talk | contribs)   20:50, 24 March 2012
  • bump*

Status & tagging log