Opened 6 years ago

Last modified 6 years ago

#18726 closed defect

scrollIntoView() suspicious code for IE8 — at Version 1

Reported by: bill Owned by:
Priority: undecided Milestone: 1.7.9
Component: Core Version: 1.7.7
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

Both test_scroll.html and window.js are accidentally including IE8, because the has("trident") flag is actually truthy for IE8 - IE11, not just IE11. The changes from https://github.com/dojo/dojo/commit/a832214a342b6e03ad4a9b1669fae71c7938c44b.

This code from windows.js:

if(elPos.x < 0 || !isIE || isIE >= 9 || has("trident")){ elPos.x = 0; } // older IE can have values > 0
if(elPos.y < 0 || !isIE || isIE >= 9 || has("trident")){ elPos.y = 0; }

The expression of isIE >= 9 || has("trident") is surely a mistake. But if the current code is working correctly on IE8, then the expression above can be simplified to:

elPos.x = elPos.y = 0;

This line is also wrong and may be breaking stuff:

if(rtl && ((isIE == 8 && !backCompat) || isIE >= 9 || has("trident"))){ s = -s; }

Relatedly, this code from test_scroll.html (deleted in master branch but in 1.10 and earlier) is inadvertently (and unnecessarily) skipping most of the tests on IE8:

var modes = (has("ie") >= 9 || has("trident")) ? ["_strict"] :
      ["_strict", "_quirks", "_loose_rtl", "_quirks_rtl" ];

So I'm guessing that https://github.com/dojo/dojo/commit/a832214a342b6e03ad4a9b1669fae71c7938c44b introduced some bugs in IE8 but they are being masked in the regression results.

Change History (1)

comment:1 Changed 6 years ago by bill

Description: modified (diff)
Note: See TracTickets for help on using tickets.