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

Fix tiny bugs of setAttr()/updateStyle() #2988

Closed
wants to merge 2 commits into from

Conversation

kfule
Copy link
Contributor

@kfule kfule commented Nov 8, 2024

Fixed a few bugs in attributes and style properties updates that almost never happen.

Description

Motivation and Context

I found a strange behavior in the process of #2985, although it rarely happens (#2985 (comment)). And there seems to be a similar trivial issue in updateAttrs() (flems.)
This could be solved by simply swapping the order of set and removal probably without creating new bugs and perf issues.

Also, I am not very familiar with custom elements, but it seemed to me that the #2799 issue could be solved by simply removing key === "is" from setAttr().

How Has This Been Tested?

  • npm run test including test code fix for custom elements
    • Sorry, but I did not make any changes to domMock because the changes seemed more complicated than the bug fix code.
  • I have also confirmed that the issues are not reproduced in flems above using fixed bundle.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My change requires a documentation update, and I've opened a pull request to update it already:
  • I have read https://mithril.js.org/contributing.html.

Sorry, something went wrong.

@kfule kfule requested a review from a team as a code owner November 8, 2024 10:47
@kfule
Copy link
Contributor Author

kfule commented Nov 8, 2024

I am a little sorry for my frequent pull requests these days.😢
I did not plan for this pull request, but it seemed like an easy bug to fix...

I think these are bugs that almost no one cares about, so I won't hurry you to review or merge it.

@kfule kfule force-pushed the fix-is-attr-style branch from edc6ab8 to 2ae7003 Compare November 8, 2024 11:02
Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still need to fix the diff algorithm (this will have negative perf impact) to compare is values for non-custom element vnodes.

  • If the is attribute disappears, the element needs replaced so it's no longer a custom element.
  • If the is attribute is newly added, the element needs replaced so it's now a custom element.
  • If the is attribute changes, the backing custom element needs to change to be that new type.

In all three cases, it's simpler and more predictable to just remove and re-create.

@dead-claudia dead-claudia mentioned this pull request Nov 8, 2024
8 tasks
@kfule
Copy link
Contributor Author

kfule commented Nov 8, 2024

@dead-claudia
Thanks for the comment. As you commented, this PR is going to have unpredictable behavior for users (even if it is straightforward javascript behavior). I will take some time to think about it some more.

I think I will close this PR for now. There is no hurry at all to fix the other bugs in this PR.

@kfule kfule closed this Nov 8, 2024
@dead-claudia
Copy link
Member

@dead-claudia

Thanks for the comment. As you commented, this PR is going to have unpredictable behavior for users (even if it is straightforward javascript behavior). I will take some time to think about it some more.

I think I will close this PR for now. There is no hurry at all to fix the other bugs in this PR.

@kfule The current behavior isn't any more "predictable". What I suggested was really just a one line fix plus some more (pretty straightforward) tests. Sorry if this wasn't clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants