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

Title finder not working correctly #576

Closed
bencroker opened this issue Aug 26, 2021 · 10 comments
Closed

Title finder not working correctly #576

bencroker opened this issue Aug 26, 2021 · 10 comments
Labels
bug Something isn't working

Comments

@bencroker
Copy link
Collaborator

bencroker commented Aug 26, 2021

The title finder, as well as the check for SVG tags, require tags that are closed immediately. Tags that have attributes are not being detected, the regexp and the indexOf functions need tweaking.

<!-- This works: -->
<title>New title</title>
<svg><title>Image title</title></svg>

<!-- This doesn't work: -->
<title id="">New title</title>
<svg id=""><title>Image title</title></svg>

<!-- This also doesn't work: -->
<svg><title>Image title</title></svg>
<title>New title</title>

htmx/src/htmx.js

Lines 708 to 718 in 0aa372d

var TITLE_FINDER = /<title>([\s\S]+?)<\/title>/im;
function findTitle(content) {
if(content.indexOf('<title>') > -1 &&
(content.indexOf('<svg>') == -1 ||
content.indexOf('<title>') < content.indexOf('<svg>'))) {
var result = TITLE_FINDER.exec(content);
if (result) {
return result[1];
}
}
}

@bencroker
Copy link
Collaborator Author

@1cg any suggestions on approaches to fixing this? The current logic is overly simple and won’t work, but getting into regexp patterns can lead to complex code. Right now I don’t see any other way though.

@1cg
Copy link
Contributor

1cg commented Sep 2, 2021

I agree, and I don't think this would be too bad.

/<svg[^>]*>/

or something like that. Are you willing to take a crack at it?

@bencroker
Copy link
Collaborator Author

bencroker commented Sep 2, 2021

Well here's my crack at implementing your version with a regexp.

function findTitle(content) { 
    var contentWithSvgsRemoved = content.replace('/<svg[^>]*>[\s\S]*?<\/svg>/gim', '');
    var result = contentWithSvgsRemoved.match('/<title[^>]*>([\s\S]*?)<\/title>/im');

    if (result) { 
        return result[1]; 
    }
}

@1cg
Copy link
Contributor

1cg commented Sep 3, 2021

how's about this hack?

image

@bencroker
Copy link
Collaborator Author

bencroker commented Sep 3, 2021

Feels quite hacky, still looks for an exact match of <title> and still fails in this case:

<svg id=""><title>Image title</title></svg>
<title id="">New title</title>

I dislike the idea of matching against <title and <svg because it doesn't rule out <titlex> and <svgx> tags, then again neither does my regular expression :(

@bencroker
Copy link
Collaborator Author

bencroker commented Sep 3, 2021

Ok, so this version does rule out <titlex> and <svgx> tags :)

function findTitle(content) { 
    var contentWithSvgsRemoved = content.replace(/<svg(\s[^>]*>|>)([\s\S]*?)<\/svg>/gim, '');
    var result = contentWithSvgsRemoved.match(/<title(\s[^>]*>|>)([\s\S]*?)<\/title>/im);

    if (result) { 
        return result[2]; 
    }
}

@1cg
Copy link
Contributor

1cg commented Sep 3, 2021

Good points. I'm good w/ this, if you want to make the change.

@bencroker
Copy link
Collaborator Author

bencroker commented Sep 3, 2021

Yup, I'll write a test and implement, thanks for taking a look.

@bencroker
Copy link
Collaborator Author

bencroker commented Sep 5, 2021

Added tests in 21ec109 and implemented in a76f9f3.

@1cg Would you manually spin up a 1.5.1 test directory or do you have an automated process to do this?

@1cg
Copy link
Contributor

1cg commented Sep 6, 2021

Tests verified and I've created a 1.5.1 directory for testing. Shooting for a release this week.

Thank you @bencroker!

@1cg 1cg closed this as completed Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants