Last modified: 2012-02-22 12:37:17 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T34241, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 32241 - WikiEditor scrolls the browser and does not insert on IE7, IE8, IE9 if textarea doesn't fit on screen
WikiEditor scrolls the browser and does not insert on IE7, IE8, IE9 if textar...
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
WikiEditor (Other open bugs)
unspecified
PC Windows 7
: Normal normal (vote)
: ---
Assigned To: Nobody - You can work on this!
: javascript, patch, patch-need-review
: 32190 (view as bug list)
Depends on:
Blocks: 24493
  Show dependency treegraph
 
Reported: 2011-11-07 19:35 UTC by Dan Barrett
Modified: 2012-02-22 12:37 UTC (History)
6 users (show)

See Also:
Web browser: Internet Explorer
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch to restore window's scroll state after focus() on IE (1.67 KB, patch)
2011-11-13 13:59 UTC, Lupo
Details
jquery.textSelection.js patch to restore window's scroll state after focus() on IE (1.77 KB, patch)
2011-11-14 07:45 UTC, Lupo
Details
Follow-up patch (apply after attachment 9441): use setActive() instead of focus() if possible (1.53 KB, patch)
2011-11-14 20:58 UTC, Lupo
Details

Description Dan Barrett 2011-11-07 19:35:45 UTC
On Windows 7 with IE8, make sure the edit page is scrolled all the way to the top and click a WikiEditor button (such as Bold) or use a dropdown (such as Heading). The entire browser page scrolls downward and the button/dropdown does not insert any text into the edit box.

Now, leave the browser window in its current (scrolled) state and click the button (or use the dropdown) again. The proper text gets inserted.

Scroll the page back up to the top.  Click the Bold button again.  Same behavior: the browser scrolls and nothing gets inserted.

This bug feels similar to 31847 and might be related. It happens only on Windows 7 (as far as I can tell). It happens on Wikipedia right now (1.18wfm1 r101368), and also on Mediawiki 1.17.0 that has been patched with the solution to the related bug 31847 (i.e., r100398).

The bad behavior happens regardless of whether the cursor focus is in the edit box, or whether text is highlighted in the edit box. I also turned off all additional features in Preferences (Side-by-side preview, etc.)

I tried this on a non-Windows-7 system (Windows 2008 Server) with IE8 and could not reproduce the problem.  But it was easy to reproduce on Windows 7.
Comment 1 Dan Barrett 2011-11-07 19:43:30 UTC
I have reproduced this on three Windows 7 machines now.
Comment 2 Mark A. Hershberger 2011-11-07 21:01:24 UTC
marking for 1.18 tarball.  Thanks!
Comment 3 Helder 2011-11-10 00:43:25 UTC
I tried to reproduce this accessing the page in a VirtualBox having Windows 7:
http://en.wikipedia.org/w/index.php?title=Palm_VII&action=edit

* I confirmed the bug when I used Internet Explorer 8.0.7600.16385
* The bug didn't happen when I used Google Chrome 15.0.874.106 m

I was not logged in during the tests.
Comment 4 Lupo 2011-11-10 23:50:00 UTC
Tried it with the "Bold" button. In IE8 debugger, each time the whole contents scrolled and no sample text was inserted (i.e., when I had the window scrolled to the top and nothing selected), the click() event bound on that button at line 373 in jQuery.wikiEditor.toolbar.js simply didn't fire.

I didn't find out why, though.

I noticed this on-wiki change at the Commons:

https://commons.wikimedia.org/w/index.php?title=MediaWiki:Edittools.js&diff=60823115&oldid=60462490

Could that be related? What was the regression Die Buche fixed there?

I also do wonder if it's really necessary to bind handlers for both mousedown and click on these buttons. Isn't a click handler sufficient, if it saves the text scroll/text selection before calling ...toolbar.doAction()? Given that a click is mousedown followed by mouseup, I could easily imagine that IE under some circumstances might not trigger the click() if the mousedown() is suppressed (e.preventDefault(), return false).

But then, I'm totally unfamiliar with this WikiEditor code, so I may be way off, and there may be good reasons to use both events...
Comment 5 Brion Vibber 2011-11-11 01:20:20 UTC
Testing hitting "Bold" button at http://en.wikipedia.org/w/index.php?title=Palm_VII&action=edit

Expected ok behavior: inserts '''Bold text''' at start of document
Expected bug behavior: scrolls down, then starts inserting '''Bold text''' somewhere partway through the doc

