Input field values sometimes remain about stimulus_reflex HOT 23 CLOSED

@leastbad Sorry for the late reply on this. My team decided to continue with a dual frontend/backend validation scheme, so we are not using SR on this project. :(

For my own sanity, I went ahead and updated SR to 3.2.1 and the problem is resolved. Everything seems to be working perfectly. Thanks for pursuing this, and I'll definitely be using SR on another project soon.

from stimulus_reflex.

Thanks for checking all of those things, Mark. I'm sorry this is involved, but it will make things better for folks to come after you, too.

The reason that I'm hung up on focus is that we have this concept/feature data-reflex-permanent that we use to mark an element to be skipped by morphdom. It's something that devs can manually put on an element that they don't want to be refreshed, like a media element or an ad, maybe Google Analytics.

We also have a mechanism by which we put it on text input elements when they gain focus, and take it off when they lose focus. This is to prevent the server from pushing updates that overwrite the input value while you're typing into it. When it comes to text inputs that have focus, the client has to be the single source of truth.

In reality, this automatic data-reflex-permanent business has been a huge pain, and it's because morphdom is (currently) all-or-nothing; it either updates every aspect of a DOM element or it doesn't. You can't cherry-pick attributes or properties to include or exclude.

I currently have a draft PR that is contingent on me convincing the morphdom maintainers to add an escape hatch so that we can specifically exclude value but otherwise update normally. They have an alternative solution that they are pushing which is, quite frankly, bringing a nuclear bomb to a dance off. We're working through it.

In short: I am concerned that the reflex is being triggered before the data-reflex-permanent is removed from the element. The Safari shadow element thing is new and unfortunate, but I suspect that this is a race condition and that yes, it really is all about the last thing that had focus before the button gets clicked.

Now, if you're telling me that you can set a owner value, then change the name, then delete and it still happens... well, back to the drawing board.

from stimulus_reflex.

FYI it looks promising that we will have a fix pushed for this issue later today!

from stimulus_reflex.

Hey Mark,

First, let me confirm that this is a bug.

• Not crazy

Second, thank you for the exceptionally helpful video. This is an excellent issue filing.

Third: I am guilty of doing most of my development work in Chrome, although I know @hopsoft and others are more diverse. At any rate, it's likely that I would never have caught this.

Okay, a few questions and ideas:

1. When you click the remove button, I assume that on the server you're destroying the child model and creating a brand new one, as it seems like in your application there are always "one or more" owners. Is that a valid assumption?

2. If I'm right so far, can I further assume that what we're seeing is that the SSN is being properly removed but the ownership is not?

3. Is it just ownership? What happens if you fill in the name? What happens if you temporarily change the order of the fields eg swap SSN and ownership? I am having trouble seeing any reason why the SSN is getting cleared out when ownership is not.

4. If you fill in a value in the ownership field, tab off, and then use your console to change the value of the field without giving it focus (or triggering your blur event handler), then click remove... what happens? This is actually the one I'm most curious about.

5. Can you try swapping out blur for focusout events?

6. This shouldn't be necessary, but can you try throwing an id on the div that contains the ownership element?

from stimulus_reflex.

Thank you for the quick response, @leastbad.

To answer your questions:

1. Correct, we're destroying the child record, and the backend always adds a new blank one if none exist. However I have seen similar behavior when I have two child records and destroy only one of them.

2. Correct, in this case SSN is being removed and ownership is not.

3. It is normally ownership that stays when it shouldn't, but I have seen this happen with other fields as well (SSN and first name).

4, 5, and 6: All good ideas; I'll get to them as soon as I am able, hopefully this evening.

from stimulus_reflex.

Updates:

4. In Safari it is not possible to edit the value, as it lives within a construct that Safari creates and won't allow me to change. Maybe this is the root of the problem?

5. Changing from blur to focusout does not change the behavior.

6. The divs containing the ownership elements already have ids.

from stimulus_reflex.

Wow - this is surreal (at least to me, on Chrome). It's surprising that nobody else has reported this earlier. The good news is that I'm close to 100% that yes, this is the root of the problem. The bad news is that this is almost certainly an issue with morphdom, but we have a vested interest in getting it resolved.

Does this happen consistently every time?

You mentioned that this can move around and impact other fields... have you ever seen it happen to >1 on a particular instance?

If you experiment with creating a batch of owners, do you notice any patterns when you remove the first of the list, last of the list or one from the middle?

I'm on a Windows machine and cannot run Safari directly, unfortunately. Does this issue manifest on Firefox?

In general, focusout is preferential to blur, although I agree it's likely the last we'll speak of it regarding this issue.

I'm looking at the DIV which contains the label and input; it does not have an id. I'm asking you to temporarily throw an id="owner_<%=owner.id%>" on there on the off-chance that it gives morphdom an additional reference point.

Speaking of morphdom, can you please verify that you're not seeing any console errors that you don't expect? Anything that might be morphdom trying to change something and failing?

Can you try a process where you use the element inspector to manually delete the input element? Will it let you?

I see the input still has data-reflex-permanent on it. Can you please try to give something else the focus before you click the remove button?

Could you please share the markup used to create the delete button? Is it a BUTTON or a styled DIV? Do you use data-reflex="" or do you have a data-action="click->Owner#remove" calling a method in a Stimulus controller?

What I'm ultimately getting to is that I want you to explicitly pull the focus off the SSN before stimulate() gets called. StimulusReflex will remove data-reflex-permanent from the element in the focusout event. I'm worried that clicking your remove button is leaving the data-reflex-permanent on the input, which will prevent everything from working smoothly. You shouldn't need to do anything special, but your issue might be proof that Safari doesn't remove focus from an input just because you clicked on a div. It should, for what it's worth.

Depending on whether you are using data-reflex or data-action (eg already have a method that you're triggering), you can either use a callback in the controller (something like beforeDelete?) or insert a line of js in your existing method which places the focus on something else before stimulate() is called. Docs are here but I'm also happy to help, I just need more info about how you're using StimulusReflex in this context.

from stimulus_reflex.

OK, lots to address, here goes:

Does this happen consistently every time?

In the animation I included, yes, that's consistent. If I mess with other things (adding owners, filling out other fields) I have occasionally seen things change. But nearly always the problem is with this one field.

You mentioned that this can move around and impact other fields... have you ever seen it happen to >1 on a particular instance?

I have never seen more than one field at a time exhibit this behavior.

If you experiment with creating a batch of owners, do you notice any patterns when you remove the first of the list, last of the list or one from the middle?

Yes, I was just playing with this. If I create multiple owners and:

• delete the first, the second owner's information shows up as expected except that the first owner's ownership percentage remains in the ownership field.
• delete a middle one, everything works as expected
• delete the last one, everything works as expected

So it just seems to be that first child record that causes problems.

I'm on a Windows machine and cannot run Safari directly, unfortunately. Does this issue manifest on Firefox?

Not sure. If I get a chance I'll give it a try.

I'm looking at the DIV which contains the label and input; it does not have an id. I'm asking you to temporarily throw an id="owner_<%=owner.id%>" on there on the off-chance that it gives morphdom an additional reference point.

I assumed you were referring to the div that includes the entire child record.

Speaking of morphdom, can you please verify that you're not seeing any console errors that you don't expect? Anything that might be morphdom trying to change something and failing?

I am not seeing any unexpected console errors that could be coming from morphdom.

Can you try a process where you use the element inspector to manually delete the input element? Will it let you?

Yes, I can delete the entire input element. If I do that and then edit another field (like first name) the ownership input returns (with an empty text field) as soon as the reflex refreshes.

I see the input still has data-reflex-permanent on it. Can you please try to give something else the focus before you click the remove button?

Changing focus before clicking the remove button does not change the behavior, and the element still has data-reflex-permanent on it.

Could you please share the markup used to create the delete button? Is it a BUTTON or a styled DIV? Do you use data-reflex="" or do you have a data-action="click->Owner#remove" calling a method in a Stimulus controller?

Sure:

    <div class="one wide field">
      <label for="owner-remove-field">Remove</label>
      <%= owner_fields.hidden_field :_destroy, :class => "owner-remove-field" %>
      <a class="remove-owner" href="#" data-action="click->section#removeBusinessOwner click->gauge#expire">
        <i class="fas fa-trash-alt"></i>
      </a>
    </div>

What I'm ultimately getting to is that I want you to explicitly pull the focus off the SSN before stimulate() gets called. StimulusReflex will remove data-reflex-permanent from the element in the focusout event. I'm worried that clicking your remove button is leaving the data-reflex-permanent on the input, which will prevent everything from working smoothly. You shouldn't need to do anything special, but your issue might be proof that Safari doesn't remove focus from an input just because you clicked on a div. It should, for what it's worth.

Understood. Seems to me that focus is not the issue though?

Depending on whether you are using data-reflex or data-action (eg already have a method that you're triggering), you can either use a callback in the controller (something like beforeDelete?) or insert a line of js in your existing method which places the focus on something else before stimulate() is called. Docs are here but I'm also happy to help, I just need more info about how you're using StimulusReflex in this context.

Not sure if or how this would help. Please let me know what you think given the additional information here.

from stimulus_reflex.

We also have a mechanism by which we put it on text input elements when they gain focus, and take it off when they lose focus. This is to prevent the server from pushing updates that overwrite the input value while you're typing into it. When it comes to text inputs that have focus, the client has to be the single source of truth.

Yes, I'm familiar with data-reflex-permanent. So the assumption is that by default, if you are using SR, you want to call a reflex action on change in an input field? Personally I'd rather just call it on blur (or focusout). Maybe the automatic data-reflex-permanent feature could be turned off for those of us who don't care about that particular feature?

Now, if you're telling me that you can set a owner value, then change the name, then delete and it still happens... well, back to the drawing board.

That is what I am telling you.

from stimulus_reflex.

Okay, thanks for verifying. I'm looking forward to hearing about the answers to some of the other questions, like whether this ever manifests in FF. I'm also curious if you can make it happen on Safari iOS.

One other thing I'd ask you to check for me: could you set an ownership value, tab off, use the elements inspector to look at the ownership input element and see whether the data-reflex-permanent attribute is still on the element even though it no longer has focus.

If it is still there, then that's a bug we can hunt down. If the attribute isn't gone, manually remove it using the inspector and then hit remove. If the attribute is gone by the time you click that remove button, then I'm triply confused.

Disabling data-reflex-permanent is possible but I'm still very much hoping to make progress with the morphdom team before we spend more energy trying to make the current solution better, as I believe the solution itself is flawed. Making morphdom not wipe out the text value means we can get rid of the automatic permanent-on-focus thing altogether, radically simplifying things in the process.

from stimulus_reflex.

Progress! I've figured out what the data-reflex-permanent attribute is doing and why it is ownership percentage that is usually (but not always) the problem. Here's what is happening.

As I alluded to earlier, I am calling my reflex on blur when the value changes. Input fields have blur->section#validateIfChanged on them. The JS in the section controller looks like this:

    validateIfChanged(event) {
        if (event.target.value !== event.target.defaultValue) {
            this.validateForm()
        }
    }

    validateForm() {
        this.validate(this.serializedParams())
    }

    validate(params) {
        this.stimulate("ValidationsReflex#perform", params)
    }

Clicking around in the input fields, I see the behavior you would expect, which is that data-reflex-permanent is removed from the field that is blurred and placed on the field that gains focus.

But the first time (and only the first time) text is entered in a field and then blurred (which is also the first time the reflex is called), data-reflex-permanent is placed on the field that should gain focus, but that field does not in fact gain focus. Instead I have to hit Tab or click in the field again, at which point things appear to proceed as normal.

Here's the catch: Once that initial glitch happens, data-reflex-permanent is never removed from that field that should have gained focus the first time the reflex was run.

For example, if I load the page and click in the last_name field, enter a value, then hit Tab, I expect the Title field to gain focus, but it does not. But the Title field gets a data-reflex-permanent attribute that is never removed. If I hit Tab again, the Title field gets focus. I then type "President" into the Title field and click to remove the owner, all other fields are updated to blanks, but "President" remains in the Title field.

I have several section controllers governing various sections of the form, and this behavior is consistent for a given controller. So we can observe the same behavior in each section of the form, always resulting in data-reflex-permanent being placed on the field that should have received (but does not actually receive) focus after the first reflex action is executed.

If I manually remove the aberrant data-reflex-permanent attribute, the problem resolves itself.

Here's a gif illustrating the problem:

from stimulus_reflex.

Update: In Chrome, I get the same failure-to-focus behavior on the first tab, but data-reflex-permanent is handled properly and is removed when expected.

from stimulus_reflex.

One more update: In Firefox, the behavior is the same as in Safari, that is to say, the data-reflex-permanent attribute becomes persistent and causes the same problem behavior.

from stimulus_reflex.

Okay, this is progress. It's also a good example of code I wrote that was supposed to be clever that is coming back to haunt me.

If you are curious, look at the logic here to follow along.

Our objective was to support two use cases for data-reflex-permanent: the automatic, focus-driven mechanism we're discussing as well as a manual, intentional placement by devs on arbitrary elements in their DOM. This is intended for things like Google Analytics scripts which should not be morphed.

What we're doing when we place the attribute during a focus event is checking to see if the attribute was already present. Then when focus is lost, we don't remove the attribute if it is "supposed" to be there.

If I had to take a guess, your focusin event is firing twice.

Could you please run the following in your console and tell me what the output looks like?

document.querySelector('#borrower_application_business_owners_attributes_0_ownership_percentage').addEventListener('focusin', () => console.log('focus'))
document.querySelector('#borrower_application_business_owners_attributes_0_ownership_percentage').addEventListener('focusout', () => console.log('blur'))

Two things that are top of mind: other people aren't having this happen, and this is an excellent reason to expedite the morphdom conversation because this mechanism is brittle and unreliable. For what it's worth, I'm sorry that you're dealing with this. It's my fault in a small way.

Is there any reason you can think of that the focus would be happening twice?

from stimulus_reflex.

Console output shows two focus each time I enter and two blur each time I leave that field. I can't think of why that would be. Ideas?

from stimulus_reflex.

No, but we're working on it. I've asked Nate to check in, too, if he's able.

This is currently top of mind, I assure you.

from stimulus_reflex.

If you switch it up so that you're listening for focus and blur instead of focusin and focusout does it do the same thing?

And just for sanity, you're seeing:

focusin
focusin
focusout
focusout

and NOT:

focusin
focusout
focusin
focusout

...right?

from stimulus_reflex.

Yes, that is what I was seeing. Two focus events when tabbing in, then two blur events when tabbing out.

I'm trying this again today, and now I'm only seeing one focus event and one blur event. This is true regardless of whether I listen for focus/blur or focusin/focusout. Not sure what could have changed. But the behavior is the same, that is to say, data-reflex-permanent is still overstaying its welcome.

from stimulus_reflex.

Unfortunately, I'm currently stuck. I don't know why your application is doing what it's doing, when it's not impacting others in the same way.

There are two ways to fix it; one involves morphdom accepting a PR for something that they are lukewarm on, and the other involves me drastically re-architecting how SR works behind the scenes, which I'm willing to do even if it would likely take a few days of concerted effort. I'd essentially have to introduce the concept of a virtual DOM into the picture, and it's safe to say that we want to avoid that if possible.

The only thing I can recommend as an embarrassing stopgap is that you could add the following to a Stimulus controller:

connect() {
  document.querySelector('XXX').addEventListener('focusin', e => {
    const ele = e.target
    setTimeout(() => {
      ele.removeAttribute('data-reflex-permanent')
      ele.reflexPermanent = false
    }, 17)
  })
}

I'm writing this on my phone with no way to test, but you seem savvy so I hope you can see what I'm going for and make it work for you. I'm sorry that it's even necessary at all.

from stimulus_reflex.

What about using a different tag for the automatic ignore versus the manual one? For example, let users place data-reflex-permanent where they don't want anything changed, and use something like data-reflex-ignore for the automatic setting on text input fields. Then tell morphdom to ignore both, but get rid of all data-reflex-ignore settings when blurring away from a text field?

from stimulus_reflex.

We’re in the process of trying hard to pull this functionality out, so adding more of the same isn’t the fix. I tried hard earlier in the thread to explain what’s happening and why, as well as offering some workarounds. You are free to fork locally and experiment if these solutions aren’t manifesting fast enough for you. We’ve literally quadrupled in size this week and i can barely keep up with the notifications coming in, I’m sorry.

from stimulus_reflex.

Here is where things are at regarding this issue:

https://discordapp.com/channels/629472241427415060/683317077423292582/703972444649553940

from stimulus_reflex.

Hey Mark, please verify that today’s new release fixes your issue. Thanks for the collaboration and willingness to try things.

from stimulus_reflex.