Remove IE-Version-Switches #233
Conversation
📝 WalkthroughWalkthroughRemoved legacy Internet Explorer detection, shims, and IE-specific branches across utils, DOM, binding, and tests; simplified implementations to modern DOM/event APIs and removed IE-related public exports and types. Changes
Sequence Diagram(s)(Skipped — changes are removals/simplifications across modules and do not introduce a new multi-component sequential flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/utils/src/dom/event.ts (5)
48-54: Remove IE-specific type guard.The
hasIEAttachEventsfunction is a type guard for Internet Explorer'sattachEventAPI. Since this PR removes IE support, this function is now dead code and should be deleted.🔎 Proposed fix
-function hasIEAttachEvents( - el: Element -): el is Element & { attachEvent: (event: string, handler: EventListener) => void } & { - detachEvent: (event: string, handler: EventListener) => void -} { - return typeof (el as any).attachEvent === 'function' && typeof (el as any).detachEvent === 'function' -} -
70-84: Remove IE attachEvent fallback path.This entire code block implements IE's legacy
attachEventAPI fallback. Since the PR removes IE support, this code should be deleted entirely, including:
- The attachEvent registration logic
- The disposal callback for IE memory leak prevention
- The error throw fallback
After removal, the function should only support
addEventListener(directly or via jQuery).🔎 Proposed fix
if (!options.useOnlyNativeEvents && !mustUseNative && jQuery) { jQuery(element).on(eventType, wrappedHandler) } else if (typeof element.addEventListener === 'function') { element.addEventListener(eventType, wrappedHandler, eventOptions) - } else if (hasIEAttachEvents(element)) { - const attachEventHandler = function (event) { - wrappedHandler.call(element, event) - } - const attachEventName = 'on' + eventType - element.attachEvent(attachEventName, attachEventHandler) - - // IE does not dispose attachEvent handlers automatically (unlike with addEventListener) - // so to avoid leaks, we have to remove them manually. See bug #856 - addDisposeCallback(element, function () { - element.detachEvent(attachEventName, attachEventHandler) - }) } else { throw new Error("Browser doesn't support addEventListener or attachEvent") }
91-93: Remove IE-specific type guard and fix existing bug.The
hasFireEventfunction is a type guard for Internet Explorer'sfireEventAPI. Since this PR removes IE support, this function should be deleted.Also note: Line 92 has a copy-paste bug—it checks for
clickinstead offireEvent. This bug becomes moot when the function is removed.🔎 Proposed fix
-function hasFireEvent(element: Element): element is Element & { fireEvent(eventType: string): void } { - return typeof (element as any).click === 'function' -} -
102-102: Update comment to remove IE reference.The comment mentions IE behavior, but the click workaround on line 104 is primarily for jQuery's toggle behavior (fixed in jQuery 1.9 for checkboxes but not radio buttons). The IE-specific part of this comment is outdated.
🔎 Proposed fix
// For click events on checkboxes and radio buttons, jQuery toggles the element checked state *after* the // event handler runs instead of *before*. (This was fixed in 1.9 for checkboxes but not for radio buttons.) - // IE doesn't change the checked state when you trigger the click event using "fireEvent". // In both cases, we'll use the click method instead.
135-136: Remove IE-specific fireEvent fallback.The
fireEventmethod is specific to Internet Explorer. Since this PR removes IE support, this fallback path should be deleted.🔎 Proposed fix
} else if (useClickWorkaround && hasClick(element)) { element.click() - } else if (hasFireEvent(element)) { - element.fireEvent('on' + eventType) } else { throw new Error("Browser doesn't support triggering events") }
🧹 Nitpick comments (7)
packages/binding.foreach/spec/eachBehavior.ts (1)
249-249: Suggestion: Verify timeout consistency across related test suites.Since this file applies timeouts to three test suites (observable array changes, DOM move capabilities, focus), consider auditing other test files (e.g.,
packages/binding.template/spec/foreachBehaviors.ts,packages/binding.core/spec/suites) to ensure similar DOM-heavy tests also have appropriate timeout adjustments if needed.Also applies to: 449-449, 1013-1013
packages/utils/src/dom/html.ts (1)
59-59: Consider acting on this TODO in a follow-up.Given this PR's goal of removing IE support entirely, the jQuery and simple parsing fallbacks may no longer be necessary. If the project now targets only modern browsers with native
<template>support, these legacy paths could be removed to simplify the codebase further.Would you like me to verify template tag support across target browsers or help identify remaining references to these legacy parsing methods?
packages/utils/src/dom/event.ts (1)
83-83: Update error message after removing IE support.After removing the
attachEventfallback, the error message should be simplified to only mentionaddEventListener.🔎 Proposed fix
} else { - throw new Error("Browser doesn't support addEventListener or attachEvent") + throw new Error("Browser doesn't support addEventListener") }packages/binding.core/src/textInput.ts (2)
144-150: Consider adding consistency check foroperaVersion.The export logic correctly handles legacy browsers, but there's a minor style inconsistency. Lines 144 and 148 use
&&to verify the version variable exists before comparison, while line 146 directly comparesoperaVersion < 11without the guard. Though functionally correct (sinceundefined < 11andNaN < 11both evaluate tofalse), adding the guard improves consistency and clarity.🔎 Proposed consistency improvement
export const textInput = safariVersion && safariVersion < 5 ? TextInputLegacySafari - : operaVersion < 11 + : operaVersion && operaVersion < 11 ? TextInputLegacyOpera : firefoxVersion && firefoxVersion < 4 ? TextInputLegacyFirefox : TextInput
50-50: Update comment to reflect IE removal.The comment still mentions IE, which is inconsistent with the PR's removal of IE support. Consider updating it to only reference browsers that are still supported.
🔎 Proposed comment update
- // blur: To deal with browsers that don't notify any kind of event for some changes (IE, Safari, etc.) + // blur: To deal with browsers that don't notify any kind of event for some changes (Safari, etc.)packages/binding.core/spec/textInputBehaviors.ts (1)
218-223: Update test description and comment to remove IE references.The test description (line 218) and comment (line 223) still mention IE-specific behavior that has been removed. These should be updated to reflect that the test now only covers modern browser behavior.
🔎 Proposed documentation updates
- it('Should update observable on input event (on supported browsers) or propertychange event (on old IE)', function () { + it('Should update observable on input event', function () { const myObservable = observable(123) testNode.innerHTML = "<input data-bind='textInput: someProp' />" applyBindings({ someProp: myObservable }, testNode) expect((testNode.children[0] as HTMLInputElement).value).toEqual('123') - ;(testNode.children[0] as HTMLInputElement).value = 'some user-entered value' // setting the value triggers the propertychange event on IE + ;(testNode.children[0] as HTMLInputElement).value = 'some user-entered value' triggerEvent(testNode.children[0], 'input')packages/utils/src/dom/data.ts (1)
21-39: LGTM! Consider simplifying themodernwrapper.The change correctly removes IE-specific branching. Since
modernis now the only implementation, you could optionally simplify by removing the wrapper object and defininggetDataForNodeandclearas direct functions.🔎 Optional refactor to remove the wrapper object
-const modern = { - getDataForNode(node: Node, createIfNotFound: boolean) { - let dataForNode = node[dataStoreSymbol] - if (!dataForNode && createIfNotFound) { - dataForNode = node[dataStoreSymbol] = {} - } - return dataForNode - }, - - clear(node: Node) { - if (node[dataStoreSymbol]) { - delete node[dataStoreSymbol] - return true - } - return false - } -} - -const { getDataForNode, clear } = modern +function getDataForNode(node: Node, createIfNotFound: boolean) { + let dataForNode = node[dataStoreSymbol] + if (!dataForNode && createIfNotFound) { + dataForNode = node[dataStoreSymbol] = {} + } + return dataForNode +} + +function clear(node: Node) { + if (node[dataStoreSymbol]) { + delete node[dataStoreSymbol] + return true + } + return false +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
global.d.tspackages/bind/spec/bindingAttributeBehaviors.tspackages/binding.core/spec/hasfocusBehaviors.tspackages/binding.core/spec/textInputBehaviors.tspackages/binding.core/spec/valueBehaviors.tspackages/binding.core/src/attr.tspackages/binding.core/src/options.tspackages/binding.core/src/textInput.tspackages/binding.core/src/uniqueName.tspackages/binding.core/src/value.tspackages/binding.foreach/spec/eachBehavior.tspackages/binding.template/spec/foreachBehaviors.tspackages/binding.template/src/nativeTemplateEngine.tspackages/provider.component/spec/customElementBehaviors.tspackages/utils/helpers/jasmine-13-helper.tspackages/utils/spec/taskBehaviors.tspackages/utils/src/bind-shim.tspackages/utils/src/dom/data.tspackages/utils/src/dom/event.tspackages/utils/src/dom/fixes.tspackages/utils/src/dom/html.tspackages/utils/src/dom/manipulation.tspackages/utils/src/ie.tspackages/utils/src/index.tspackages/utils/src/tasks.ts
💤 Files with no reviewable changes (8)
- packages/utils/src/index.ts
- packages/binding.core/src/options.ts
- packages/utils/src/tasks.ts
- packages/utils/src/ie.ts
- global.d.ts
- packages/utils/src/bind-shim.ts
- packages/utils/src/dom/manipulation.ts
- packages/bind/spec/bindingAttributeBehaviors.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/binding.core/spec/textInputBehaviors.ts (1)
packages/utils/src/dom/event.ts (1)
triggerEvent(95-140)
packages/binding.core/spec/valueBehaviors.ts (1)
packages/utils/src/dom/event.ts (1)
triggerEvent(95-140)
packages/utils/src/dom/html.ts (2)
packages/binding.core/src/options.ts (1)
options(17-197)packages/utils/src/array.ts (1)
makeArray(93-95)
🪛 ast-grep (0.40.3)
packages/provider.component/spec/customElementBehaviors.ts
[warning] 73-73: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: testNode.innerHTML = initialMarkup
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 73-73: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: testNode.innerHTML = initialMarkup
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
packages/binding.core/spec/valueBehaviors.ts
[warning] 298-298: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: testNode.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 298-298: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: testNode.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
packages/utils/src/dom/html.ts
[warning] 18-18: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: div.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 18-18: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: div.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 Biome (2.1.2)
packages/utils/helpers/jasmine-13-helper.ts
[error] 11-11: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testheadless (24, test-headless-ff)
🔇 Additional comments (18)
packages/binding.foreach/spec/eachBehavior.ts (1)
249-249: LGTM! Test timeout additions for Karma-Firefox compatibility.The three
this.timeout(5000)additions are correctly placed and scoped to describe blocks containing DOM-intensive operations (observable array mutations, DOM reordering, and focus handling). The 5-second timeout is reasonable and aligns with the PR objective of removing IE-specific code that may have affected test performance characteristics in the Karma-Firefox environment.Also applies to: 449-449, 1013-1013
packages/binding.template/src/nativeTemplateEngine.ts (1)
1-1: LGTM! Cleanup of unused import.The removal of the
ieVersionimport aligns with the PR's objective to eliminate IE-specific code. SinceieVersionwas never used in this file, this is a straightforward cleanup with no functional impact.packages/binding.template/spec/foreachBehaviors.ts (2)
762-770: LGTM! IE-specific guard removed.The test now runs unconditionally for all browsers. The previous guard that checked for IE shiv or jQuery 1.7+/Modernizr support is no longer necessary, as HTML5 elements are universally supported in modern browsers.
772-783: LGTM! Consistent with the modernization effort.Removing the environment-specific guard ensures this test runs unconditionally, aligning with the removal of IE-specific workarounds throughout the codebase.
packages/utils/helpers/jasmine-13-helper.ts (1)
11-11: LGTM! Removal of IE version detection from test helpers.The removal of
ieVersionfrom the imports and its corresponding assignment to thejasmineobject eliminates IE-specific test infrastructure, consistent with the PR's objective.packages/utils/spec/taskBehaviors.ts (1)
3-3: LGTM! Standardized test timing across browsers.The removal of
ieVersionand the associated conditionalbeforeEachblock that added a 100ms delay for IE9 simplifies the test setup. Modern browsers no longer require this timing workaround.packages/provider.component/spec/customElementBehaviors.ts (1)
65-85: LGTM! Test modernization complete.The removal of the environment-specific guard ensures this test runs unconditionally across all browsers. The test flow has been simplified with the unregister hook properly placed at the beginning, and the test logic executing without browser-specific branching.
packages/utils/src/dom/fixes.ts (1)
55-57: LGTM! Clean simplification to standard DOM API.The direct assignment using
optionNode.selected = isSelectedis the correct modern approach and eliminates the need for IE-specific workarounds. This simplification improves code maintainability while maintaining the same function signature.packages/utils/src/dom/html.ts (3)
9-11: LGTM! Clean feature detection for template tag support.The simplified check correctly tests for HTML5
<template>support using the standard approach.
13-21: Excellent simplification! The IE-era complexity is no longer needed.The removal of table-wrapping logic and cross-browser shims in favor of a straightforward
div.innerHTMLapproach is appropriate given that:
- Template-based parsing (preferred path) handles complex elements natively
- jQuery (second fallback) handles arbitrary HTML fragments
- This simple parser is now a last-resort legacy fallback
Note on static analysis warnings: The innerHTML warnings flagged by static analysis tools are false positives. All HTML is sanitized via
validateHTMLInput()at line 54 before reaching this function, which callsoptions.sanitizeHtmlTemplate()at line 82.
23-30: LGTM! Clean implementation of template-based parsing.The straightforward use of
HTMLTemplateElement.contentis the modern, standards-based approach and correctly serves as the preferred parsing method.packages/utils/src/dom/event.ts (1)
66-69: Verify simplified event registration logic.The condition changes look reasonable for removing IE-specific checks. However, this assumes the IE fallback code below (lines 70-82) will also be removed.
packages/binding.core/src/uniqueName.ts (1)
5-5: LGTM! Clean removal of IE-specific wrapper.Direct assignment to
element.nameis the standard approach and works correctly in all modern browsers.packages/binding.core/src/attr.ts (1)
1-1: LGTM! Consistent removal of IE-specific wrapper.The change from
setElementNameto direct property assignment aligns with modern browser standards and matches the approach taken inuniqueName.ts.Also applies to: 40-40
packages/binding.core/spec/hasfocusBehaviors.ts (1)
1-1: LGTM! Test modernization complete.Removal of the
ieVersionimport and IE8-specific timing workaround appropriately modernizes the test suite.packages/binding.core/src/textInput.ts (1)
1-1: LGTM! IE version import successfully removed.The removal of
ieVersionfrom imports aligns with the PR objective and is consistent with the elimination of IE-specific code paths throughout the file.packages/binding.core/spec/textInputBehaviors.ts (1)
224-226: LGTM! Test logic correctly simplified for modern browsers.The test now directly triggers the
inputevent and immediately expects the observable to update, which is the correct behavior for modern browsers without IE-specificpropertychangeevent handling.packages/utils/src/dom/data.ts (1)
16-20: Comment reasoning is still valid—WeakMap has unresolved cross-window issues in modern browsers.While the mention of IE 11 and Edge is historical, the underlying issue persists: WeakMap cannot reliably store DOM nodes as keys across windows/iframes in modern browsers due to object-identity isolation between JavaScript realms. The comment accurately explains why the current Symbol-based approach was chosen and remains appropriate today. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/utils/src/dom/virtualElements.ts (1)
242-248: Consider updating the outdated IE reference in comment.Since this PR removes IE9 and earlier support, the comment referencing "IE <= 8 or IE 9 quirks mode" is now outdated. The workaround function itself may still be valuable for handling HTML parsing quirks, but the comment could be updated to reflect current browser support.
🔎 Suggested comment update
export function normaliseVirtualElementDomStructure(elementVerified) { // Workaround for https://github.com/SteveSanderson/knockout/issues/155 - // (IE <= 8 or IE 9 quirks mode parses your HTML weirdly, treating closing </li> tags as if they don't exist, thereby moving comment nodes - // that are direct descendants of <ul> into the preceding <li>) + // (Some browsers/quirks modes parse HTML weirdly, treating closing </li> tags as if they don't exist, thereby moving comment nodes + // that are direct descendants of <ul> into the preceding <li>) if (!htmlTagsWithOptionallyClosingChildren[tagNameLower(elementVerified)]) { return }packages/binding.core/src/value.ts (1)
64-72: Consider clarifying the comment for accuracy.The user-agent regex pattern correctly detects IE browsers, but the comment states "only if IE10 or IE11" while the implementation actually matches any IE version (the
MSIEpattern matches IE10 and earlier, whilerv:matches IE11).Based on learnings, IE10/11 support is intentionally retained. If IE9 and earlier are no longer a concern in practice, the current implementation is acceptable.
Optional: Update comment to reflect actual behavior
- && window.navigator.userAgent.match(/MSIE ([^ ;]+)|rv:([^ )]+)/) //only if IE10 or IE11 + && window.navigator.userAgent.match(/MSIE ([^ ;]+)|rv:([^ )]+)/) //IE detection (primarily for IE10/11)packages/utils/src/dom/event.ts (2)
54-60: Update error message to reflect removed IE support.The logic correctly simplifies to jQuery → addEventListener → error, which works for IE9+. However, the error message on line 59 still mentions
attachEvent, which is no longer attempted.🔎 Proposed fix
} else { - throw new Error("Browser doesn't support addEventListener or attachEvent") + throw new Error("Browser doesn't support addEventListener") }
Verify IE10/11 compatibility aligns with PR scope.
Based on learnings, IE10/11 support is retained in
value.tsfor autocomplete workarounds. The changes here removeattachEvent(IE8 and below), which is consistent with dropping IE9 support while keeping IE10/11. Please confirm this aligns with the intended browser support matrix.
67-107: LGTM: Event triggering logic correctly preserved.The logic flow is correct:
- jQuery trigger (when available and applicable)
- Modern createEvent/dispatchEvent (IE9+)
- Click workaround for checkboxes/radio buttons
- Error fallback
The click workaround for checkable elements is appropriately retained to handle jQuery's quirk with toggling checked state.
Consider clarifying the fallback error message.
The error on line 106 should rarely be reached in supported browsers. Consider making it more specific about what's missing.
🔎 Optional improvement
} else { - throw new Error("Browser doesn't support triggering events") + throw new Error("Unable to trigger events: missing createEvent/dispatchEvent support") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/binding.core/spec/textInputBehaviors.tspackages/binding.core/spec/valueBehaviors.tspackages/binding.core/src/textInput.tspackages/binding.core/src/value.tspackages/utils/src/dom/data.tspackages/utils/src/dom/event.tspackages/utils/src/dom/virtualElements.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/binding.core/spec/textInputBehaviors.ts
- packages/utils/src/dom/data.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: phillipc
Repo: knockout/tko PR: 233
File: packages/binding.core/src/value.ts:64-72
Timestamp: 2026-01-03T18:06:06.239Z
Learning: In packages/binding.core/src/value.ts, the ieAutoCompleteHackNeeded getter and related propertyChange/focus/blur event handlers for IE10/11 autocomplete workarounds should be kept, as IE10/11 support is intentionally retained even though IE9 support has been removed.
📚 Learning: 2026-01-03T18:06:06.239Z
Learnt from: phillipc
Repo: knockout/tko PR: 233
File: packages/binding.core/src/value.ts:64-72
Timestamp: 2026-01-03T18:06:06.239Z
Learning: In packages/binding.core/src/value.ts, the ieAutoCompleteHackNeeded getter and related propertyChange/focus/blur event handlers for IE10/11 autocomplete workarounds should be kept, as IE10/11 support is intentionally retained even though IE9 support has been removed.
Applied to files:
packages/binding.core/src/value.tspackages/utils/src/dom/virtualElements.tspackages/utils/src/dom/event.tspackages/binding.core/src/textInput.tspackages/binding.core/spec/valueBehaviors.ts
🧬 Code graph analysis (1)
packages/utils/src/dom/event.ts (1)
packages/utils/src/options.ts (2)
jQuery(52-55)jQuery(62-70)
🪛 ast-grep (0.40.3)
packages/binding.core/spec/valueBehaviors.ts
[warning] 298-298: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: testNode.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 298-298: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: testNode.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🔇 Additional comments (7)
packages/utils/src/dom/virtualElements.ts (3)
20-21: Clean simplification of regex exports.The unified regex patterns correctly match Knockout virtual element comment syntax and eliminate the need for the previous
commentNodesHaveTextPropertyconditional branching.
24-30: LGTM!Using
node.nodeValuedirectly is the standard DOM API for accessing comment node content. The regextest()method handles null/undefined gracefully, returning false.
237-240: LGTM!The direct
nodeValueaccess is consistent with the other changes. Since this function is typically called afterisStartCommentvalidation (as indicated byhasBindingValue = isStartCommenton line 114),nodeValuewill be non-null at this point.packages/binding.core/src/value.ts (1)
1-1: LGTM - ieVersion dependency removed.Removing the
ieVersionimport aligns with the PR objective to eliminate IE version switches. The code now uses direct user-agent matching where needed.Based on learnings, IE10/11 support is intentionally retained while IE9 compatibility has been removed.
packages/utils/src/dom/event.ts (2)
8-8: LGTM: Clean removal of IE-specific imports.The removal of IE-specific imports (likely
ieVersionand related utilities) is consistent with the PR objective and the remaining code no longer references these symbols.
15-15: Verify the corrected event type mapping.The AI summary indicates this line contains a "fixed mapping entry." The current mapping looks correct (UIEvents → keyboard events), but please confirm what was corrected here to ensure the fix is accurate.
packages/binding.core/src/textInput.ts (1)
144-163: LGTM: IE detection simplified while retaining IE10/11 support.The code correctly simplifies IE version detection from separate IE8/IE9/IE10+ branches to a single
ieVersion <= 11check, while maintaining necessary IE-specific event handling through theTextInputIEclass. This aligns with the retrieved learning indicating IE10/11 support should be retained.Based on learnings, IE10/11 autocomplete workarounds are intentionally kept even as older IE version-specific code is removed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
13-13: Clarify the scope of IE removal in the changelog entry.The changelog states "remove IE9 switches from TKO," which is appropriately specific based on the learnings that IE10/11 support is being retained. However, the AI summary and PR description use broader language ("Internet Explorer–specific detection, shims, workarounds...") that doesn't clarify this distinction.
To prevent confusion for future developers reviewing release notes, consider updating the changelog entry to be more explicit about what remains:
-- #233 remove IE9 switches from TKO @phillipc +- #233 remove IE9 and earlier switches from TKO (IE10/11 support retained) @phillipcThis ensures the changelog accurately reflects that IE10/11 compatibility code (e.g.,
ieAutoCompleteHackNeededworkarounds) is intentionally preserved.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdpackages/binding.core/spec/valueBehaviors.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: phillipc
Repo: knockout/tko PR: 233
File: packages/binding.core/src/value.ts:64-72
Timestamp: 2026-01-03T18:06:10.239Z
Learning: In packages/binding.core/src/value.ts, the ieAutoCompleteHackNeeded getter and related propertyChange/focus/blur event handlers for IE10/11 autocomplete workarounds should be kept, as IE10/11 support is intentionally retained even though IE9 support has been removed.
📚 Learning: 2026-01-03T18:06:10.239Z
Learnt from: phillipc
Repo: knockout/tko PR: 233
File: packages/binding.core/src/value.ts:64-72
Timestamp: 2026-01-03T18:06:10.239Z
Learning: In packages/binding.core/src/value.ts, the ieAutoCompleteHackNeeded getter and related propertyChange/focus/blur event handlers for IE10/11 autocomplete workarounds should be kept, as IE10/11 support is intentionally retained even though IE9 support has been removed.
Applied to files:
packages/binding.core/spec/valueBehaviors.ts
🧬 Code graph analysis (1)
packages/binding.core/spec/valueBehaviors.ts (3)
packages/observable/src/observable.ts (1)
observable(87-121)packages/bind/src/applyBindings.ts (1)
applyBindings(483-502)packages/utils/src/dom/event.ts (1)
triggerEvent(67-108)
🪛 ast-grep (0.40.3)
packages/binding.core/spec/valueBehaviors.ts
[warning] 297-297: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: testNode.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 297-297: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: testNode.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testheadless (24, test-headless-ff)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @COVERAGE.md:
- Around line 4-6: Remove the excessive trailing spaces from each row in the
COVERAGE.md table and ensure each row has the same number of pipe-separated
columns as the header (including the final "Uncovered Line #s" column); also
align the separator row (the dashes/pipe row) to match the column count so the
table renders correctly in MarkdownLint.
In @README.md:
- Around line 100-103: The README contains an inconsistent bundle size: the
header text says "~125KB" while the image caption reads "130KB"; update the
image caption to match the header (change "Bundled only 130KB" to "Bundled only
~125KB") or remove the specific size from the alt text (e.g., "Bundle analysis")
so both lines are consistent; modify the markdown around the header and the
image alt text to keep them in sync.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/esbuild_analyzer.pngis excluded by!**/*.png
📒 Files selected for processing (2)
COVERAGE.mdREADME.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: phillipc
Repo: knockout/tko PR: 233
File: packages/binding.core/src/value.ts:64-72
Timestamp: 2026-01-03T18:06:10.239Z
Learning: In packages/binding.core/src/value.ts, the ieAutoCompleteHackNeeded getter and related propertyChange/focus/blur event handlers for IE10/11 autocomplete workarounds should be kept, as IE10/11 support is intentionally retained even though IE9 support has been removed.
🪛 markdownlint-cli2 (0.18.1)
COVERAGE.md
6-6: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
6-6: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
7-7: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
7-7: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
8-8: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
8-8: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
9-9: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
9-9: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
11-11: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
11-11: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
12-12: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
12-12: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
28-28: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
28-28: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
31-31: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
31-31: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
32-32: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
32-32: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
34-34: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
34-34: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
35-35: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
35-35: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
37-37: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
37-37: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
38-38: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
38-38: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
39-39: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
39-39: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
41-41: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
41-41: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
44-44: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
44-44: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
46-46: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
46-46: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
47-47: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
47-47: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
59-59: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
59-59: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
60-60: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
60-60: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
61-61: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
61-61: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
79-79: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
79-79: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
107-107: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
107-107: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
111-111: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
111-111: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
113-113: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
113-113: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
152-152: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
152-152: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
154-154: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
154-154: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
158-158: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
158-158: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
165-165: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
165-165: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
169-169: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
169-169: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
175-175: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
175-175: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
🔇 Additional comments (1)
COVERAGE.md (1)
2-6: Coverage metrics updated to reflect IE removal refactoring.The coverage report date has been updated to 2026-01-10, and metrics across multiple modules have been recalculated. The changes appear consistent with the IE removal PR objectives—some modules show improved coverage (lifecycle, utils modules), while others show changes reflecting removed IE-specific code paths (binding.core, binding.template, binding.core.dist).
Spot-check of notable changes:
packages/binding.core/dist(line 28): metrics adjusted, with descendantsComplete.js showing 50% coverage (line 33)packages/binding.template/dist(line 59): coverage slightly improvedpackages/utils/dist/dom(line 165): various coverage changes reflecting DOM API simplificationspackages/lifecycle/src(line 79): improved to 91.66% statementsThese metrics align with the removal of IE-specific branching and simplification of modern APIs.
| File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s | ||
| --------------------------------------|---------|----------|---------|---------|-------------------- | ||
| All files | 88.21 | 80.71 | 84.94 | 88.15 | | ||
| All files | 89.73 | 83.31 | 86.25 | 89.77 | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's view the COVERAGE.md file to see its current state
cat -n COVERAGE.mdRepository: knockout/tko
Length of output: 18804
🏁 Script executed:
#!/bin/bash
# Check the actual number of pipes per row in the table
head -20 COVERAGE.md | tail -15 | sed 's/[^|]//g' | awk '{print length}' | sort | uniq -cRepository: knockout/tko
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Check if markdownlint is available and run it on COVERAGE.md
if command -v markdownlint &> /dev/null; then
markdownlint COVERAGE.md 2>&1 || true
else
# Try with npm/node if available
if command -v npx &> /dev/null; then
npx markdownlint-cli COVERAGE.md 2>&1 || npx markdownlint COVERAGE.md 2>&1 || echo "markdownlint not available"
else
echo "markdownlint not available"
fi
fiRepository: knockout/tko
Length of output: 50368
Remove trailing whitespace and fix markdown table alignment.
The table has formatting issues flagged by markdownlint: excessive trailing spaces at the end of most rows (typically 20 spaces) cause table-pipe-style violations and column count mismatches. Remove trailing whitespace from all table rows to restore proper formatting.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
6-6: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
6-6: Table column count
Expected: 6; Actual: 5; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
In @COVERAGE.md around lines 4 - 6, Remove the excessive trailing spaces from
each row in the COVERAGE.md table and ensure each row has the same number of
pipe-separated columns as the header (including the final "Uncovered Line #s"
column); also align the separator row (the dashes/pipe row) to match the column
count so the table renders correctly in MarkdownLint.
There was a problem hiding this comment.
@phillipc Thanks for clarifying! Since COVERAGE.md is auto-generated, you're absolutely right that manual formatting fixes wouldn't be appropriate.
For generated files checked into source control that produce linting violations, the recommended approach is to exclude them from markdown linting. You can do this by:
-
Adding a
.markdownlintignorefile in the repository root:COVERAGE.md -
Or configuring in
.markdownlint-cli2.jsonc(or.yaml/.cjs/.mjs):{ "ignores": ["COVERAGE.md"] }
This will prevent CodeRabbit's markdown linter from flagging formatting issues in auto-generated files going forward.
✏️ Learnings added
Learnt from: phillipc
Repo: knockout/tko PR: 233
File: COVERAGE.md:4-6
Timestamp: 2026-01-10T12:14:23.772Z
Learning: In the knockout/tko repository, COVERAGE.md is an auto-generated file (by the test coverage tool) and should be excluded from markdown linting to avoid flagging formatting issues that cannot be manually addressed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: phillipc
Repo: knockout/tko PR: 231
File: Makefile:29-34
Timestamp: 2025-12-28T20:10:15.056Z
Learning: In the knockout/tko repository, coverage reports should be committed to COVERAGE.md for documentation and revision tracking purposes, rather than being gitignored.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/binding.core/src/value.ts (1)
64-74: Add version gate to exclude IE9 from autocomplete workaround.The
MSIE_REGEXpattern matches IE9, IE10, and IE11 equally, but the PR objective to remove IE9 support means IE9 should be excluded from this workaround. The comment indicates the hack is "primarily for IE10/11", yet the current regex-only check applies it to IE9 as well.Add an explicit version check to exclude IE9:
get ieAutoCompleteHackNeeded() { return ( this.isInput && this.$element.type == 'text' && this.$element.autocomplete != 'off' && (!this.$element.form || this.$element.form.autocomplete != 'off') && MSIE_REGEX.test(window.navigator.userAgent) //IE detection (primarily for IE10/11) + && ieVersion > 9 ) }(Import
ieVersionfromtextInput.tsif needed.)
🤖 Fix all issues with AI agents
In @packages/binding.core/spec/textInputBehaviors.ts:
- Around line 28-62: Remove the IE9 case from the uaList and update the
test/title to match IE10 + IE11: delete the object with agent containing "MSIE
9.0" and version: 9 from uaList, rename the test description string if needed
(currently "User-Agent detection IE10 + IE11"), and keep the existing assertions
that use MSIE_REGEX and ieVersion for the remaining entries so the test only
asserts IE10, IE11 and non-IE agents.
In @packages/binding.core/src/textInput.ts:
- Around line 157-165: The IE branch condition currently uses "ieVersion &&
ieVersion <= 11" which matches IE9 and thus enables TextInputIE for IE9; update
the condition in the selection expression that decides TextInputIE so IE9 is
explicitly excluded (e.g., require ieVersion >= 10 && ieVersion <= 11 or
ieVersion !== 9 && ieVersion <= 11) so that TextInputIE is only used for
IE10/IE11 and IE9 falls through to the non-IE legacy path.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/binding.core/spec/selectedOptionsBehaviors.tspackages/binding.core/spec/textInputBehaviors.tspackages/binding.core/src/textInput.tspackages/binding.core/src/value.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: phillipc
Repo: knockout/tko PR: 233
File: packages/binding.core/src/value.ts:64-72
Timestamp: 2026-01-03T18:06:10.239Z
Learning: In packages/binding.core/src/value.ts, the ieAutoCompleteHackNeeded getter and related propertyChange/focus/blur event handlers for IE10/11 autocomplete workarounds should be kept, as IE10/11 support is intentionally retained even though IE9 support has been removed.
📚 Learning: 2026-01-03T18:06:10.239Z
Learnt from: phillipc
Repo: knockout/tko PR: 233
File: packages/binding.core/src/value.ts:64-72
Timestamp: 2026-01-03T18:06:10.239Z
Learning: In packages/binding.core/src/value.ts, the ieAutoCompleteHackNeeded getter and related propertyChange/focus/blur event handlers for IE10/11 autocomplete workarounds should be kept, as IE10/11 support is intentionally retained even though IE9 support has been removed.
Applied to files:
packages/binding.core/spec/selectedOptionsBehaviors.tspackages/binding.core/src/textInput.tspackages/binding.core/src/value.tspackages/binding.core/spec/textInputBehaviors.ts
🧬 Code graph analysis (2)
packages/binding.core/src/value.ts (1)
packages/binding.core/src/textInput.ts (1)
MSIE_REGEX(7-7)
packages/binding.core/spec/textInputBehaviors.ts (2)
packages/binding.core/src/textInput.ts (1)
MSIE_REGEX(7-7)packages/utils/src/dom/event.ts (1)
triggerEvent(67-108)
🔇 Additional comments (2)
packages/binding.core/spec/selectedOptionsBehaviors.ts (1)
50-52: LGTM! Simplified to modern DOM API.The removal of the IE6-specific fallback in favor of direct
optionElement.selected = stateassignment is consistent with dropping legacy IE support and aligns with modern DOM standards.Also applies to: 72-74, 95-97
packages/binding.core/spec/textInputBehaviors.ts (1)
250-259: LGTM! Simplified to modern event model.The removal of IE-specific conditional logic in favor of consistently triggering the
inputevent simplifies the test and aligns with modern browser event handling. This is consistent with the goal of removing legacy IE-specific code paths.
| it('User-Agent detection IE10 + IE11', function () { | ||
| let uaList = [ | ||
| { agent: 'Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/7.0', version: 9 }, | ||
| { agent: 'Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.2; Trident/6.0)', version: 10 }, | ||
| { agent: 'Mozilla/5.0 (Windows NT 6.3; Trident/7.0; rv:11.0) like Gecko', version: 11 }, | ||
| { | ||
| agent: | ||
| 'Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.71 Safari/537.36 Edge/12.0', | ||
| version: null | ||
| }, | ||
| { | ||
| agent: | ||
| 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2486.0 Safari/537.36 Edge/13.10586', | ||
| version: null | ||
| }, | ||
| { | ||
| agent: | ||
| 'Mozilla/5.0 (Macintosh; Intel Mac OS X 15_7_3) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/26.0 Safari/605.1.15', | ||
| version: null | ||
| } | ||
| ] | ||
|
|
||
| uaList.forEach(userAgent => { | ||
| let match = userAgent['agent'].match(MSIE_REGEX) | ||
| let ieVersion = match && (parseFloat(match[1]) || parseFloat(match[2])) | ||
| expect(ieVersion).toBe(userAgent['version']) | ||
|
|
||
| if (userAgent['version'] == null) { | ||
| expect(match).toBeFalsy() | ||
| } else { | ||
| expect(match).toBeTruthy() | ||
| expect(MSIE_REGEX.test(userAgent['agent'])).toBeTruthy() | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Test includes IE9 despite removal goal.
The test is named "User-Agent detection IE10 + IE11" but includes an IE9 user agent test case at line 30 that expects version: 9. This is inconsistent with:
- The PR objective to remove IE9 support
- The learning that states IE9 support has been removed
- The test name itself, which only mentions IE10 + IE11
🧪 Suggested fix to remove IE9 test case and update test name
- it('User-Agent detection IE10 + IE11', function () {
+ it('User-Agent detection for IE10 and IE11', function () {
let uaList = [
- { agent: 'Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/7.0', version: 9 },
{ agent: 'Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.2; Trident/6.0)', version: 10 },
{ agent: 'Mozilla/5.0 (Windows NT 6.3; Trident/7.0; rv:11.0) like Gecko', version: 11 },Based on learnings, IE9 support was intentionally removed while IE10/11 support is retained.
🤖 Prompt for AI Agents
In @packages/binding.core/spec/textInputBehaviors.ts around lines 28 - 62,
Remove the IE9 case from the uaList and update the test/title to match IE10 +
IE11: delete the object with agent containing "MSIE 9.0" and version: 9 from
uaList, rename the test description string if needed (currently "User-Agent
detection IE10 + IE11"), and keep the existing assertions that use MSIE_REGEX
and ieVersion for the remaining entries so the test only asserts IE10, IE11 and
non-IE agents.
| ieVersion && ieVersion <= 11 | ||
| ? TextInputIE | ||
| : safariVersion && safariVersion < 5 | ||
| ? TextInputLegacySafari | ||
| : operaVersion && operaVersion < 11 | ||
| ? TextInputLegacyOpera | ||
| : firefoxVersion && firefoxVersion < 4 | ||
| ? TextInputLegacyFirefox | ||
| : TextInput |
There was a problem hiding this comment.
Critical: IE9 inadvertently included in version check.
The condition ieVersion && ieVersion <= 11 will evaluate to true for IE9, IE10, and IE11, causing TextInputIE to be used for IE9. This contradicts the PR objective to remove IE9 support and the learning that states "IE10/11 support is intentionally retained even though IE9 support has been removed."
🐛 Proposed fix to exclude IE9
export const textInput =
- ieVersion && ieVersion <= 11
+ ieVersion && ieVersion >= 10 && ieVersion <= 11
? TextInputIE
: safariVersion && safariVersion < 5
? TextInputLegacySafariBased on learnings, IE9 support was intentionally removed while IE10/11 support is retained.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ieVersion && ieVersion <= 11 | |
| ? TextInputIE | |
| : safariVersion && safariVersion < 5 | |
| ? TextInputLegacySafari | |
| : operaVersion && operaVersion < 11 | |
| ? TextInputLegacyOpera | |
| : firefoxVersion && firefoxVersion < 4 | |
| ? TextInputLegacyFirefox | |
| : TextInput | |
| ieVersion && ieVersion >= 10 && ieVersion <= 11 | |
| ? TextInputIE | |
| : safariVersion && safariVersion < 5 | |
| ? TextInputLegacySafari | |
| : operaVersion && operaVersion < 11 | |
| ? TextInputLegacyOpera | |
| : firefoxVersion && firefoxVersion < 4 | |
| ? TextInputLegacyFirefox | |
| : TextInput |
🤖 Prompt for AI Agents
In @packages/binding.core/src/textInput.ts around lines 157 - 165, The IE branch
condition currently uses "ieVersion && ieVersion <= 11" which matches IE9 and
thus enables TextInputIE for IE9; update the condition in the selection
expression that decides TextInputIE so IE9 is explicitly excluded (e.g., require
ieVersion >= 10 && ieVersion <= 11 or ieVersion !== 9 && ieVersion <= 11) so
that TextInputIE is only used for IE10/IE11 and IE9 falls through to the non-IE
legacy path.
1 participant
Around 15 years ago, we had to deal with big differences between browsers and browser-specific CSS rules. It's now 2026, and I've removed the IE-9-Version switches from TKO!
Happy New Year!
Summary by CodeRabbit
Refactor
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.