Launching fullscreen media view should require tapping directly on media bubble

[michaelkirk]

Steps to reproduce

• receive attachment
• tap on white space horizontally adjacent to attachment

Actual result: Fullscreen media view opens
Expected result: Fullscreen media view should only open if you click on the media bubble.

[gluxon]

This appears to be the line causing the issue.

4d320d6#diff-cdc353260e006d747b55ce6d90702e1dR1041

I'd like to do a pull request, but I would be concerned that deleting the line breaks something else. What's the reason didTapMessageBubbleAtIndexPath is being called by didTapCellAtIndexPath?

[michaelkirk]

I believe it's there to support tapping on info/error messages.

e.g. The "key changed" dialog.
On Sun, Jul 17, 2016 at 6:50 PM Brandon Cheng [email protected]
wrote:

This appears to be the line causing the issue.

4d320d6#diff-cdc353260e006d747b55ce6d90702e1dR1041
4d320d6#diff-cdc353260e006d747b55ce6d90702e1dR1041

I'd like to do a pull request, but I would be concerned that deleting the
line breaks something else. What's the reason
didTapMessageBubbleAtIndexPath is being called by didTapCellAtIndexPath?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1257 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANP4T580ijtdYxkkMT0fUJKa1R6TX6iks5qWtv9gaJpZM4JMAgw
.

[gluxon]

Ah. I assumed the comment was referring to internal debugging info/error messages. Thanks for the quick response.

[big-r81]

@michaelkirk
Hi,
on my device (iPhone 6, iOS 9.3.2, Signal 2.3.5) there is no such a problem.

it seems that this was introduced after Signal 2.3.5 ...

[michaelkirk]

it seems that this was introduced after Signal 2.3.5 ...

That's likely, a lot of code moved around in the recently merged #1255

[gluxon]

Yeah, the line I linked to was introduced a few days ago. So the root of the issue is that Signal's subclassed JSQMessageCollectionViewCell has no messageBubbleContainerView property for info and error messages. JSQMessages relies heavily on that for bubble tap math. The workaround is to just add a check in the didTapCellAtIndexPath to only call didTapMessageBubbleAtIndexPath for those messages.

I'm going to try getting OWSDisplayedMessageCollectionViewCell properly subclass. If that introduces too many errors, I'll use the workaround.

[michaelkirk]

Thanks for look @gluxon. As this is a new regression, it will have to be fixed soon before the next release can go out. I'm conceptually in favor of being able to use the default bubble sizing, if it is straight forward enough and means we can avoid hacks like this: https://github.com/WhisperSystems/Signal-iOS/blob/master/Signal/src/Models/OWSMessagesBubblesSizeCalculator.m#L27

But if not, I'll just merge in something like this: https://github.com/WhisperSystems/Signal-iOS/compare/fix-tap-directly-on-media-cell%231257

[michaelkirk]

closed in #1260