8000 Do not recycle lastElementChild in display · Issue #1780 · pyscript/pyscript · GitHub
[go: up one dir, main page]
Skip to content

Do not recycle lastElementChild in display #1780

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.

Sign up for GitHub

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

Jump to bottom
Closed
3 tasks done
WebReflection opened this issue Oct 2, 2023 · 3 comments
Closed
3 tasks done

Do not recycle lastElementChild in display #1780

WebReflection opened this issue Oct 2, 2023 · 3 comments
Labels
type: bug Something isn't working

Comments

@WebReflection
Copy link
Contributor
WebReflection commented Oct 2, 2023 •
edited
Loading

Checklist

  • I added a descriptive title
  • I searched for other issues and couldn't find a solution or duplication
  • I already searched in Google and didn't find any good information or help

What happened?

Instead of cleaning up the target container when append=False is used, we use the lastElementChild assuming that's the div we eventually previously created on empty containers.

The issues I see with this approach (this is PyScript classic too IIRC):

  • when <py-script> is a target, it's already a visual container
  • any element as target, is already a visual container
  • because of previous 2 points, it's never been too clear to me why we even need to create a div to append anything, but then all integration tests expect that so there must be a reason - amend probably not
  • when <script type="py"> is used, its target is already a visual container ... so that previous questions apply
  • in no circumstance, when append=False, we should reuse any previous content, as the new content goal is to replace it, whatever it was
  • checking for lastElementChild to then branch out logic when append means element.append(new_content) is also not super clear or useful, neither with new empty nodes, nor with already populated ones
  • there are containers that don't accept div as content at all (<picture> and <video> IIRC and to name a few)

Accordingly, we should (imho) improve the display append attribute story, as right now it's rather causing issues instead and weird edge cases, failing expectations.

What browsers are you seeing the problem on? (if applicable)

No response

Console info

No response

Additional Context

No response
@WebReflection WebReflection added type: bug Something isn't working needs-triage Issue needs triage and removed needs-triage Issue needs triage labels Oct 2, 2023
@WebReflection
Copy link
Contributor Author

I've removed the needs-triage label because we've been discussing this in Discord for a while so that @leriomaggio provided dozen variant of the issue and all of those variants were solved by cleaning up upfront when append=False was used.

@JeffersGlass
Copy link
Contributor
JeffersGlass commented Oct 2, 2023 •
edited
Loading

If i had to guess... I would say that the fact that we're creating a div to append things probably doesn't have a good reason. The code for _write (which underlies display) goes back a loooong way. Actually, all the way back to PyScript Alpha! Here's that code from the very first release:

pyscript/pyscriptjs/src/pyscript.py

Lines 111 to 129 in 5d49c20

def write(element_id, value, append=False, exec_id=0):
"""Writes value to the element with id "element_id"""
console.log(f"APPENDING: {append} ==> {element_id} --> {value}")
if append:
child = document.createElement("div")
element = document.querySelector(f"#{element_id}")
if not element:
return
exec_id = exec_id or element.childElementCount + 1
element_id = child.id = f"{element_id}-{exec_id}"
element.appendChild(child)
element = document.getElementById(element_id)
html, mime_type = format_mime(value)
if mime_type in ("application/javascript", "text/html"):
script_element = document.createRange().createContextualFragment(html)
element.appendChild(script_element)
else:
element.innerHTML = html

As such, I think it would be fair to say that _write has some bad parts that we should feel free to correct.

@WebReflection WebReflection mentioned this issue Oct 2, 2023
Merged
3 tasks
@WebReflection
Copy link
Contributor Author
WebReflection commented Oct 2, 2023 •
edited
Loading

To whom it might concern, the current MR simply takes the easy approach by cleaning up the whole thing before applying any logic.

This should guarantee no breaking changes whatsoever, but I do believe the current display is full of potential improvements.

To track my thoughts:

  • a target that is a special container that doesn't allow div element in it (<picture> or <video> to name a few) would break badly
  • it's not clear why append for a specific container target means append to its latest element child
  • if it was for me, the _write helper would've been 8LOC
def _write(element, value, append=False):
    html, mime_type = _format_mime(value)
    if html == "\\n":
        return
    if not append:
        element.replaceChildren()
    fragment = document.createRange().createContextualFragment(html)
    element.append(fragment)

that's it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@WebReflection @JeffersGlass
0