Windows XP SP3 with IE 8.0.6001.18702: ok
Windows 7 Home Premium with IE 9: ok 
Windows 7 Home Premium downgraded to IE 8.0.7601.17514 by removing IE 9: bug!

Aha! The bug only occurs if the *textarea didn't start out fully on screen*. My window was smaller after downgrading, so I hit the bug.

In this case I can repro it on XP SP3 as well as on Windows 7 -- so it's IE 8-specific not Windows 7-specific.
Comment 6 Brion Vibber 2011-11-11 01:42:57 UTC
The scrolling happens on IE 7 and 8, and sorta but not as bad on IE 9 -- but only if the window isn't tall enough to fit the entire textarea.

Insertion in wrong position on the second click (as in bug 31847) happens on IE 7 and IE 8 but not IE 9, where the insert seems to go right.

If the window still isn't tall enough after scrolling, second click still doesn't insert -- we end up scrolling to start of the textarea on every click.


WikiEditor not enabled:
* Windows XP SP 3 with IE 6

No problems if window is tall enough to fit the entire textarea at initial scroll position:
* Windows XP SP3 with IE 7
* Windows XP SP3 with IE 8
* Windows 7 with IE 8
* Windows 7 with IE 9

If window not tall enough: first click scrolls down, second click inserts '''Bold''' at an unexpected position:
* Windows XP SP3 with IE 7
* Windows XP SP3 with IE 8
* Windows 7 with IE 8

If window not tall enough: first click does cause scroll, but sets the focus and cursor where we expect it; second click inserts '''Bold''' at caret position at start of page:
* Windows 7 with IE 9
Comment 7 Lupo 2011-11-11 07:31:03 UTC
Here's my conjecture:

On mousedown on the button, we get the current selection. To do so, we need a textarea.focus() on IE. IE8 seems to scrollIntoView() the textarea upon focus under the above circumstances (small window, textarea not wholly visible and not already at the top of the viewport -- which it never is, because then the button would not be visible).

After that scrollIntoView(), the element under the mouse pointer no longer is the button, but the textarea itself. When the mouse button is released, the button does not get that event, and thus doesn't trigger the click(), since it never sees the mouseup.

Hence the question is "how to prevent IE to scrollIntoView() the textarea upon textarea.focus()?"

Maybe suppressing the propagation of the resulting focusin event on the textarea.

If that's not possible, another approach might be getting and re-setting the global (document's) scrollTop/scrollLeft anywhere you focus() inside the mousedown of such a mousedown-click construction.
Comment 8 Lupo 2011-11-11 11:11:40 UTC
Just took a look at the old pre-RL version of [[:commons:MediaWiki:Edittools.js]], which is of interest here because the current RL-version exhibits a similar problem (also scrolls the textarea to the top of the viewport, and inserts at a bogus location).

Now, the last pre-RL version of that at

https://commons.wikimedia.org/w/index.php?title=MediaWiki:Edittools.js&oldid=40547085

does exactly what I proposed in comment 7 above: it saves and restores the windows' scroll position.

That code in the insertTags() function comes from the old bug 10526 (fixed more than four years ago); the code is due to AlexSm.

That should at least avoid (or rather, undo) the scrolling. Remains to be seen if it then inserts at a sane location.
Comment 9 Lupo 2011-11-13 13:59:31 UTC
Created attachment 9436 [details]
Patch to restore window's scroll state after focus() on IE

The attached patch solves the *functional* problem, i.e., the insertion at a bogus position, or even non-insertion of the text. With this patch, a click on the "bold" button does insert at the correct position.

Th patch solves the scrolling issue only halfway: while the textbox is where it should be in the end, the screen flickers briefly.

I have not been able to find out where exactly this flickering comes from. In debugging, it appears to occur during the mousedown event, but I don't see why. When stepping over the call to context.fn.saveCursorAndScrollTop(), it flickers; but when stepping through, it doesn't. Executing without debugging also flickers.

Also note that there a few more focus() calls that do not go through the jQuery focus() but call the native focus() method directly on DOM elements; these are of course not caught by this patch to the jQuery focus(). I see such direct focusing in

jQuery.wikiEditor.dialogs.config.js, line 1123
jQuery.wikiEditor.iframe.js (5 times, 4 times on $iframe[0].contentWindow)
jQuery.wikiEditor.toolbar.js, line 284 (on $iframe[0].contentWindow)
jQuery.ui.accordion.js, line 243

