Page MenuHomePhabricator

Check for current presence of old IE bug in WikiEditor
Closed, ResolvedPublic

Description

The Internet Explorer/Spartan team is asking about issues related to UA-sniffing bug workarounds in our code; there may be "surprises" as our current sniffing code lists their new "edge mode" engine as 'msie' although many of those old bugs are long gone.

One such 'msie' check is the scroll & selection save-and-restore dance that's used a lot in WikiEditor (saveCursorAndScrollTop/restoreCursorAndScrollTop).

Need to check if bug has disappeared in some particular version of IE (or in the latest Edge mode) and then add the version check to the bug workaround's sniffing usage.

Event Timeline

brooke claimed this task.
brooke raised the priority of this task from to Low.
brooke updated the task description. (Show Details)
brooke added a project: WikiEditor.
brooke subscribed.

Tracked the workaround code down to these bugs reported against IE 8 back years ago:

However I can not reproduce the bug with the code disabled on IE 8, 9, 10, or 11 virtual machines. It may have been fixed in some software update on IE 8 long ago?

Safest thing for now is probably to wrap the 'msie' check in a check also for the old document.selection API, which means the hack will disable itself on IE 11 and above automatically.

gerritbot subscribed.

Change 189162 had a related patch set uploaded (by Brion VIBBER):
Skip an old IE 8 bug workaround in WikiEditor on IE 11/Spartan

https://gerrit.wikimedia.org/r/189162

Patch-For-Review

In T88875#1022326, @brion wrote:

Tracked the workaround code down to these bugs reported against IE 8 back years ago:

However I can not reproduce the bug with the code disabled on IE 8, 9, 10, or 11 virtual machines.
It may have been fixed in some software update on IE 8 long ago?

Safest thing for now is probably to wrap the 'msie' check in a check also for the old document.selection API,
which means the hack will disable itself on IE 11 and above automatically.

Wasn't the IE8 scroll-bug addressed in one or both of these reports?

I know the first bug no. is still "mentioned" in the current jquery.wikiEditor.js file.

(I've been wrong before but figure best to mention it anyway since the commit message seems "more related" to these two "work-arounds" than the previously mentioned two.)

Hmmmmm perhaps (that does sound more like what I remember), but I couldn't repro those bugs either, even on IE 8/Win7.

You shouldn't be able to re-pro either of them as well as the other two in today's terms - they've all (finally) been resolved as far as I know (& have personally tested for as well).

The ''hidden comments'' found in the current jquery.wikiEditor.js file (among others) confuse matters more than clarify them since just about everything "scroll-bug-ish" has been attributed over time to a single report number (Bug 61908; now T63908).

In reality, the fixes targeting each scroll-ish nuance were applied across a span of several months (the "final" batch being merged in 2014), involved at least one other report if not more [to the best of my recollection] & only semi-related code-wise to each other at best.

Making sure the IE sniffing thing can detect & differentiate across all the versions still in play seems prudent but I'd also add/revise the current notes/comments to indicate today's bug-state-to-IE-version so if & when the day comes another particular version of IE becomes obsolete, is no longer supported & formally deprecated - all these piss-ant "fixes" & such can be removed safely rather than kept & continued into endless future revisions of the extension and/or its component file make-up. (IMHO that is)

Patchset updated, and one of the other version checks also clarified to explicitly test for '10 and below' rather than 'msie and also some other random bits'.

Referring to Patch Set 2:

  • Doesn't the workaround beginning around line 487 make the workaround beginning around line 321 redundant?
  • ...and shouldn't they be modified to follow the 'client.test...' approach as well instead of the current 'profile.name...' approach for IE detection or whatever its called regardless?

Adding Roan, as walking encyclopedia of 'weird IE bugs encountered when implementing wiki editors' and original author of the workaround.

Wondering if this code is superflous, with the actual problems now fixed in the textSelection plugin instead. (where we don't check for msie, but instead just for the document.selection variable.

Change 191698 had a related patch set uploaded (by Brion VIBBER):
Enable search-replace dialog on IE 11

https://gerrit.wikimedia.org/r/191698

Patch-For-Review

Note that the first patch is just a hygiene fix; the second patch actually fixes the search-replace dialog on IE 11 (confirmed on Windows 8.1 and 10 Tech Preview) which is a user-visible bug that the IE/Spartan team reported.

Change 191698 merged by jenkins-bot:
Enable search-replace dialog on IE 11

https://gerrit.wikimedia.org/r/191698

Change 189162 merged by jenkins-bot:
Skip an old IE 8 bug workaround in WikiEditor on IE 11/Spartan

https://gerrit.wikimedia.org/r/189162

T34184 WikiEditor: Replace $.client.profile().name === 'msie' with feature detection

The above seems related... should it be merged? closed?