HTML Search: Use anchor for search preview #11944
Conversation
wlach
changed the title
| const docContent = htmlElement.querySelector('[role="main"]'); | ||
| let docContent = undefined; | ||
| if (anchor && htmlElement.querySelector(anchor)) { | ||
| docContent = htmlElement.querySelector(anchor); |
There was a problem hiding this comment.
I have a concern here: I think this is going to retrieve the text content within the anchor element?
In practice we may want to retrieve some of the following HTML sibling element text as well.
We do have some test coverage on searchtools.js and I wonder whether we could use that to make some assertions about what we retrieve and summarize (and also safeguard the feature against regressions).
There was a problem hiding this comment.
I don't think we want the sibling element. The anchor defines a section within the documentation. I don't think we want (or need) any text outside of it: it's (1) likely to be out of context and (2) probably outside of the text we'd take for the snippit except in edge cases.
Here's the section for the 2to3 example you gave earlier:
There was a problem hiding this comment.
Ok, brilliant. I'm doing a few more tests locally, and getting around to agreeing with all this.
|
@jayaddison Ready for another look whenever you are. Added some tests while I was here |
| </html>`; | ||
|
|
||
| it("basic case", () => { | ||
| expect(Search.htmlToText(testHTML).trim().split(/\s+/)).toEqual([ |
There was a problem hiding this comment.
I have some ideas about the reason, but just to check they match reality: why are we testing htmlToText and not makeSearchSummary?
There was a problem hiding this comment.
I think it's easier to test the lower-level construct to verify that it doesn't have any bugs. makeSearchSummary has a bunch of magic numbers and heuristics in it which seems much more subject to change.
| }); | ||
|
|
||
| it("will start reading from the anchor", () => { | ||
| expect(Search.htmlToText(testHTML, '#other-section').trim().split(/\s+/)).toEqual(['Other', 'Section', 'Other', 'text']); |
There was a problem hiding this comment.
Is the trim...split behaviour here to avoid being too-precise about whitespace in the output?
If so, an alternative could be to make a less-restrictive assertion; something like retrievedText.startsWith("Other Section").
There was a problem hiding this comment.
That's right, just trying not to test whitespace. I think I prefer to check for all the tokens in the output, feels more comprehensive to me.
|
@wlach Could you add a changelog entry btw? |
wlach
force-pushed
the
use-anchor-for-search-previews
branch
from
00607a9
to
9c5d55f
Compare
Done. |
|
Thank you! |
…d summary retrieval)
) This is a post-merge adjustments for #11944.
Subject: Use anchor for generating search previews
Feature or Bugfix
Purpose
Attempts to use anchor for generating HTML preview, if available. This ensures that the preview is based on the anchor in the document, for cases where we matched on that title.
Detail
You can reproduce this bug using the procedure @jayaddison mentioned in #11943
Then search for something like "test"
Before:
After:
Note how the content in the "after" image actually reflects what you'd be jumping to!
Relates
Closes #11943
Discovered when discussing #11942