If one of these can be invoked here (other event listeners?), that might be the cause of the flickering.

Could somebody else please take a look and try to figure out where this flickering comes from?
Comment 10 Lupo 2011-11-14 07:45:36 UTC
Created attachment 9441 [details]
jquery.textSelection.js patch to restore window's scroll state after focus() on IE

Patch corrected. Behavior identical.
Comment 11 Roan Kattouw 2011-11-14 08:37:55 UTC
(In reply to comment #10)
> Created attachment 9441 [details]
> jquery.textSelection.js patch to restore window's scroll state after focus() on
> IE
> 
> Patch corrected. Behavior identical.
Thanks for your patch. Applied in r102945.
Comment 12 Lupo 2011-11-14 08:57:43 UTC
What about the flickering and the remaining direct calls to native focus() in the other sources? I wrote in comment 9 that this patch did *not* fully solve the problem. It fixes the functionality, but the visual effects are hideous.
Comment 13 Roan Kattouw 2011-11-14 09:05:17 UTC
(In reply to comment #12)
> What about the flickering and the remaining direct calls to native focus() in
> the other sources? I wrote in comment 9 that this patch did *not* fully solve
> the problem. It fixes the functionality, but the visual effects are hideous.
Hmm. I don't know where the flickering comes from. I have a hard time seeing how the native focus() calls you've listed (which are still a problem, granted), would be reached when clicking the bold button, or at all.
Comment 14 Lupo 2011-11-14 12:06:07 UTC
It appears that IE makes a distinction between the focused element and the active element. A focused element always is also the active element, but an element can be active without being focused.

The point with IE is: if an element is focused, the browser brings it into view, as we've seen. If you only activate an element, it doesn't!

See 
http://msdn.microsoft.com/en-us/library/ms536738%28v=VS.85%29.aspx (setActive)
and
http://msdn.microsoft.com/en-us/library/ms536425%28v=VS.85%29.aspx (focus)

It appears that an element must be active, not necessarily focused, for createRange() to work properly.

I've played around using textarea.setActive() instead of textarea.focus(), and that indeed seems to solve this IE-specific problem completely: insertions work as expected, and there's no screen flicker on IE8 anymore.

(Test code replacing jquery.textSelection is at [[:commons:User:Lupo Test/common.js]].)

I'll test a bit more, and if I don't find any problems, I'll post a new patch tonight (UTC). Using an IE-specific non-standard operation like setActive() should be OK in these code paths that are taken on IE only anyway.
Comment 15 Roan Kattouw 2011-11-14 13:33:30 UTC
(In reply to comment #14)
> I'll test a bit more, and if I don't find any problems, I'll post a new patch
> tonight (UTC). Using an IE-specific non-standard operation like setActive()
> should be OK in these code paths that are taken on IE only anyway.
Yay, thanks!
Comment 16 Lupo 2011-11-14 20:58:55 UTC
Created attachment 9449 [details]
Follow-up patch (apply after attachment 9441 [details]): use setActive() instead of focus() if possible

Follow-up patch based on trunk after Roan's commit in r102948 (i.e., after the focus() patch in attachment 9441 [details]).

This patch uses the IE-specific setActive() operation in preference over focus(), if possible. Using setActive() avoids the screen flicker, and insertions work as expected.

Tested manually on IE8/Win7 and IE8/WinXP. Also tested in IE8(Compatibility mode) on Win7 (the closest I can get to an IE7... I have neither IE7 nor IE9 available). I didn't notice any problems.
Comment 17 Lupo 2011-11-14 23:02:31 UTC
*** Bug 32190 has been marked as a duplicate of this bug. ***
Comment 18 Roan Kattouw 2011-11-15 10:00:06 UTC
(In reply to comment #16)
> Created attachment 9449 [details]
> Follow-up patch (apply after attachment 9441 [details]): use setActive() instead of
> focus() if possible
> 
> Follow-up patch based on trunk after Roan's commit in r102948 (i.e., after the
> focus() patch in attachment 9441 [details]).
> 
> This patch uses the IE-specific setActive() operation in preference over
> focus(), if possible. Using setActive() avoids the screen flicker, and
> insertions work as expected.
> 
> Tested manually on IE8/Win7 and IE8/WinXP. Also tested in IE8(Compatibility
> mode) on Win7 (the closest I can get to an IE7... I have neither IE7 nor IE9
> available). I didn't notice any problems.
Thanks! Applied in r103138.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links