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

Fix incorrect link handling of inline MD images #4543

Merged
merged 3 commits into from Jan 6, 2020

Conversation

@abdusabri
Copy link
Contributor

abdusabri commented Dec 18, 2019

Summary

As of MM-12898, inline markdown images open the image preview modal, except if the markdown image itself is a link. e.g. [![Build Status](https://travis-ci.org/mattermost/platform.svg?branch=master)](https://travis-ci.org/mattermost/platform).

This PR fixes the following bug:

For a case like ![image](https://www.mattermost.org/wp-content/uploads/2016/03/logoHorizontal.png) an image plus some text that has [a link](http://somelink)

Observed behavior: The image doesn't have a pointer cursor, and doesn't open the preview modal

Expected behavior: The image should have a pointer cursor and open the preview modal

Reason of wrong behavior: just because there is a link ([a link](http://somelink)) in the markdown, it falsely assumed that the image itself is actually a link - which is not the case in the example given above

The bug is not a regression, it is a missed case since the implementation of MM-12898, but was only discovered after MM-15232, MM-18158. The issue can be reproduced on community V5.18 and latest master. The bug is not limited to channel header descriptions or its associated system messages, and can be reproduced in regular chat messages as well.

Ticket Link

https://mattermost.atlassian.net/browse/MM-21519

Screenshots

Before the fix
image

After the fix
image

Copy link

jgilliam17 left a comment

Thank you @abdusabri
Tested, looks good to merge.
Markdown has a pointer and opens in the preview as expected.

@mattermod

This comment has been minimized.

Copy link
Contributor

mattermod commented Dec 26, 2019

Test server destroyed

@mattermod

This comment has been minimized.

Copy link
Contributor

mattermod commented Jan 6, 2020

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

Copy link
Member

sudheerDev left a comment

@abdusabri 👍 Thanks for the PR.

@sudheerDev

This comment has been minimized.

Copy link
Member

sudheerDev commented Jan 6, 2020

/update-branch

@jasonblais jasonblais added the AutoMerge label Jan 6, 2020
@mattermod

This comment has been minimized.

Copy link
Contributor

mattermod commented Jan 6, 2020

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@sudheerDev sudheerDev removed the AutoMerge label Jan 6, 2020
@sudheerDev sudheerDev merged commit 63b56b5 into mattermost:master Jan 6, 2020
15 checks passed
15 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build-docker Your tests passed on CircleCI!
Details
ci/circleci: build-storybook Your tests passed on CircleCI!
Details
ci/circleci: i18n-check Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: prepare-docker-build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: type-check Your tests passed on CircleCI!
Details
ci/circleci: upload-s3 Your tests passed on CircleCI!
Details
ci/circleci: upload-storybook Your tests passed on CircleCI!
Details
cla/mattermost abdusabri authorized
Details
mattermost/serverless-ops/github PR is up-to-date.
merge/blocked Merged allowed
Details
untagged-build Workflow: untagged-build
Details
@amyblais amyblais added this to the v5.19.0 milestone Jan 6, 2020
@amyblais

This comment has been minimized.

Copy link
Member

amyblais commented Jan 6, 2020

This can be cherry-picked to v5.19 as long as it's not risky. @sudheerDev

@sudheerDev

This comment has been minimized.

Copy link
Member

sudheerDev commented Jan 6, 2020

/cherry-pick release-5.19

@mattermod

This comment has been minimized.

Copy link
Contributor

mattermod commented Jan 6, 2020

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Fetching origin
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-#4543-upstream-release-5.19-1578335234
Branch 'automated-cherry-pick-of-#4543-upstream-release-5.19-1578335234' set up to track remote branch 'release-5.19' from 'upstream'.
+++ Downloading patch to /tmp/4543.patch (in case you need to do this again)

+++ About to attempt cherry pick of PR. To reattempt:
  $ git am -3 /tmp/4543.patch

Applying: Fix incorrect link handling of inline MD images
Using index info to reconstruct a base tree...
M	utils/__snapshots__/message_html_to_component.test.jsx.snap
M	utils/message_html_to_component.jsx
M	utils/message_html_to_component.test.jsx
Falling back to patching base and 3-way merge...
Auto-merging utils/message_html_to_component.test.jsx
CONFLICT (content): Merge conflict in utils/message_html_to_component.test.jsx
Auto-merging utils/message_html_to_component.jsx
CONFLICT (content): Merge conflict in utils/message_html_to_component.jsx
Auto-merging utils/__snapshots__/message_html_to_component.test.jsx.snap
Patch failed at 0001 Fix incorrect link handling of inline MD images
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

+++ Conflicts detected:

UU utils/message_html_to_component.jsx
UU utils/message_html_to_component.test.jsx

+++ Aborting in-progress git am.

+++ Returning you to the master branch and cleaning up.

@sudheerDev

This comment has been minimized.

Copy link
Member

sudheerDev commented Jan 6, 2020

@amyblais will manually cherrypick

sudheerDev added a commit that referenced this pull request Jan 6, 2020
Co-authored-by: mattermod <mattermod@users.noreply.github.com>
@ogi-m ogi-m added the Tests/Done label Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.