Tags td, tr or th ignored by new parseHTML implementation

Created on 5 February 2025, 2 months ago

Problem/Motivation

I have used such command
$ajax_response->addCommand(new ReplaceCommand($selector, '<td>Hello!</td>'));

but in result i see only - "Hello!", without tags

Steps to reproduce

Proposed resolution

from https://stackoverflow.com/a/43112816

var template = document.createElement( 'template' )
template.innerHTML = '<td></td>'
var frag = template.content
console.log( frag.childNodes )   // output => [td]

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

๐Ÿ› Bug report
Status

Active

Version

10.4 โœจ

Component

ajax system

Created by

๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @alexdoma
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone

    Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies โ†’ .

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia anjaliprasannan

    @alexdoma to output with html tags you need to add the

  • ๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma

    @anjaliprasannan
    i have checked your example and get the same results as you.

    But, in your html output additional '

  • ๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma
  • ๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia anjaliprasannan

    Ok I tried with

    $response->addCommand(new ReplaceCommand('#ajax-message', '<table><td>Hello!</td></table>'));

    this removed the additional tr. Are you giving the proper selector to replace?

  • ๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma

    PLease, read whole message i have wrote my test case 2 times.
    I have provided correct test case with an error.

    I don`t want to replace full table like in your case. I want to replace one cell.
    Case for one cell is not working.

    P.S. in your test case you missed closing </tr> thats why you have 2 rows.

  • ๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma

    Replace one cell not working.

    // Not working.
    $ajax_response->addCommand(new ReplaceCommand($selector, '<td>Hello!</td>')); 
    

    Replace one table working.

    // Working.
    $ajax_response->addCommand(new ReplaceCommand($selector, '<table><tr><td>Hello!</td></tr></table>'));
    
    
  • ๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia anjaliprasannan

    yes I understand, I am checking.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia anjaliprasannan

    @alexdoma, Please use this

    $response->addCommand(new HtmlCommand('#replace-me', '<td>Hello</td>'));

    before replace

    After replace

  • ๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma

    @nod_ @quietone
    @anjaliprasannan Thank you for your feedback. But i think you have provided incorrect example.

    Upon encountering this error for the first time, I also came to the conclusion that using HtmlCommand would be the best solution. However, I have a large number of places in my code where I use ReplaceCommand

    The desire to get rid of the jQuery library is commendable, but not at any cost.

    Frankly, I donโ€™t understand why I should rewrite my code because of a bug in the core. I havenโ€™t seen any updates in the documentation for ReplaceCommand indicating that it doesn't work for certain tags.

    I believe the parent issue (#3238870 Refactor (if feasible) use of jquery parseHTML function to use vanillaJS) was resolved incorrectly and it needs to be reopened. In the meantime, the changes should be reverted.

  • ๐Ÿ‡ง๐Ÿ‡พBelarus alexdoma

    any updates?

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia anjaliprasannan

    I tried with RemoveCommand then using AppendCommand, that too returned as ReplaceCommand. So there is issue in core which needs to be fixed.

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance nod_ Lille

    Not sure when I gave feedback about this issue. looking at the difference between jQuery and our code, they have a shortcut when a single element is parsed, which we didn't replicate.

    I like the template solution, we should try it out and see if the tests are green. We might need an additional test to make sure we don't break this again.

    As for revert, given that this code has been shipped in several minor and major releases for over half a year, It's easier to fix it here and ship the fix in the next release. Nobody should have to rewrite their code because of this change, otherwise there'd be a change record attached to the issue.

  • First commit to issue fork.
  • Pipeline finished with Failed
    about 1 month ago
    Total: 126s
    #432520
  • ๐Ÿ‡บ๐Ÿ‡ฆUkraine artem_sylchuk Lutsk

    Created the MR with the described changes.
    I suppose it will require additional tests, maybe some code comments updates but I hope it is OK to have it as the first step and see what the tests results will say.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 90s
    #432738
  • ๐Ÿ‡บ๐Ÿ‡ฆUkraine artem_sylchuk Lutsk

    The first attempt has broken the behaviors attachment. Thanks @reinfate for spotting and fixing it.
    Hope this time it works.

  • Pipeline finished with Success
    about 1 month ago
    Total: 1161s
    #432741
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    To avoid breaking this in the future probably best to add test coverage for the issue.

Production build 0.71.5 2024