Skip to content

Commit

Permalink
webview: Better match the invoke-lightbox logic to the webapp.
Browse files Browse the repository at this point in the history
When the user touches an image in the message list, we've been
deciding whether to invoke the lightbox for it based on whether its
parent element is a link with `target="_blank"`.  This is unsemantic
and kind of quirky, and in fact it doesn't always get the right
answer; for example, this was causing us to pull up a (failed,
blank) lightbox for the avatar in an embedded tweet, or for the
giant file-type icon in an embedded Dropbox link.

Instead, use the rather more semantically plausible test found in
the webapp.

Also add a few comments; explain in particular the "video" exceptions.

This code still isn't quite right, and the difference shows up in
the case of an embedded Dropbox *image*: we should be getting the
image URL from, well, the `img` element, but instead we're getting
it from the enclosing link, which has a different job and in the
case of a Dropbox image points to the `?dl=0` HTML page which
displays the image.  In that case, a lightbox is the right thing,
but because we use the wrong URL we show a blank one.  Leave that as
a TODO to be fixed separately, along with thumbnailing-awareness.
  • Loading branch information
gnprice committed Oct 2, 2018
1 parent f0a4709 commit 61c1017
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 8 deletions.
7 changes: 4 additions & 3 deletions src/webview/js/generatedEs3.js
Original file line number Diff line number Diff line change
Expand Up @@ -300,11 +300,12 @@ documentBody.addEventListener('click', function (e) {
return;
}
if (e.target.matches('a[target="_blank"] > img') && !e.target.matches('.youtube-video > a[target="_blank"] > img') && !e.target.matches('.vimeo-video > a[target="_blank"] > img')) {
var inlineImageLink = e.target.closest('.message_inline_image a');
if (inlineImageLink && !inlineImageLink.closest('.youtube-video, .vimeo-video')) {
sendMessage({
type: 'image',
src: e.target.parentNode.getAttribute('href'),
messageId: getMessageIdFromNode(e.target)
src: inlineImageLink.getAttribute('href'),
messageId: getMessageIdFromNode(inlineImageLink)
});
return;
}
Expand Down
14 changes: 9 additions & 5 deletions src/webview/js/js.js
Original file line number Diff line number Diff line change
Expand Up @@ -340,15 +340,19 @@ documentBody.addEventListener('click', e => {
return;
}

/* Should we pull up the lightbox? For comparison, see the webapp's
* static/js/lightbox.js , starting at the `#main_div` click handler. */
const inlineImageLink = e.target.closest('.message_inline_image a');
if (
e.target.matches('a[target="_blank"] > img')
&& !e.target.matches('.youtube-video > a[target="_blank"] > img')
&& !e.target.matches('.vimeo-video > a[target="_blank"] > img')
inlineImageLink
/* The webapp displays certain videos inline, but on mobile
* we'd rather let another app handle them, as links. */
&& !inlineImageLink.closest('.youtube-video, .vimeo-video')
) {
sendMessage({
type: 'image',
src: e.target.parentNode.getAttribute('href'),
messageId: getMessageIdFromNode(e.target),
src: inlineImageLink.getAttribute('href'), // TODO: should be `src` / `data-src-fullsize`.
messageId: getMessageIdFromNode(inlineImageLink),
});
return;
}
Expand Down

0 comments on commit 61c1017

Please sign in to comment.