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

Select polyfill alt #2335

Open
wants to merge 5 commits into
base: select-polyfill
Choose a base branch
from
Open

Select polyfill alt #2335

wants to merge 5 commits into from

Conversation

developit
Copy link
Member

@developit developit commented Feb 11, 2020

Alternative solution. There are a few properties that exist on <select> elements but not other inputs. This isn't ideal from a performance perspective, but the check only runs on onInput handlers, so the impact should be minimal. Alternatively, we could plumb vnode through prop diffing functions and check vnode.type.

Properties audit:

const select = document.createElement('select');
const others = ['input', 'textarea', 'div', 'option', 'optgroup'].map(x=>document.createElement(x));
others.push(document.createTextNode(''));
for (let i in select) {
  if (!others.some(o => i in o)) {
    console.log(`select.${i} = ${select[i]}.`);
  }
}

/*
// results:
select.options = [object HTMLOptionsCollection]
select.selectedOptions = [object HTMLCollection]
select.selectedIndex = -1.
select.item = function item() { [native code] }
select.namedItem = function namedItem() { [native code] }
select.add = function add() { [native code] }
*/

@github-actions
Copy link

github-actions bot commented Feb 11, 2020

Size Change: +73 B (0%)

Total Size: 38.6 kB

Filename Size Change
dist/preact.js 3.74 kB +15 B (0%)
dist/preact.min.js 3.74 kB +15 B (0%)
dist/preact.module.js 3.76 kB +15 B (0%)
dist/preact.umd.js 3.82 kB +28 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3 kB 0 B
compat/dist/compat.module.js 3.03 kB 0 B
compat/dist/compat.umd.js 3.05 kB 0 B
debug/dist/debug.js 3.08 kB 0 B
debug/dist/debug.module.js 3.07 kB 0 B
debug/dist/debug.umd.js 3.15 kB 0 B
devtools/dist/devtools.js 175 B 0 B
devtools/dist/devtools.module.js 185 B 0 B
devtools/dist/devtools.umd.js 252 B 0 B
hooks/dist/hooks.js 1.05 kB 0 B
hooks/dist/hooks.module.js 1.08 kB 0 B
hooks/dist/hooks.umd.js 1.13 kB 0 B
test-utils/dist/testUtils.js 390 B 0 B
test-utils/dist/testUtils.module.js 392 B 0 B
test-utils/dist/testUtils.umd.js 469 B 0 B

compressed-size-action

@coveralls
Copy link

coveralls commented Feb 11, 2020

Coverage Status

Coverage decreased (-0.8%) to 98.958% when pulling 73198f9 on select-polyfill-alt into d0215a1 on select-polyfill.

@developit developit changed the base branch from select-polyfill to master February 11, 2020 20:12
@developit developit changed the base branch from master to select-polyfill February 11, 2020 20:12
Copy link
Member

@cristianbote cristianbote left a comment

Choose a reason for hiding this comment

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

I like it 😃

// Fix onInput not firing on select/radio/checkbox in Edge <= 18.
if (name === 'input' && (
type === 'select' ||
type === 'input' && /rad|check/.test(props.type)
Copy link
Member

Choose a reason for hiding this comment

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

🤔 are props defined in here? Couldn't find them. Maybe I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to differentiate vnode.type and the input type which is usually in props.type here. As far as I can tell both are needed to pass this check. So we'd need to pass props through the arguments.

// Fix onInput not firing on select/radio/checkbox in Edge <= 18.
if (name === 'input' && (
type === 'select' ||
type === 'input' && /rad|check/.test(props.type)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type === 'input' && /rad|check/.test(props.type)
type === 'input' && /rad|check/.test(type)

I guess @cristianbote is right here :)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking this is what @developit wanted but, since the first condition checks that type is strictly equal to 'input' the next condition will never be true, isn't it? Cause that'll end-up with /rad|check/.test('input') which is not true.

Copy link
Member

Choose a reason for hiding this comment

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

the type in scope right now is the vnode type (html tag / component)

we need props.type which is the type of the input

<input type="radio">
 -----       -----
 |           |
 |           props.type
 vnode.type

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that means that value should be there instead.

Copy link
Member

Choose a reason for hiding this comment

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

if you're talking about the value in scope, no
that's the event listener function

Copy link
Member

Choose a reason for hiding this comment

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

You are right, I've added a solution below.

Comment on lines +102 to +104
if (name === 'input' && (
type === 'select' ||
type === 'input' && /rad|check/.test(props.type)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (name === 'input' && (
type === 'select' ||
type === 'input' && /rad|check/.test(props.type)
if ((name === 'input' || /rad|check/.test(name)) &&
(type === 'select' || type === 'input')

This one makes the tests to pass for radio and checkboxes as well. Is this what you had in mind @developit? 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants