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

Custom element fix #2221

Merged
merged 2 commits into from
Sep 20, 2018
Merged

Conversation

dead-claudia
Copy link
Member

@dead-claudia dead-claudia commented Sep 1, 2018

Description

Align custom elements to work just like normal elements, minus our HTML workarounds. Previously, our support has been undocumented and completely untested, but I plan to fix that in a follow-up commit (along with a changelog entry).

Drive-by: add a spy.calls property to ospec that helped me develop the patch. It's technically unnecessary, but it helped me clean up the tests some, and it made the tests a little more concise.

Motivation and Context

Fixes #2220

How Has This Been Tested?

Ran the tests in ospec + Node. Performance changes are within the margin of error, although I made a few size-related optimizations that incidentally reclaimed 33 bytes.

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)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@dead-claudia
Copy link
Member Author

@MithrilJS/collaborators Can I get a review for this real quick?


o.only("when vnode is customElement with property, custom setAttribute not called", function(){
Copy link
Member

Choose a reason for hiding this comment

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

kick only

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooops... 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@StephanHoyer
Copy link
Member

just took a brief look. Seems plausible. Will try to take a closer look in about 8h or so.

May already remove the only :D

@dead-claudia
Copy link
Member Author

@StephanHoyer Was that the only issue with it? (I've removed the offending call.)

@dead-claudia dead-claudia force-pushed the custom-element-fix branch 2 times, most recently from 3f5ec61 to 03320e0 Compare September 19, 2018 05:34
@dead-claudia
Copy link
Member Author

dead-claudia commented Sep 19, 2018

Apologies for the Git mess here...I plan to squash this all. Edit: Not necessary.

@dead-claudia dead-claudia force-pushed the custom-element-fix branch 5 times, most recently from 7e00cb1 to 89fcb34 Compare September 19, 2018 06:21
@dead-claudia
Copy link
Member Author

I fixed the Git mess on my end - the history should be a lot cleaner and less polluted now.

This made it much easier to debug multiple calls while developing this
patch.
- Fix custom elements attribute application to acknowledge that not all
  custom elements operate purely based on attributes. (Plus, those
  blasted things are verbose as heck when you're working with them in
  raw form. It's also not that uncommon for functionality to be exposed
  via property and *not* attribute.)
- Don't memoize the normalized value when we 1. only use it once in each
  branch, and 2. only use it for a few special cases.
- Centralize the "has property key" code, so it's easier to tune and
  read. I also inlined a couple functions while I was at it since they
  were small and only used once.
- Actually test for how attributes are applied to raw DOM elements vs
  when we choose to use keys. When I first developed the patch, it
  silently worked, when I should've been breaking things.
@StephanHoyer
Copy link
Member

lgtm.

Sorry for the spam here. I created a flems to test this but the url was too long to share here.

@porsager is there an easy way to share large flems?

I finally got it working with ionicons.

@porsager
Copy link
Contributor

porsager commented Sep 20, 2018

Unfortunately the best way currently is in gists :P

@dead-claudia dead-claudia merged commit 1ecc30a into MithrilJS:next Sep 20, 2018
@dead-claudia dead-claudia deleted the custom-element-fix branch September 20, 2018 19:08
@dead-claudia
Copy link
Member Author

@StephanHoyer Worst case scenario, you can always hide long stuff with a <details>. GH issues do in fact support it, and I've used it before to hide things to "archive" them if I redo a lengthy, detailed proposal in a bug.

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.

Custom Elements Everywhere compliance
3 participants