ShaMan123 / react-math-view

React bindings for MathLive
https://shaman123.github.io/react-math-view/
9 stars 12 forks source link

onChange callback doesn't trigger #6

Open hanshs opened 2 years ago

hanshs commented 2 years ago

Describe the bug onChange callback doesn't trigger

To Reproduce Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/zealous-aryabhata-39tw8
  2. Type something in the field
  3. Check console

Expected behavior Console should print the event

Additional context It seems to work with last version 1.3.1

hanshs commented 2 years ago

Current workaround is to use onContentDidChange

ShaMan123 commented 2 years ago

It seems to work with last version 1.3.1

What about 1.3.2?

Sounds like something from Mathlive

hanshs commented 2 years ago

What about 1.3.2?

Doesn't work with 1.3.2 as seen from the repro sandbox

tech-chieftain commented 2 years ago

Hey @ShaMan123! I'm facing the same issue. I tried toggling onChange even from inside <math-field> itself and still it's not triggered. I'm willing to work on this and help out but it seems that the issue might be from mathlive itself rather than your code. I have also tried to use onContetDidChange, but found it to be commented in the code, any other workarounds for this?

tech-chieftain commented 2 years ago

After digging into the code, it's as if the onChange event is not being fired when mapped to input as seen here, but when I map it to change, despite not being what we want, it runs it as needed. I'm gonna raise an issue in Mathlive and see where this leads us.

ShaMan123 commented 2 years ago

Thanks for this. I suggest you create a repro only with mathlive to reproduce this issue. Mathlive might have changed something internally. Meaning check only the input event. What does the change event fire and when? Because the last version it fired only after the math input was blurred. This is why I attached to input. If it fires on every change I don't mind changing it (will we loose data of the event?) Another thing I would check is what happens if we remove this line: https://github.com/ShaMan123/react-math-view/blob/f20b80c29ea62e5835aaa6767dae8ca3c87dd459/src/utils.tsx#L78 It might be overriding the the previous line.

ShaMan123 commented 2 years ago

Another relevant check would be to test if onInput fires. If it does we can simply call onInput and onChange together from one function and attach that to the input handler.

ShaMan123 commented 2 years ago

Could you mention this issue on the ticket in mathlive?

tech-chieftain commented 2 years ago

So far, I have tried changing onChange: input to onChange: change and as you know it only gets triggered when the element loses focus, but at least it gets triggered. onInput same thing happens to onInput so I don't think that this is an issue on your side. It seems that the input event itself is the culprit.

And thanks for the suggestion, I will create a new repo and tests things out on mathlive itself just to see what happens. Also, I will mention this ticket in mathlive.

Thanks for your support @ShaMan123!

ShaMan123 commented 2 years ago

Great. If a PR is needed I will back you, so ask what you need.

ShaMan123 commented 2 years ago

And a disclosure: there's another repo that abstracts mathlive to react. You can try it as well https://github.com/concludio/react-mathlive

ShaMan123 commented 2 years ago

Try this as well https://github.com/ShaMan123/react-math-view/blob/f20b80c29ea62e5835aaa6767dae8ca3c87dd459/src/utils.tsx#L84 It didn't work previous version, that's why I commented it and attached it in options: https://github.com/ShaMan123/react-math-view/blob/f20b80c29ea62e5835aaa6767dae8ca3c87dd459/src/utils.tsx#L27

Try reversing the comment (or not) and listening to this event

tech-chieftain commented 2 years ago

Thank you @ShaMan123! So I've done a bit of testing on mathlive itself directly on html and css. I used the same copy of mathlive.min.js that exists in my node_modules to be sure that nothing affects this test.

I've added the following code in my html file and input worked flawlessly! It was triggered on every keystroke as expected.

  <math-field id="math" virtual-keyboard-mode=manual>
    x=\frac{-b\pm\sqrt{b^2-4ac}}{2a}
  </math-field>

  <script>
    math.addEventListener('input', (ev) => {
      console.log(ev.target.value);
    });
  </script>

Then I went and added the following code in this file by referencing _ref.

    useEffect(() => {
      console.log(_ref);
      _ref.current?.addEventListener("input", () => {
        console.log("input");
      });

      return () => {
        _ref.current?.removeEventListener("input", () => {});
      };
    }, []);

To my surprise, the above code didn't work! Theoretically it should, but what I noticed is the fact the console logs input once when the page is first loaded!

tech-chieftain commented 2 years ago

Try this as well

https://github.com/ShaMan123/react-math-view/blob/f20b80c29ea62e5835aaa6767dae8ca3c87dd459/src/utils.tsx#L84

It didn't work previous version, that's why I commented it and attached it in options: https://github.com/ShaMan123/react-math-view/blob/f20b80c29ea62e5835aaa6767dae8ca3c87dd459/src/utils.tsx#L27

Try reversing the comment (or not) and listening to this event

I tried working with onContentDidChange but it brought the caret position bug back again since I'm using a controlled component (I'm using a React state to keep the formula)

ShaMan123 commented 2 years ago

This is strange.

Thank you @ShaMan123! So I've done a bit of testing on mathlive itself directly on html and css. I used the same copy of mathlive.min.js that exists in my node_modules to be sure that nothing affects this test.

I've added the following code in my html file and input worked flawlessly! It was triggered on every keystroke as expected.

  <math-field id="math" virtual-keyboard-mode=manual>
    x=\frac{-b\pm\sqrt{b^2-4ac}}{2a}
  </math-field>

  <script>
    math.addEventListener('input', (ev) => {
      console.log(ev.target.value);
    });
  </script>

Then I went and added the following code in this file by referencing _ref.

    useEffect(() => {
      console.log(_ref);
      _ref.current?.addEventListener("input", () => {
        console.log("input");
      });

      return () => {
        _ref.current?.removeEventListener("input", () => {});
      };
    }, []);

To my surprise, the above code didn't work! Theoretically it should, but what I noticed is the fact the console logs input once when the page is first loaded!

Can you attach 2 event listeners to input and make sure both fire?

ShaMan123 commented 2 years ago

Another thing I want to check is how many time this function is called. I suspect something is calling this too many times: https://github.com/ShaMan123/react-math-view/blob/6d073dfedb80ecf63bcd92a002fb49f3eef9c9e0/src/utils.tsx#L194

ShaMan123 commented 2 years ago

Try this as well https://github.com/ShaMan123/react-math-view/blob/f20b80c29ea62e5835aaa6767dae8ca3c87dd459/src/utils.tsx#L84

It didn't work previous version, that's why I commented it and attached it in options: https://github.com/ShaMan123/react-math-view/blob/f20b80c29ea62e5835aaa6767dae8ca3c87dd459/src/utils.tsx#L27

Try reversing the comment (or not) and listening to this event

I tried working with onContentDidChange but it brought the caret position bug back again since I'm using a controlled component (I'm using a React state to keep the formula)

Good, I suppose that's why it's commented out.

ShaMan123 commented 2 years ago

Alright another check. Is your callback wrapped with useCallback?

tech-chieftain commented 2 years ago

Can you attach 2 event listeners to input and make sure both fire?

Both fired once right when the page loaded.

Another thing I want to check is how many time this function is called. I suspect something is calling this too many times:

https://github.com/ShaMan123/react-math-view/blob/6d073dfedb80ecf63bcd92a002fb49f3eef9c9e0/src/utils.tsx#L194

Tested it. It only fires once.

Is your callback wrapped with useCallback?

No it isn't. I haven't touched useCallback since I was trying simply to get the event to trigger. Can you write a quick code with your suggestions if you have the time?

ShaMan123 commented 2 years ago

Can you share some fiddle/codesandbox? It will be easier.

Can you attach 2 event listeners to input and make sure both fire?

Can you do that on the mathlive pure prepo?

tech-chieftain commented 2 years ago

Can you share some fiddle/codesandbox?

https://codesandbox.io/s/stupefied-https-7kr01?file=/src/App.tsx In this codesandbox project, I have added react-math-view v1.3.2 with onChange and onContentDidChange and only onContentDidChange worked. onChange showed the same bug I told you where it gets triggered when the page loads. Then I degraded to v1.3.1 and both worked as intended! I surmise from this that the issue was introduced with version 1.3.2. This really tightens our search.

Can you do that on the mathlive pure prepo?

I've done that to the pure repo and it worked.

ShaMan123 commented 2 years ago

this looks like a major commit https://github.com/ShaMan123/react-math-view/commit/daf92fbfafc7e3fbed28678976fff165a650369a

I would start with useValue

ShaMan123 commented 2 years ago

Or try to revert it altogether

tech-chieftain commented 2 years ago

this looks like a major commit daf92fb

I would start with useValue

Thanks, this was a good pointer! useValue didn't have any issues; however, when I commented useControlledConfig and passed config directly into useUpdateOptions it worked! So the issue was in the useControlledConfig from the beginning.

So far, this means that I can access the value by calling onContentDidChange, but this of course means that the caret position issue will come back. When I tested useControlledConfig, the functions were actually undefined and props was empty. This relates to filterConfig returning an empty object. Maybe this is because onContentDid/WillChange are events and not other props.

I'm looking into the code and will create a PR if I can fix this. Your input would be much appreciated.

ShaMan123 commented 2 years ago

try safeguarding https://github.com/ShaMan123/react-math-view/blob/daf92fbfafc7e3fbed28678976fff165a650369a/src/utils.tsx#L163

- ...props,
+ ...(props || {}),
ShaMan123 commented 2 years ago

try removing useMemo

https://github.com/ShaMan123/react-math-view/blob/daf92fbfafc7e3fbed28678976fff165a650369a/src/index.tsx#L12

ShaMan123 commented 2 years ago

and test this as well https://github.com/ShaMan123/react-math-view/blob/daf92fbfafc7e3fbed28678976fff165a650369a/src/index.tsx#L22 Does it fire?

tech-chieftain commented 2 years ago

try safeguarding

https://github.com/ShaMan123/react-math-view/blob/daf92fbfafc7e3fbed28678976fff165a650369a/src/utils.tsx#L163

- ...props,
+ ...(props || {}),

I don't see how this will work, syntax-wise, inside a function.

try removing useMemo

Nope, nothing happened here.

Does it fire?

Since onChange is not a realy HTMLAttribute this didn't fire; however, testing onInput fire correctly.

xing38 commented 2 years ago

@ShaMan123 @tech-chieftain any updates on this? I can't get it to work as well

ShaMan123 commented 2 years ago

As I mentioned I don't have enough time to dedicate to this project. Does onInput work instead?

coprocoder commented 2 years ago

@ShaMan123, onInput and onChange are still not called. I temporarily solved this problem by calling sender.getValue() (sender's prototype method) via onContentDidChange. This is frankly a crutch. A very temporary solution. I really look forward to when the oninput and onchange methods work as expected.

I changed this souce code line

  onContentDidChange={(sender) => {
    const _value = sender.getValue(); 
    setValue(_value);
  }}

Your library makes a huge contribution. I really hope that this will be fixed and the editing will be adjusted.

ShaMan123 commented 2 years ago

@coprocoder What about updating mathlive? do you try it? Did you look for issues there regarding this problem? Maybe it is react? Did you change versions and test what happens?