From e3feae32e1ba8fdcfc48fa979006d7b7bbf9e73e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 17 Feb 2016 19:50:04 +0000 Subject: [PATCH] Fix search clickthrough for HTML events Switch to using a normal link for search result clickthrough. Apart from generally giving a better experience, this means that it also works on html messages. The problem there was that we were attaching onClick handlers to s which we were then flattening into HTML with ReactDOMServer (which meant the onClick handlers were never attached to React's list of listeners). To make this work without jumping through React hoops, the highlighter now returns either a list of strings or a list of nodes, depending on whether we are dealing with an HTML event or a text one. We therefore have a separate HtmlHighlighter and TextHighlighter. --- src/HtmlUtils.js | 93 +++++++++++++------ src/components/structures/RoomView.js | 13 +-- src/components/views/messages/MessageEvent.js | 14 ++- src/components/views/messages/TextualBody.js | 16 +++- src/components/views/rooms/EventTile.js | 8 +- .../views/rooms/SearchResultTile.js | 6 +- 6 files changed, 102 insertions(+), 48 deletions(-) diff --git a/src/HtmlUtils.js b/src/HtmlUtils.js index 0b7f17b2b2..fe97d7b84f 100644 --- a/src/HtmlUtils.js +++ b/src/HtmlUtils.js @@ -17,7 +17,6 @@ limitations under the License. 'use strict'; var React = require('react'); -var ReactDOMServer = require('react-dom/server') var sanitizeHtml = require('sanitize-html'); var highlight = require('highlight.js'); @@ -50,14 +49,23 @@ var sanitizeHtmlParams = { }, }; -class Highlighter { - constructor(html, highlightClass, onHighlightClick) { - this.html = html; +class BaseHighlighter { + constructor(highlightClass, highlightLink) { this.highlightClass = highlightClass; - this.onHighlightClick = onHighlightClick; - this._key = 0; + this.highlightLink = highlightLink; } + /** + * apply the highlights to a section of text + * + * @param {string} safeSnippet The snippet of text to apply the highlights + * to. + * @param {string[]} safeHighlights A list of substrings to highlight, + * sorted by descending length. + * + * returns a list of results (strings for HtmlHighligher, react nodes for + * TextHighlighter). + */ applyHighlights(safeSnippet, safeHighlights) { var lastOffset = 0; var offset; @@ -71,10 +79,12 @@ class Highlighter { nodes = nodes.concat(this._applySubHighlights(subSnippet, safeHighlights)); } - // do highlight - nodes.push(this._createSpan(safeHighlight, true)); + // do highlight. use the original string rather than safeHighlight + // to preserve the original casing. + var endOffset = offset + safeHighlight.length; + nodes.push(this._processSnippet(safeSnippet.substring(offset, endOffset), true)); - lastOffset = offset + safeHighlight.length; + lastOffset = endOffset; } // handle postamble @@ -92,31 +102,64 @@ class Highlighter { } else { // no more highlights to be found, just return the unhighlighted string - return [this._createSpan(safeSnippet, false)]; + return [this._processSnippet(safeSnippet, false)]; } } +} + +class HtmlHighlighter extends BaseHighlighter { + /* highlight the given snippet if required + * + * snippet: content of the span; must have been sanitised + * highlight: true to highlight as a search match + * + * returns an HTML string + */ + _processSnippet(snippet, highlight) { + if (!highlight) { + // nothing required here + return snippet; + } + + var span = "" + + snippet + ""; + + if (this.highlightLink) { + span = "" + +span+""; + } + return span; + } +} + +class TextHighlighter extends BaseHighlighter { + constructor(highlightClass, highlightLink) { + super(highlightClass, highlightLink); + this._key = 0; + } /* create a node to hold the given content * - * spanBody: content of the span. If html, must have been sanitised + * snippet: content of the span * highlight: true to highlight as a search match + * + * returns a React node */ - _createSpan(spanBody, highlight) { + _processSnippet(snippet, highlight) { var spanProps = { key: this._key++, }; if (highlight) { - spanProps.onClick = this.onHighlightClick; spanProps.className = this.highlightClass; } - if (this.html) { - return (); - } - else { - return ({ spanBody }); + var node = { snippet }; + + if (highlight && this.highlightLink) { + node = {node} } + return node; } } @@ -128,8 +171,7 @@ module.exports = { * * highlights: optional list of words to highlight, ordered by longest word first * - * opts.onHighlightClick: optional callback function to be called when a - * highlighted word is clicked + * opts.highlightLink: optional href to add to highlights */ bodyToHtml: function(content, highlights, opts) { opts = opts || {}; @@ -144,18 +186,13 @@ module.exports = { // by an attempt to search for 'foobar'. Then again, the search query probably wouldn't work either try { if (highlights && highlights.length > 0) { - var highlighter = new Highlighter(isHtml, "mx_EventTile_searchHighlight", opts.onHighlightClick); + var highlighter = new HtmlHighlighter("mx_EventTile_searchHighlight", opts.highlightLink); var safeHighlights = highlights.map(function(highlight) { return sanitizeHtml(highlight, sanitizeHtmlParams); }); // XXX: hacky bodge to temporarily apply a textFilter to the sanitizeHtmlParams structure. sanitizeHtmlParams.textFilter = function(safeText) { - return highlighter.applyHighlights(safeText, safeHighlights).map(function(span) { - // XXX: rather clunky conversion from the react nodes returned by applyHighlights - // (which need to be nodes for the non-html highlighting case), to convert them - // back into raw HTML given that's what sanitize-html works in terms of. - return ReactDOMServer.renderToString(span); - }).join(''); + return highlighter.applyHighlights(safeText, safeHighlights).join(''); }; } safeBody = sanitizeHtml(content.formatted_body, sanitizeHtmlParams); @@ -167,7 +204,7 @@ module.exports = { } else { safeBody = content.body; if (highlights && highlights.length > 0) { - var highlighter = new Highlighter(isHtml, "mx_EventTile_searchHighlight", opts.onHighlightClick); + var highlighter = new TextHighlighter("mx_EventTile_searchHighlight", opts.highlightLink); return highlighter.applyHighlights(safeBody, highlights); } else { diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 25c289ba96..cd02d724b5 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -879,15 +879,6 @@ module.exports = React.createClass({ }); }, - _onSearchResultSelected: function(result) { - var event = result.context.getEvent(); - dis.dispatch({ - action: 'view_room', - room_id: event.getRoomId(), - event_id: event.getId(), - }); - }, - getSearchResultTiles: function() { var EventTile = sdk.getComponent('rooms.EventTile'); var SearchResultTile = sdk.getComponent('rooms.SearchResultTile'); @@ -948,10 +939,12 @@ module.exports = React.createClass({ } } + var resultLink = "#/room/"+this.props.roomId+"/"+mxEv.getId(); + ret.push(); + resultLink={resultLink}/>); } return ret; }, diff --git a/src/components/views/messages/MessageEvent.js b/src/components/views/messages/MessageEvent.js index 2490d9be8b..9cc0e22c59 100644 --- a/src/components/views/messages/MessageEvent.js +++ b/src/components/views/messages/MessageEvent.js @@ -28,6 +28,18 @@ module.exports = React.createClass({ } }, + propTypes: { + /* the MatrixEvent to show */ + mxEvent: React.PropTypes.object.isRequired, + + /* a list of words to highlight */ + highlights: React.PropTypes.array, + + /* link URL for the highlights */ + highlightLink: React.PropTypes.string, + }, + + render: function() { var UnknownMessageTile = sdk.getComponent('messages.UnknownBody'); @@ -48,6 +60,6 @@ module.exports = React.createClass({ } return ; + highlightLink={this.props.highlightLink} />; }, }); diff --git a/src/components/views/messages/TextualBody.js b/src/components/views/messages/TextualBody.js index e3613ef9a3..92447dd1da 100644 --- a/src/components/views/messages/TextualBody.js +++ b/src/components/views/messages/TextualBody.js @@ -28,6 +28,17 @@ linkifyMatrix(linkify); module.exports = React.createClass({ displayName: 'TextualBody', + propTypes: { + /* the MatrixEvent to show */ + mxEvent: React.PropTypes.object.isRequired, + + /* a list of words to highlight */ + highlights: React.PropTypes.array, + + /* link URL for the highlights */ + highlightLink: React.PropTypes.string, + }, + componentDidMount: function() { linkifyElement(this.refs.content, linkifyMatrix.options); @@ -46,14 +57,15 @@ module.exports = React.createClass({ shouldComponentUpdate: function(nextProps) { // exploit that events are immutable :) return (nextProps.mxEvent.getId() !== this.props.mxEvent.getId() || - nextProps.highlights !== this.props.highlights); + nextProps.highlights !== this.props.highlights || + nextProps.highlightLink !== this.props.highlightLink); }, render: function() { var mxEvent = this.props.mxEvent; var content = mxEvent.getContent(); var body = HtmlUtils.bodyToHtml(content, this.props.highlights, - {onHighlightClick: this.props.onHighlightClick}); + {highlightLink: this.props.highlightLink}); switch (content.msgtype) { case "m.emote": diff --git a/src/components/views/rooms/EventTile.js b/src/components/views/rooms/EventTile.js index f580686128..36ec85e91b 100644 --- a/src/components/views/rooms/EventTile.js +++ b/src/components/views/rooms/EventTile.js @@ -96,8 +96,8 @@ module.exports = React.createClass({ /* a list of words to highlight */ highlights: React.PropTypes.array, - /* a function to be called when the highlight is clicked */ - onHighlightClick: React.PropTypes.func, + /* link URL for the highlights */ + highlightLink: React.PropTypes.string, /* is this the focussed event */ isSelectedEvent: React.PropTypes.bool, @@ -313,8 +313,8 @@ module.exports = React.createClass({ { avatar } { sender }
- +
); diff --git a/src/components/views/rooms/SearchResultTile.js b/src/components/views/rooms/SearchResultTile.js index 9d3af16ee7..9c793e8705 100644 --- a/src/components/views/rooms/SearchResultTile.js +++ b/src/components/views/rooms/SearchResultTile.js @@ -29,8 +29,8 @@ module.exports = React.createClass({ // a list of strings to be highlighted in the results searchHighlights: React.PropTypes.array, - // callback to be called when the user selects this result - onSelect: React.PropTypes.func, + // href for the highlights in this result + resultLink: React.PropTypes.string, }, render: function() { @@ -53,7 +53,7 @@ module.exports = React.createClass({ } if (EventTile.haveTileForEvent(ev)) { ret.push() + highlightLink={this.props.resultLink}/>); } } return (