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

Interaction glitch w/ BitWarden #23

Open
c17r opened this issue Aug 19, 2020 · 14 comments
Open

Interaction glitch w/ BitWarden #23

c17r opened this issue Aug 19, 2020 · 14 comments
Assignees
Labels
bug Something isn't working

Comments

@c17r
Copy link

c17r commented Aug 19, 2020

hello Adam, long time no talk!

I was trying out the demo on the website and something BitWarden is doing to the input box, I think added it's own attribute to the html, causes the input to be redrawn empty but if you click the button add it will still load something but not everything.

Video:
django-unicorn-glitch.mov.zip

If I disable BitWarden and reload the page, the interaction works fine.

@adamghill
Copy link
Owner

Hey Christian! Hmm, that's weird. Can you inspect that text input and show me what the element looks like to see what BitWarden might be doing? Maybe I can deal with it relatively easily? I'm also wondering if you see any console errors? I'm trying to spit out useful messages when possible.

@adamghill adamghill self-assigned this Aug 19, 2020
@c17r
Copy link
Author

c17r commented Aug 19, 2020

No console errors. Attaching another video but this still shows the BitWarden attribute
image

django-unicorn-glitch-2.mov.zip

@adamghill
Copy link
Owner

Interesting that I don't have the same issues with 1Password. However, I added the Bitwarden Firefox extension and can replicate what you are seeing. Not sure exactly how to fix, yet.

It does look like Livewire has similar issues with autofills (livewire/livewire#1081) so I'll be watching how they fix with interest to see if there is a clean way to deal with this issue.

@adamghill
Copy link
Owner

I updated the example component to use the new lazy modifier on the model (https://www.django-unicorn.com/documentation/components/templates#lazy) and BitWarden doesn't interfere anymore. This isn't a real solution and more like a work-around, though. I'll keep this issue around to look into it further when I get a chance.

@adamghill adamghill added the bug Something isn't working label Jan 29, 2021
@zain
Copy link

zain commented Apr 29, 2021

@adamghill it looks like livewire fixed the issue in livewire/livewire#1721. Any thoughts about whether their solution can be ported to django-unicorn?

@sharno
Copy link

sharno commented Dec 27, 2021

I'm getting the same exact issue with or without the lazy modifier on the model. As soon as the event fires the input field gets cleared because of BitWarden. Is there a way to fix this or work around it?

@c17r
Copy link
Author

c17r commented Dec 28, 2021

I tried the example that was giving me the issue (https://www.django-unicorn.com/examples/todo) and it works fine now though another example (https://www.django-unicorn.com/examples/search) which doesn't have the lazy modifier still exhibits the odd interaction with Bitwarden

@adamghill
Copy link
Owner

After reading through some Livewire and Bitwarden issues and trying a few attempts to solve for this, I'm not sure I'm any closer to fixing this. I'd love a PR, though if anyone is interested in tackling it.

@c17r
Copy link
Author

c17r commented Dec 30, 2021

Spent some time looking at this tonight. It's not something BitWarden-specific but something the BitWarden extension does (the 1Password extension use to do this as well because that's where BitWarden got it from, see https://github.com/bitwarden/browser/blob/176c14188ca3fe55c88687658dc350e2c31a0226/src/content/autofill.js).

I was able to distill it down to the following repeatable process:

  1. Load /examples/search in the browser in such a way that it'll work properly
  2. Open the console
  3. copy/paste/execute the following javascript:
document.addEventListener('input', function (inputevent) {
  if (inputevent.target.tagName.toLowerCase() === 'input') {
    inputevent.target.dataset['userEdited'] = 'yes'
  }
}, true);
  1. Try to use the search box and now see the broken behavior

I might get a chance to test more tomorrow to confirm but my guess is the changing of the existing DOM before unicorn does the morphdom process is glitching things up.

@sharno
Copy link

sharno commented Dec 30, 2021

My guess is that it's a race condition between Bitwarden and Unicorn. Sometimes the input field would keep the input and behave as expected and at this point these are the attributes on the element:
image

other times the input gets cleared and the attributes on the element are:
image

as you can see the difference is mainly those extra attributes that bitwarden adds and when unicorn manages to update the element before Bitwarden it behaves as expected but when Bitwarden does it first it gets cleared.

Any pointers to where in the code here does the element get updated? Maybe there's a way to make it behave nicely with Bitwarden and other autofillers

@adamghill
Copy link
Owner

Ah, interesting! Yeah, morphdom is probably getting confused with the extra attributes. I had to deal with a similar issue before for error validation around here: https://github.com/adamghill/django-unicorn/blob/master/django_unicorn/static/unicorn/js/messageSender.js#L119-L124. Maybe we could loop through all modelEls and remove some of the extra attributes? It doesn't feel super elegant, but it might work?

@c17r
Copy link
Author

c17r commented Dec 30, 2021

I've done some more testing and 3rd party code that does anything with the input tag before morphdom is called (data element, add a class, add style, etc) makes morphdom rebuild the input box thereby losing the current value (which makes sense because morphdom is creating a new node). So the idea of "...remove some of the extra attributes" isn't going to work.

Oddly, I haven't been able to find a working example of Phoenix Liveview to test this theory but since it uses morphdom I would suspect that it could be suffering the same way.

I did find a way to make things work (for various definitions of "work") by changing the search component template slightly to return back what the current value of the box should be:

<!-- unicorn/templates/unicorn/search.html -->
<div>
    <label>State typeahead</label><br />
    <input type="text" unicorn:model="state" value="{{ state }}" placeholder="State name" id="state-name"></input>

    <p>
        Matching states:

        {% if states %}
        <ul>
        {% for s in states %}
        <li>{{ s }}</li>
        {% endfor %}
        </ul>

        <button unicorn:click="clear_states">Clear result</button>
        {% else %}
        n/a
        {% endif %}
    </p>
</div>

morphdom still re-writes the input box but server side has -- quite correctly -- told the front end what the value of the input should be. And I think this makes sense because the whole point of unicorn (and livewire and liveview) is the "action" happens server side and the server is the source of truth. So the statement of unicorn:model="state" could be translated to HTML that sets the value property on the entity.

Will it break extensions that muck around with the DOM? Sure, but better they be broken then allowing them to break your application.

The only other thing I can think of is unicorn clones the DOM of the component before the call, does a morphdom maneuver with the clone and replace the real DOM with the modified clone ... which, in effect, would be switching over to a virtual DOM process which I'm guessing isn't something you want to do since it will add to the complexity.

@adamghill
Copy link
Owner

I did find a way to make things work (for various definitions of "work") by changing the search component template slightly to return back what the current value of the box should be

I can see why this works, but it does feel sub-optimal and redundant. Or at least it takes away some of the "magic" of Unicorn, especially since it would be needed for anything that might possibly be auto-completed (or would it be necessary for any input that uses unicorn-model?). :(

The only other thing I can think of is unicorn clones the DOM of the component before the call, does a morphdom maneuver with the clone and replace the real DOM with the modified clone

Yeah, I'd love to avoid that complexity if at all possible. However... I might try to see if I could get that to work.

One thing I'm curious about is testing out https://alpinejs.dev/plugins/morph as a replacement to morphdom. I'm not sure it would actually fix this issue without other tweaks, but it might prove useful to follow in the footsteps of Livewire as much as possible just in general.

@c17r
Copy link
Author

c17r commented Jan 2, 2022

I haven't picked through it all yet but I did stand up a Laravel Livewire project and recreated the search:

// Search.php
<?php

namespace App\Http\Livewire;

use Livewire\Component;
use Illuminate\Support\Arr;
use Illuminate\Support\Str;

class Search extends Component
{
    public $state = "";

    private const ALL_STATES = [
        "Alabama",
        // ...snip...
        "Wyoming",
    ];

    public function clear_state()
    {
        $this->state = "";
    }

    public function states()
    {
        if ($this->state === '')
            return [];

        $query = Str::lower($this->state);
        return Arr::where($this::ALL_STATES, fn($v, $k) => Str::startsWith(Str::lower($v), $query));
    }

    public function render()
    {
        return view('livewire.search', [
            'states' => $this->states(),
        ]);
    }
}

// search.blade.php
<div>
    <label>State typeahead</label><br />
    <input type="text" wire:model="state" placeholder="State name" id='state-name'></input>

    <p>
        Matching states:

        @if(count($states) > 0)
            <ul>
                @foreach($states as $state)
                    <li>{{ $state }}</li>
                @endforeach
            </ul>

            <button wire:click="clear_state">Clear Result</button>
        @else
            n/a
        @endif
    </p>
</div>

and it continues to work properly even with the offending 3rd party javascript added to the browser window. It clearly overwrites the DOM -- the troublesome attribute is no longer there -- so they must do a post morphdom value update process.

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

No branches or pull requests

4 participants