Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Check for constructor name of element on bootstrap #1058

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andreaskienast
Copy link

Description

The previous check using instanceof fails in case the Choices.js component is rendered in the top frame while the JavaScript actually using Choices.js is in an iframe.

The conditions are changed to check for constructor.name now, solving the issue.

Due to the rather messy constructor, some quirks like operational chaining and explicit type casts need to be used to make the TypeScript compiling process happy.

This fixes #1057.

Screenshots (if appropriate)

Types of changes

  • Chore (tooling change or documentation change)
  • Refactor (non-breaking change which maintains existing functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • My code follows the code style of this project.
  • I have added new tests for the bug I fixed/the new feature I added.
  • I have modified existing tests for the bug I fixed/the new feature I added.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

The previous check using `instanceof` fails in case the Choices.js
component is rendered in the `top` frame while the JavaScript actually
using Choices.js is in an iframe.

The conditions are changed to check for `constructor.name` now, solving
the issue.

Due to the rather messy constructor, some quirks like operational
chaining and explicit type casts need to be used to make the TypeScript
compiling process happy.

Fixes: Choices-js#1057
@@ -306,7 +306,7 @@ function () {

var passedElement = typeof element === 'string' ? document.querySelector(element) : element;

if (!(passedElement instanceof HTMLInputElement || passedElement instanceof HTMLSelectElement)) {
if (!((passedElement === null || passedElement === void 0 ? void 0 : passedElement.constructor.name) === 'HTMLInputElement' || (passedElement === null || passedElement === void 0 ? void 0 : passedElement.constructor.name) === 'HTMLSelectElement')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is more complex than what you've done elsewhere, does it need to be?

Assuming they can all be the same, let's move this functionality out to a function in utils.ts:

export const isHTMLInputOrSelectElement = (element: HTMLElement): boolean => {
  const constructorName = element?.constructor.name;
  if (constructorName === 'HTMLInputElement' || constructorName === 'HTMLSelectElement') {
    return true;
  }
  return false;
}

public/assets/scripts/choices.js Show resolved Hide resolved
@mtriff mtriff added the changes required Pull request requires changes before it can be merged label Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes required Pull request requires changes before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use Choices if rendered from iframe
2 participants