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

input.trigger('compositionstart') not work #391

Closed
danranVm opened this issue Feb 16, 2021 · 14 comments · Fixed by #394
Closed

input.trigger('compositionstart') not work #391

danranVm opened this issue Feb 16, 2021 · 14 comments · Fixed by #394
Labels
bug Something isn't working

Comments

@danranVm
Copy link

danranVm commented Feb 16, 2021

The code works in rc0, but fails in rc1.

  test('v-model work', async () => {
    const valueRef = ref('init value')
    const wrapper = mount({
      components: { IxInput },
      template: `<ix-input v-model:value="valueRef" />`,
      setup() {
        return { valueRef }
      },
    })
    const input = wrapper.find('input')

    expect(input.element.value).toBe('init value')

    await input.setValue('input setValue')

    expect(valueRef.value).toBe('input setValue')

    valueRef.value = 'valueRef change'

    await nextTick()

    expect(input.element.value).toBe('valueRef change')

    input.element.value = '使用拼音'
    await input.trigger('compositionstart')

    expect(wrapper.emitted()).toHaveProperty('compositionStart')
    expect(valueRef.value).toBe('valueRef change')

    await input.trigger('input')

    expect(wrapper.emitted()).toHaveProperty('input')
    expect(valueRef.value).toBe('valueRef change')

    await input.trigger('compositionend')

    expect(wrapper.emitted()).toHaveProperty('compositionEnd')
    expect(valueRef.value).toBe('使用拼音')
  })

repo: https://github.com/IduxFE/idux

@lmiller1990 lmiller1990 added the bug Something isn't working label Feb 16, 2021
@lmiller1990
Copy link
Member

Uh oh... regression.

@lmiller1990
Copy link
Member

lmiller1990 commented Feb 16, 2021

Maybe this commit? #354.

Actually I think this is the intended behavior (maybe)? I'll need to look into it. emitted should only capture events emitted to the parent wrapper. @aethr do you have any input?

@aethr
Copy link
Contributor

aethr commented Feb 17, 2021

@lmiller1990 interesting, at first I was inclined to agree, in a Vue app in the browser the events emitted by inner components/elements should not escape the component, and we would expect this to fail:

const input = wrapper.find('input')    
await input.trigger('input')
expect(wrapper.emitted()).toHaveProperty('input')

But this would pass:

const input = wrapper.find('input')    
await input.trigger('input')
expect(input.emitted()).toHaveProperty('input')

However, in this case since the <ix-input> component is the root element and inheritAttr isn't false, would events emitted by <ix-input> be emitted? I'm actually not sure. Would be worth making a repro in the browser to check.

@lmiller1990
Copy link
Member

lmiller1990 commented Feb 17, 2021

Hm. Looking that <ix-input /> it is basically like this:

export const useCommonBindings = () => {
  const { emit } = getCurrentInstance()!
  const onCompositionStart = (evt) => {
    emit('compositionStart', evt)
  }
  return { onCompositionStart }
}
<input @compositionstart="onCompositionStart" /> 
const { onCompositionStart } = useCommonBindings(props, inputConfig, inputRef)

Further reduced:

<input @compositionstart="(evt) => emit('compositionStart', evt)" /> 

But the test does await input.trigger('compositionstart'). I don't think this is valid - trigger is supposed to be simulating user inputs, and a user cannot do compositionStart. trigger only worked with DOM events in Test Utils v1 - so I think your fix is actually valid.

EDIT: compositionStart is actually a real DOM event. I'd expect to be able to capture this in wrapper.emitted() - it seems like a pretty common use case, correct or not. I wonder how VTU v1 works - that should likely be the reference behavior.

EDIT: @danranVm is correct - this appears to be incorrect behavior. The event can be captured in a browser as expected. See this pen. Note to trigger the event you need to enter some text using an input device (I used Japanese input).

I think we need to revert or change how emit works - maybe just a clean revert of #354? Unless I am missing something - perhaps I misunderstood the original PR and what problem is solved.

@aethr
Copy link
Contributor

aethr commented Feb 17, 2021

I see the problem now.

I think the issue is the difference between DOM events and Vue component custom events. DOM events bubble up through parent elements in the DOM until propagation is stopped. Vue events occur on the component and no further unless they're manually re-emitted by the parent.

I made a code sandbox to illustrate: https://codesandbox.io/s/amazing-knuth-hm91e?file=/src/App.vue

In this example a native event (input) can be listened for at the grand-parent App, but custom event update:modelValue doesn't bubble outside of Parent.

#354 doesn't handle native DOM events properly in this regard, I naively assumed the VTU emitted was for component events but obviously it needs to provide native events as well. My bad!

I do think whatever solution VTU adopts needs to treat both event types the way they occur in the browser, Wrapper.emitted() should capture all native events that bubbled out to that component, but only custom events that were emitted by the wrapper's direct component. I'm certain this was the old behaviour as we have an entire test suite that was written against that assumption.

@lmiller1990
Copy link
Member

I think you are correct regarding custom vs native.

Do you think it would be difficult to make this fix? Note we have the dom-event-types lib that may help with thisl

@aethr
Copy link
Contributor

aethr commented Feb 17, 2021

Well, after a brief investigation it appears the Vue devtools hooks only captures custom events.

@aethr
Copy link
Contributor

aethr commented Feb 17, 2021

I'm going to try adding a native event listener to the element of each Wrapper to capture native events. I'll try and have a draft PR up tonight.

@lmiller1990
Copy link
Member

Ohhhh true, why would it capture non custom events? Interesting.

I think we can just do what we did before for native events. See this file. We would need to check if it's an native event or not (using dom-event-types).

The reason we moved to using devtools was to be able to capture events from setup - since the eventMixin is not added until beforeCreate. That said, you cannot possibly emit a DOM event in setup, so I think we can do a combo approach - devtools hook for custom, mixin for native.

Thoughts? If you have something better, by all means go with that. But this approach is definitely confirmed to work for native events.

@aethr
Copy link
Contributor

aethr commented Feb 17, 2021

I made a PR that I think should add native events to emitted but it's pretty ugly, feel free to close if you think the mixin approach is more solid. It's late here so I can't take it any further tonight. :)

@lmiller1990
Copy link
Member

I found a fix - but @danranVm I think you will need to change your assertion to toHaveProperty('compositionstart') when we fix this - native events will bubble, but non native ones won't - the native event is compositionstart, not compositionStart.

I will ping you again when this is fixed and released. Probably rc.2 will be this weekend. Thanks!

@danranVm
Copy link
Author

@lmiller1990 Okay, I will try again in rc.2. Thanks!

@lmiller1990
Copy link
Member

@danranVm it's out now, please try rc.2 Note you will likely need compositionstart not compositionStart for your event name.

@danranVm
Copy link
Author

danranVm commented Mar 2, 2021

@danranVm it's out now, please try rc.2 Note you will likely need compositionstart not compositionStart for your event name.

Tks, after modifying according to your suggestion, it can work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants