search

gsilber / WebEZ

0 stars 1 forks source link

Error with @Change decorator for checkbox inputs #17

Closed acbart closed 7 months ago

acbart commented 7 months ago

The @Change decorator does not work correctly with checkbox inputs. The issue comes down with how JS/HTML expects checkboxes to behave:

The idea in HTML is that when you submit a form with checkboxes, the value is either present or not present in the submitted form based on the checkboxes status. The checkbox's value never changes based on its checked status!

Unfortunately, this is a little tricky to fix, but here's my initial ideas:

  1. Do some witchcraft to modify Change to manipulate value into being based on the checked attribute instead of the value attribute. This would require inspecting element's type attribute to see if it is 'checkbox', but I think it could work? Then the values might be "off" and "on"? Or "" and "on" (with "on" being replaced with whatever the value initially is in HTML).
  2. Do something weird to allow ValueEvent to have a checked field in certain circumstances? I'm not even sure this is possible, let alone a good idea.
  3. Allow Change to accept either a ValueEvent or a CheckEvent, and dispatch the correct one via overloading, somehow.
  4. Make a separate Checked event decorator and CheckEvent parameter, with a checked field instead of a value field.
  5. Tell people to never use Change with Checkboxes, just use Click and manage the state yourself (but what about getValue?)

These all have drawbacks. I'd be interested in your thoughts on solving the issue, perhaps there's something clever I'm not seeing. But I think that the first one would be the easiest for students at least.

acbart commented 7 months ago

Just to be clear, this shouldn't be merged yet, it's just to demonstrate the issue and raise the problem. I can raise an issue for this, if you'd like.

gsilber commented 7 months ago

I will try to take a look at some point.

You can certainly do this with @BindAttribute(“cb1”,”checked”) although I also think you can set the value to “on” or “” to check uncheck it.

From: Austin Cory Bart @.> Sent: Sunday, April 14, 2024 11:37 AM To: gsilber/WebEZ @.> Cc: Greg Silber @.>; Assign @.> Subject: Re: [gsilber/WebEZ] Error with @Change decorator for checkbox inputs (PR #17)

Just to be clear, this shouldn't be merged yet, it's just to demonstrate the issue and raise the problem. I can raise an issue for this, if you'd like.

— Reply to this email directly, view it on GitHub https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054100406 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVF3IFAU5NUR4XE4QQBL2DY5KPCXAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGEYDANBQGY . You are receiving this because you were assigned. https://github.com/notifications/beacon/AHVF3IGS4HJL7H2HCUJIKKTY5KPCXA5CNFSM6AAAAABGGHJUF6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT2N4K3M.gif Message ID: @. @.> >

acbart commented 7 months ago

That's the other direction, though, which my other PR handles. This is to bind the event. Unless I misunderstood you.

On Sun, Apr 14, 2024, 3:43 PM Greg Silber @.***> wrote:

I will try to take a look at some point.

You can certainly do this with @BindAttribute(“cb1”,”checked”) although I also think you can set the value to “on” or “” to check uncheck it.

From: Austin Cory Bart @.> Sent: Sunday, April 14, 2024 11:37 AM To: gsilber/WebEZ @.> Cc: Greg Silber @.>; Assign @.> Subject: Re: [gsilber/WebEZ] Error with @Change decorator for checkbox inputs (PR #17)

Just to be clear, this shouldn't be merged yet, it's just to demonstrate the issue and raise the problem. I can raise an issue for this, if you'd like.

— Reply to this email directly, view it on GitHub < https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054100406> , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AHVF3IFAU5NUR4XE4QQBL2DY5KPCXAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGEYDANBQGY> . You are receiving this because you were assigned. < https://github.com/notifications/beacon/AHVF3IGS4HJL7H2HCUJIKKTY5KPCXA5CNFSM6AAAAABGGHJUF6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT2N4K3M.gif> Message ID: @. @.> >

— Reply to this email directly, view it on GitHub https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054165391, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG3BS6APAEHOLE3YOR46J3Y5LL6TAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGE3DKMZZGE . You are receiving this because you authored the thread.Message ID: @.***>

gsilber commented 7 months ago

Ok, I see the issue. It’s the value property is always “on” even if it is unchecked. I will look at fixing this.

As for the list code, the problem is a tad more complex than the solution Jim provided, but it is pointing me in the right direction.

The issue is that on some commands (like splice), the event handler would be fired once for every element in the list, meaning we would redraw the element n times. It would probably work, but it would be horrible.

I am working on some code to do it cleanly that involves using the Proxy, but passing all non-element set functionality through the get accessor and overriding the proxy behavior for those things like push, pop, slice, shift, unshift, etc. and ignoring sets on the length property. It will not likely be implemented in time, but I will try.

From: Austin Cory Bart @.> Sent: Sunday, April 14, 2024 3:55 PM To: gsilber/WebEZ @.> Cc: Greg Silber @.>; Assign @.> Subject: Re: [gsilber/WebEZ] Error with @Change decorator for checkbox inputs (PR #17)

That's the other direction, though, which my other PR handles. This is to bind the event. Unless I misunderstood you.

On Sun, Apr 14, 2024, 3:43 PM Greg Silber @. <mailto:@.> > wrote:

I will try to take a look at some point.

You can certainly do this with @BindAttribute(“cb1”,”checked”) although I also think you can set the value to “on” or “” to check uncheck it.

From: Austin Cory Bart @. <mailto:@.> > Sent: Sunday, April 14, 2024 11:37 AM To: gsilber/WebEZ @. <mailto:@.> > Cc: Greg Silber @. <mailto:@.> >; Assign @. <mailto:@.> > Subject: Re: [gsilber/WebEZ] Error with @Change decorator for checkbox inputs (PR #17)

Just to be clear, this shouldn't be merged yet, it's just to demonstrate the issue and raise the problem. I can raise an issue for this, if you'd like.

— Reply to this email directly, view it on GitHub < https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054100406> , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AHVF3IFAU5NUR4XE4QQBL2DY5KPCXAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGEYDANBQGY> . You are receiving this because you were assigned. < https://github.com/notifications/beacon/AHVF3IGS4HJL7H2HCUJIKKTY5KPCXA5CNFSM6AAAAABGGHJUF6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT2N4K3M.gif> Message ID: @. <mailto:@.> @. <mailto:@.> > >

— Reply to this email directly, view it on GitHub https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054165391, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG3BS6APAEHOLE3YOR46J3Y5LL6TAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGE3DKMZZGE . You are receiving this because you authored the thread.Message ID: @. <mailto:@.> >

— Reply to this email directly, view it on GitHub https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054167925 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVF3IEEJ6SWYFZVQZT3SPDY5LNJNAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGE3DOOJSGU . You are receiving this because you were assigned. https://github.com/notifications/beacon/AHVF3IG3HLILSKIW2E4D4R3Y5LNJNA5CNFSM6AAAAABGGHJUF6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT2OAOXK.gif Message ID: @. @.> >

gsilber commented 7 months ago

I have pushed out a patch. @Change now works correctly on checkboxes. The value will be “on” or the empty string.

Greg

From: Austin Cory Bart @.> Sent: Sunday, April 14, 2024 3:55 PM To: gsilber/WebEZ @.> Cc: Greg Silber @.>; Assign @.> Subject: Re: [gsilber/WebEZ] Error with @Change decorator for checkbox inputs (PR #17)

That's the other direction, though, which my other PR handles. This is to bind the event. Unless I misunderstood you.

On Sun, Apr 14, 2024, 3:43 PM Greg Silber @. <mailto:@.> > wrote:

I will try to take a look at some point.

You can certainly do this with @BindAttribute(“cb1”,”checked”) although I also think you can set the value to “on” or “” to check uncheck it.

From: Austin Cory Bart @. <mailto:@.> > Sent: Sunday, April 14, 2024 11:37 AM To: gsilber/WebEZ @. <mailto:@.> > Cc: Greg Silber @. <mailto:@.> >; Assign @. <mailto:@.> > Subject: Re: [gsilber/WebEZ] Error with @Change decorator for checkbox inputs (PR #17)

Just to be clear, this shouldn't be merged yet, it's just to demonstrate the issue and raise the problem. I can raise an issue for this, if you'd like.

— Reply to this email directly, view it on GitHub < https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054100406> , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AHVF3IFAU5NUR4XE4QQBL2DY5KPCXAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGEYDANBQGY> . You are receiving this because you were assigned. < https://github.com/notifications/beacon/AHVF3IGS4HJL7H2HCUJIKKTY5KPCXA5CNFSM6AAAAABGGHJUF6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT2N4K3M.gif> Message ID: @. <mailto:@.> @. <mailto:@.> > >

— Reply to this email directly, view it on GitHub https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054165391, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG3BS6APAEHOLE3YOR46J3Y5LL6TAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGE3DKMZZGE . You are receiving this because you authored the thread.Message ID: @. <mailto:@.> >

— Reply to this email directly, view it on GitHub https://github.com/gsilber/WebEZ/pull/17#issuecomment-2054167925 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVF3IEEJ6SWYFZVQZT3SPDY5LNJNAVCNFSM6AAAAABGGHJUF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGE3DOOJSGU . You are receiving this because you were assigned. https://github.com/notifications/beacon/AHVF3IG3HLILSKIW2E4D4R3Y5LNJNA5CNFSM6AAAAABGGHJUF6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT2OAOXK.gif Message ID: @. @.> >

gsilber commented 7 months ago

I did not merge this, but I did implement a fix. @Change now works as expected for checkboxes. value of the ValueEvent will be either on or the empty string.

gsilber commented 7 months ago

And yes, I used witchcraft.