Fix html entity in search results #2844
Conversation
coffee/lib/abstract-chosen.coffee
Outdated
| if query.length | ||
| startpos = text.search highlightRegex | ||
| prefix = text.slice(0, startpos) | ||
| fix = text.slice(startpos, startpos + query.length) |
|
See also:
Plus like a scrillion other issues and PRs linked off of those. |
The conflict was in `abstract-chosen-coffee`, and was a result of slightly overlapping areas of effort between this and #2844.
|
I'm sorry – I had forgotten about this PR. Just got this up-to-speed and mergeable with 800b7c9.
@koenpunt I feel pretty certain that we aren't reintroducing the earlier problems. The first commit in this PR is what prevents the same problem that was fixed in #2254. The main problem with the prior attempts was not escaping the HTML as it's built up for the highlighted result, and here we're very explicit about it: 0adf2d5#diff-a3162852f1360124c43007c8130261dcR201 |
coffee/lib/abstract-chosen.coffee
Outdated
| prefix = text.slice(0, startpos) | ||
| fix = text.slice(startpos, startpos + query.length) | ||
| suffix = text.slice(startpos + query.length) | ||
| option.highlighted_html = "#{@escape_html(prefix)}<em>#{@escape_html(fix)}</em>#{@escape_html(suffix)}" |
There was a problem hiding this comment.
For functions we use this instead of @, so please use this.escape_html(prefix)
There was a problem hiding this comment.
Updated in 5940e3b.
|
Probably not a change that you can add some tests for this? 😅 |
I think maybe I can. I'll try and see what happens. |
|
@satchmorun Can you squash this down to one (or a few) logical commits? |
satchmorun
force-pushed
the
fix-html-entity-in-search-results
branch
from
d4715b1 to
d500495
Compare
Before this change, if you tried to search for "&" to match "Stuff
& Nonsense", what we would do is html-escape _both_ things and compare
the escaped strings.
This works fine in the case where you just type "&". However, if you
type "mp", that will match the html entity "&" in the middle of the
html-escaped text we're comparing it to. Which is incorrect.
Additionally, we highlight our "matched" result. When we do, we break up
the entity so that the resulting HTML looks like:
Stuff &a<em>mp</em>; Nonsense
And you get to see what basically looks like garbage on the screen.
This commit fixes all of that by just comparing the actual text of
things. We avoid the problem previous addressed in #2254 by escaping
each piece of the to-be-highlighted option text before assembling it
into HTML.
satchmorun
force-pushed
the
fix-html-entity-in-search-results
branch
from
d500495 to
8c638e9
Compare
It's no longer being used for the searching/matching process and is now simply a string of highlighted html.
satchmorun
force-pushed
the
fix-html-entity-in-search-results
branch
from
8c638e9 to
07e5179
Compare
|
@tjschuck done and done |
Great! |
Don't search escaped HTML text
Before this change, if you tried to search for "&" to match "Stuff & Nonsense", what we would do is html-escape both things and compare the escaped strings.
This works fine in the case where you just type "&". However, if you type "mp", that will match the html entity "&" in the middle of the html-escaped text we're comparing it to. Which is incorrect.
Additionally, we highlight our "matched" result. When we do, we break up the entity so that the resulting HTML looks like:
And you get to see what basically looks like garbage on the screen:
The meat of this PR and this PR description comes from this commit. The subsequent commits are just refactorings and tidyings.
PR made in a pairing session with @braddunbar.