870950 - [MMS] [UX] Thread view. Image thumbnail in bubble has a wrong size

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Victoria Gerchinhoren [:vicky]]

Attachment: shows the big size of the thumbnail

Image inside the bubble should never pass the limit of 100px both height and width keeping the proportion of the image of course.

Comment 1

[Mike Pennisi [:jugglinmike]]

Hi Victoria,

I'm working on this now, but I have one question about the maximum width:

Currently, images are displayed as "block-level" elements, so no text will appear on the same line. This makes limiting their width to 100px seem somewhat unnecessary--wide images may be scaled down further than necessary. The resulting unused space will be especially noticeable when the Messaging application is run at wide resolutions. Do you think we could set the max-width to be the width of the available space?

(Note that this does not effect the maximum height of the image--we can set that to be 100px regardless of the maximum width we choose.)

Comment 2

[Mike Pennisi [:jugglinmike]]

Attachment: Fix image dimensions

I'm attaching my proposal for a fix for this bug. This patch implements a maximum image width of 100%, which is in opposition to the bug description. Because of this, I will wait until I hear back from Victoria before requesting a formal review.

Comment 3

[Victoria Gerchinhoren [:vicky]]

Hi Mike,
The fixed size has 2 reasons: 
1. to keep a consistant size of thumbnails, since the image is only one of the kind of files that can be attached in an MMS. 
2. to keep a size that allows the user to have a more general view of the thread without an image covering all the screen. 

Basically the thumbnail should just be a preview hint of the content given in a message but does not aim to work as a full preview, same as you cannot play a video or a song without going to the video/music app.

Thanks!

Comment 4

[ayman maat :maat]

(In reply to Victoria Gerchinhoren from comment #3)
> Hi Mike,
> The fixed size has 2 reasons: 
> 1. to keep a consistant size of thumbnails, since the image is only one of
> the kind of files that can be attached in an MMS. 
> 2. to keep a size that allows the user to have a more general view of the
> thread without an image covering all the screen. 

Victoria, Looking at Mike's comment 1 he is talking about fixing the hight to 100px but allowing flexible width. Surely this would not impact negatively on screen content as there is nothing to the left and right of the image, but would indeed allow wider images to scale and prevent them from being scaled down further than necessary. Are you sure you want to specify the fixing of image hight *and* width?

Comment 5

[Mike Pennisi [:jugglinmike]]

Attachment: "Hard" width limit

This demonstrates the effect that a "hard" limit on image width will have for "wide" images.

Comment 6

[Mike Pennisi [:jugglinmike]]

Attachment: "Flexible" width limit

This demonstrates the effect that a "flexible" limit on image width will have for "wide" images.

Comment 7

[Mike Pennisi [:jugglinmike]]

Thanks Ayman, that is the crux of it! I created a couple images (see comment 5 and comment 6) to help illustrate the issue.

Comment 8

[Victoria Gerchinhoren [:vicky]]

Thanks Mike,

I see your point, and appreciate your solution. But anyhow, the visual proposal is to have a maximum of 12rem (chequed with Patryk and since gallery uses that size instead of 10rem, will go for that for the sake of consistency) horizontal for landscape as well as 12rem vertical for portrait.

let me know if you have further questions. 

PS: the hit area for triggering the gallery app is the whole bubble, right?

Comment 9

[ayman maat :maat]

> PS: the hit area for triggering the gallery app is the whole bubble, right?

hey Victoria. 

The whole bubble cannot be the target area for a single app as a single MMS message can contain multiple files of different types: image, video and music as well as web links and phone numbers within the text. in order to achieve the requirements outlined in the wireframes each individual item within the bubble needs to be its own hit area.

Comment 10

[Mike Pennisi [:jugglinmike]]

Attachment: Fix maximum image dimensions

This patch implements the "hard" limit on image height and width as described in comment 8.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 752277 [details] [diff] [review]
Fix maximum image dimensions

Review of attachment 752277 [details] [diff] [review]:
-----------------------------------------------------------------

You've added twice the same patch as a patch ;) be careful next time.

Not r+ing because I'd like the answer to the question first.

::: apps/sms/js/thread_ui.js
@@ +729,5 @@
>      ThreadUI.scrollViewToBottom();
>    },
>  
>    createMmsContent: function thui_createMmsContent(dataArray) {
> +    var container = document.createDocumentFragment();

why this change ?

I mean, I'm not against it per se, and it doesn't seem to break anything, so just curious here.

Comment 12

[Mike Pennisi [:jugglinmike]]

Attachment: Fix maximum image dimensions

I'm re-submitting the patch with the duplicate commit removed (strange as it may sound, I sometimes confuse the Unix `>` and `>>` operators...)

The reason I have replaced the `div` element with a Document Fragment is to minimize the amount of unnecessary markup generated. Because we already have a way to target MMS content containers (`.message.mms p`), this markup is redundant. Using this class in a CSS selector ignores the more regular structure we have defined for message markup, and would result in less maintainable stylesheets.

Comment 13

[Julien Wajsberg [:julienw]]

(In reply to Mike Pennisi [:jugglinmike] from comment #12)
> Created attachment 752735 [details] [diff] [review]
> Fix maximum image dimensions
> 
> I'm re-submitting the patch with the duplicate commit removed (strange as it
> may sound, I sometimes confuse the Unix `>` and `>>` operators...)


Strange, I don't use this at all. `git format-patch` creates the file for me :)
Another alternative is to `git format-patch -U8 --stdout xxx | xclip` :)

> 
> The reason I have replaced the `div` element with a Document Fragment is to
> minimize the amount of unnecessary markup generated. Because we already have
> a way to target MMS content containers (`.message.mms p`), this markup is
> redundant. Using this class in a CSS selector ignores the more regular
> structure we have defined for message markup, and would result in less
> maintainable stylesheets.

oki, fine for me

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 752735 [details] [diff] [review]
Fix maximum image dimensions

r=me !

Comment 15

[Mike Pennisi [:jugglinmike]]

Thanks, Julien!

Landed at b774a63c6a374d97da66c2310cf910c50acfc914

Comment 16

[Victoria Gerchinhoren [:vicky]]

Mike, alignment in the thread is wrong, and that's causing the images thumbnails to look so weird. Please take a look at the attachment, where I specify the issues that are not correctly implemented. It would be good to have a call or chat about it, in order to clarify the specs.

My skype user @vixvixy

Thanks!

Comment 17

[Victoria Gerchinhoren [:vicky]]

Attachment: specs alignment in thread view and image placement

Comment 18

[Julien Wajsberg [:julienw]]

There was a landing, Victoria could we file another bug, put depends on this one ? Will make uplifts easier !

Thanks !

Comment 19

[Mike Pennisi [:jugglinmike]]

Hi Victoria,

Two of the issues you have specified in this new attachment have dedicated bugs on BugZilla:

- Bug 871055 - [MMS] [UX] Thread view. Wrong spacing between Image thumbnail and text below it
- Bug 873033 - [MMS] [UX] Messages thread view. Correct the line height for text inside msg bubbles

The final issue (relating to text justification and alignment) has no associated bug (as far as I can see). I recommend creating a new bug for that issue and marking it as another dependency of "Bug 872514 - [MMS] [UX] Align with UX specs".

I think you have done a good job explaining these issues; I understand them well. It doesn't look like anyone has had the opportunity to address the other UX problems yet. The reason why I closed this bug with the Status "Resolved" is because this bug pertains specifically to image sizing.

If you don't mind, could we re-close this bug and continue the discussion on those other issues in their specific pages?

Comment 20

[Victoria Gerchinhoren [:vicky]]

Yes, Mike, here is the bug: https://bugzilla.mozilla.org/show_bug.cgi?id=875690

Thanks!

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train:5d6dc5c