Graphite icon indicating copy to clipboard operation
Graphite copied to clipboard

Fix checkbox labels becoming unclickable after first toggle

Open ashishmohapatra240 opened this issue 1 month ago • 7 comments

Fixes #3368

Checkbox labels could only be clicked once, then stopped working because CheckboxId::new() generated random IDs on every layout update. It updated CheckboxInput widgets with new IDs but skipped updating TextLabel widgets.

I added CheckboxId::from_string() to generate stable, hash-based IDs, so each checkbox/label pair now maintains consistent IDs across renders.

https://github.com/user-attachments/assets/00ee985a-00f2-4171-b898-a46c0c6048ad

ashishmohapatra240 avatar Nov 12 '25 06:11 ashishmohapatra240

Thank you for your attempt, but this is not an appropriate solution. The regression needs to be fixed, not worked around by fundamentally changing its approach.

Keavon avatar Nov 12 '25 09:11 Keavon

I did also think about hashing since the random id generation just isn't a a particularly sensible design. What we could do is that we do actually unse a content based hash, which includes its location in the widget tree. Then the ids would be stable which makes the diffing substantially easier and would remove some foot guns

TrueDoctor avatar Nov 13 '25 01:11 TrueDoctor

Happy to implement either approach. Just let me know which direction you'd prefer!

ashishmohapatra240 avatar Nov 13 '25 04:11 ashishmohapatra240

Let's please try to fix the regression for now before switching approaches quite yet.

Keavon avatar Nov 13 '25 04:11 Keavon

I think trying to fix this in the old system would take more effort than implementing a proper fix which would be to not use random ids. The random ids did work fine before had diffing, but combining both is just a very fragile system which is prone to breaking. (And I think we should do this for all widget ids, not just the checkboxes)

TrueDoctor avatar Nov 13 '25 06:11 TrueDoctor

Ok, in that case, I suggest we move back to the issue instead of the PR to discuss those plans.

Keavon avatar Nov 13 '25 08:11 Keavon

Sure @Keavon @TrueDoctor , let's move back to the issue. 😅

ashishmohapatra240 avatar Nov 13 '25 09:11 ashishmohapatra240