925216 - [Media] [Gallery][User Story] Display file information for the content displayed in the gallery app

Status: RESOLVED FIXED

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

Description

[Sri Kasetti]

User story:
As a user, I want to see the file type, size, resolution and date of creation of the content displayed in the gallery app

Comment 2

[Tom Herold]

Attachment: Screenshot.png

I added an "i" (info) button to the interface. The icon is same as in the video app. (Since this feature is basically the copy of the video app's info screen)

@Rob: Just wanted to let you know about that, so that we can keep count of all the buttons :-)

Comment 3

[Tom Herold]

Attachment: Github Pull Request

Comment 4

[Tom Herold]

Attachment: Screenshot Info view.png

I attached a screenshot of the current (and very basic) version of the information view for gallery images. We were debating whether we should a histogram of the image in there as well. It would be easy to do, but its value to the user may be limited. 
@kyee David Flanagan mentioned you were a (hobby) photographer. How valuable is a histogram really?

Also, for the future, I could image that we could display additional metadata information like GPS coordinates, camera manufacturer or whatever else we can read from the EXIF data. :dmarcos has similar intentions with all the metadata stuff.

So, the real question here. Do you want anything else here? Should we add the histogram here or just ship it as is?

Comment 5

[Tom Herold]

Attachment: Github Pull Request

Comment 6

[David Flanagan [:djf]]

I'd think that the Close button should be opaque. Is it unintentionally inheriting opacity < 1 from its container?

Comment 7

[Rob MacDonald [:robmac]]

(In reply to David Flanagan [:djf] from comment #6)
> I'd think that the Close button should be opaque. Is it unintentionally
> inheriting opacity < 1 from its container?

Agree. Tom, please take a look at the building blocks and they'll show you examples of what this should look like.

https://developer.mozilla.org/en-US/Apps/Design/Building_Blocks/Confirmation?redirectlocale=en-US&redirectslug=Web%2FApps%2FGraphics_and_UX%2FBuilding_Blocks%2FConfirmation

Also, I'm not sure if this is the right font. I'll flag Eric in visual for feedback as well.

Comment 8

[Rob MacDonald [:robmac]]

Jacqueline will be providing additional feedback on the interaction side.

Comment 9

[Jacqueline Savory [:jsavory] UX]

Attachment: Gallery Additional Metadata.pdf

Everything looks great, however the only issue I can see is that the floating back icon is not ideal. I've attached a single page spec to illustrate an idea we could use instead of the back icon. Essentially, replacing the camera icon with the grid icon will allow users to quickly return to the grid through the toolbar. Users will still have easy access to the camera in the grid view. Let me know if you have any questions.

Comment 10

[Rob MacDonald [:robmac]]

Comment on attachment 816817 [details]
Screenshot.png

Hi Tom...

Please see Jacqueline's and my earlier comments. Almost there! 

Thanks,
Rob

Comment 11

[Punam Dahiya [:pdahiya]]

Thanks Tom for working on this, overall the code looks good, few points noted that i have commented in the pull request. 

Also, i tried taking screen shot of the info view in gallery app (press power + home icon at the same time), the screen shot of the info view loses it background grey (see attached)

Comment 12

[Punam Dahiya [:pdahiya]]

Attachment: screenshot_info_view_missing_greybkg.png

Comment 13

[Eric Pang [:epang] UX]

(In reply to Punam Dahiya from comment #12)
> Created attachment 821369 [details]
> screenshot_info_view_missing_greybkg.png

Hi Tom, I think this looks good but a semi transparent background is not needed.  Seeing as the Gallery's background is black you can make the background 100% black, this will also solve the issue of the transparent close button.  Thanks!

Comment 14

[Tom Herold]

(In reply to Eric Pang [:epang] from comment #13)
> (In reply to Punam Dahiya from comment #12)
> > Created attachment 821369 [details]
> > screenshot_info_view_missing_greybkg.png
> 
> Hi Tom, I think this looks good but a semi transparent background is not
> needed.  Seeing as the Gallery's background is black you can make the
> background 100% black, this will also solve the issue of the transparent
> close button.  Thanks!

Regarding the background:
Fixing the opacity of button is no problem. It is a very easy fix. Do you really want me to make the background 100% black? What about the pattern that is applied to the current background. Do you still want this?
Th current background loads 2 images. It seems to me that "gradient.png" has some transparency. So if you want an opaque background but still keep the gradient I need an new image:

url("images/ui/pattern.png") repeat scroll left top, url("images/ui/gradient.png") no-repeat scroll left top / 100% 100% transparent

Regarding the fonts: 
The screenshot was taken by running Gaia on my desktop browser. It used my default system font Helvetica. (and Lucia Grande for the close button) In the device it falls back to FFOX standard font.

Comment 15

[Tom Herold]

Attachment: dark opaque background screenshot

I used a very subtle darkish gradient for the background now plus the standard FFOX grainy pattern on top of that. I found the gradient in the defauts of the video app's CSS.

(Again, this picture was taken on the desktop, hence Helvetica)

Comment 16

[Tom Herold]

(In reply to Punam Dahiya from comment #12)
> Created attachment 821369 [details]
> screenshot_info_view_missing_greybkg.png

I can not reproduce that on my Leo phone. I tried taking sreenshots several time, but they always came out right.

Comment 17

[Punam Dahiya [:pdahiya]]

Hi Tom, 
Pull request looks good. One last thing that i commented in pr also is about calling updateimageinfo on click of info button instead of showfile, previousfile and nextfile. This will avoid extra work every time a user is opening gallery file. what do you think?

With above point fixed, its r+ from me, i tested the screen shot and it came out fine.

I will suggest getting a ui-review done of the updated flow and info view background before landing the patch.

Thanks

Comment 18

[Punam Dahiya [:pdahiya]]

Also, it will be good to see  unit/integration tests added for this feature, I know we have a separate PR for gallery tests, not sure if the info view test cases are included in bug 915661 PR

Comment 19

[Tom Herold]

Attachment: app.zip for UX-review

@jsavory: I hope I packed the app correctly for a UX-review. Please let me know if you are satisfied with the background, the layout of stuff and the new "back" button workflow. Thanks

Comment 20

[Punam Dahiya [:pdahiya]]

Comment on attachment 820066 [details] [review]
Github Pull Request

Looks good, Thanks Tom for working on it!

Comment 21

[Hema Koka [:hema]]

Jacqueline, 

Could you please do the ux review so we can close this one down?

Thanks
Hema

Comment 22

[Stephany Wilkes]

Removing the flag for Casey; this one is on Jacqueline's list for today.

Comment 23

[Stephany Wilkes]

Tom or David, UX needs to see a screenshot of this (or a pointer to what the correct screenshot is, if one exists) before we can understand the patch and advise. If someone can post one, Jacqueline can review. Thanks!

Comment 24

[Hema Koka [:hema]]

DJF - In Tom's absence could you please provide the screenshot for UX

Comment 25

[David Flanagan [:djf]]

Stephany, Jacqueline:

Tom attached an app.zip file that should enable you to try out his patch live. This is the one he set the ui-review flag on for Jacqueline.

Or, failing that, it looks like this attachment is the latest screenshot: https://bugzilla.mozilla.org/attachment.cgi?id=822389

If neither of those work, please let me know what you need.

Comment 26

[Hema Koka [:hema]]

Not a 1.3 blocker, however targeted for 1.3

Comment 27

[Jacqueline Savory [:jsavory] UX]

Thanks for the screenshot, David. Everything looks good to me. 
Peter - I was wondering if you could take a quick look at the screenshot David references in comment 25 to ensure that it looks correct to you.

Comment 28

[Peter La]

Attachment: MediaInfo_VisualSpec+Icons.zip

Hi Jacqueline/David,

Please see the attached zip for a Spec Patryk did for Video Details.  It should be followed for this feature as well.

The current visuals and layout are off (as of the image provided in comment 25 by David).  It should follow our pattern for dialogs.  The current one has the wrong background and button, as well as layout and font issues.  Patryk's PSDs spec this out nicely.

Comment 29

[Tom Herold]

I had no idea the Spec Sheet existed. I will take a look at it again next week.

@djf: Are we in a rush to deliver this? If not, I'd be happy to take a look at it on Monday and make the necessary fixes.

Comment 30

[Hema Koka [:hema]]

(In reply to Tom Herold from comment #29)
> I had no idea the Spec Sheet existed. I will take a look at it again next
> week.
> 
> @djf: Are we in a rush to deliver this? If not, I'd be happy to take a look
> at it on Monday and make the necessary fixes.

Tom, This is a target feature for 1.3. If you are able to address it next week, that would be wonderful! (Updating the milestone)

Thanks
Hema

Comment 31

[Tom Herold]

Attachment: GalleryVsVideo.jpg

Hey Peter, 

I need some more concrete feedback for fixing the style of this. I attached a screenshot comparing this view with the video app's info view. 

Looking at the design spec for the video app it seems that it was never followed to the point or changed a long the way. However, since I so far have been following the style of the video app there are some difference that I need input for:

1)
Both don't have the dividers as outlined in the spec. Do you want them in both or none?

2) 
Both apps use the "i" button icon with a straight "i". (I just copied the video app's icon.) Do you want the slightly angled one?

3) 
Higher up in the discussion it was proposed to use an opaque background for the gallery's view. Is this still the case? If so, please let me know whether you want a gradient or not. (and what colors for the gradient)

4) 
As far as offsets and fonts are concerned they are the exact same as in the video app. (see screenshot)

I hope this covers everything to get this finished.

Comment 32

[Peter La]

(In reply to Tom Herold from comment #31)
> Created attachment 8334133 [details]
> GalleryVsVideo.jpg
> 
> Hey Peter, 
> 
> I need some more concrete feedback for fixing the style of this. I attached
> a screenshot comparing this view with the video app's info view. 
> 
> Looking at the design spec for the video app it seems that it was never
> followed to the point or changed a long the way. However, since I so far
> have been following the style of the video app there are some difference
> that I need input for:
> 
> 1)
> Both don't have the dividers as outlined in the spec. Do you want them in
> both or none?
> 

Peter: Both apps should use the divider lines as spec'd.

> 2) 
> Both apps use the "i" button icon with a straight "i". (I just copied the
> video app's icon.) Do you want the slightly angled one?
> 

Peter: The angled info icon is new for 1.3, so it should be the one that is used.  The new angled icon was attached with the spec zip file.

> 3) 
> Higher up in the discussion it was proposed to use an opaque background for
> the gallery's view. Is this still the case? If so, please let me know
> whether you want a gradient or not. (and what colors for the gradient)
> 

Peter:
Both gallery and video apps should use the same background, which is the same background used for ALL dialogs.  This dialog background has a radial semi-transparency in it and also a noise pattern.  However, the one they implemented for video app is actually not correct.  Please align both apps to use this background.  This background image was included in the zip file I attached in comment 28 in the file called Video_Details_130820.psd.  I will also attach it in the following comment.

> 4) 
> As far as offsets and fonts are concerned they are the exact same as in the
> video app. (see screenshot)
> 

Peter: Again, both apps should be following the spec as far as text size and layout are concerned.  If they are off, they need to be aligned.

> I hope this covers everything to get this finished.

Comment 33

[Peter La]

Attachment: dialogbackground.zip

Comment 34

[Hema Koka [:hema]]

Tom, Could you please provide an update -- I am assuming that you are working on this based on your last email. If not, please let us know so we can reassign it.

Thanks
Hema

Comment 35

[Tom Herold]

Hey Hema. Sorry for the silence, but I was out of town for some days. I am working on this and should expect the final patch this weekend.

Comment 36

[Jean Gong :jgong]

Update whiteboard tag to follow format [ucid:{id}, {release}:p{1,2}, ft:{team-id}]

Comment 37

[Jean Gong :jgong]

Update whiteboard tag to follow format [ucid:{id}, {release}:p{1,2}, ft:{team-id}]

Comment 38

[Tom Herold]

I updated the pull request to make the dialog look ore in line with the UX spec. The CSS changes are both to the gallery and the video app. Since I didn't find a reference dialog in the system I could only manually compare this patch with the UX spec when fine tuning the background opacity.

Comment 39

[Hema Koka [:hema]]

NI Fabrice (1.3 sheriff) to get his approval for landing this feature for 1.3 once we have all the reviews completed this week.

Comment 40

[Punam Dahiya [:pdahiya]]

Comment on attachment 820066 [details] [review]
Github Pull Request

Hi Tom,

Thanks for fixing styles in both video and gallery, it looks good. Only thing that we should fix is the opacity in background. 

As Peter clarified in comment https://bugzilla.mozilla.org/show_bug.cgi?id=925216#c32, info dialog background should have radial semi-transparency, similar to other dialogs, we can take out the opacity.I have left comment in github also. With that fixed let's set it for ui-review with Przemek Abratowski, as Peter is OOO.

Comment 41

[Tom Herold]

Attachment: opacity.jpg

Form left to right:
1) Background pattern/image + 0.95 opacity applied
2) Example from the spec Peter provided
3) Background pattern/image with no additional opacity. 

#1 looks more like the original, but I agree it doesn't feel radial. On the other hand #3 hardly shows any transparency at all. 

I get the feeling that in order to get this right and have more prominent radial transparency effect the background image has to be modified. Or am I missing something here? I will post a comparison image later.

Comment 42

[Tom Herold]

Attachment: background.jpg

Well, this is interesting. It looks like if the background is computed with the CSS styles that mix the pattern and the gradient it looks different then when using Peter's background image (which already merges the pattern+gradient into one image) from comment #33.

Either Gecko renders it differently then Photoshop or the new background image just has a lot more radial transparency. 

@Peter can you please send me the background image again, but this time as separate gradient and pattern. Thank you.

Comment 43

[Hema Koka [:hema]]

Peter La is OOO, NI Przemek for the images (separate images) so we can wrap this up soon.

Comment 44

[Przemek Abratowski [:przemek] UX]

You should be using the background that's already in the building blocks for this dialog. 

Here is a link to a bug that contains the correct building block.
https://docs.google.com/a/mozilla.com/spreadshee/ccc?key=0AtVT90hlMtdSdEd4TVVjWXNfU3ctMlVhWFRrWkpweVE#gid=39

-Przemek

Comment 45

[Przemek Abratowski [:przemek] UX]

Sorry wrong I sent the wrong link, this is the correct link for that background building block,
http://buildingfirefoxos.com/building-blocks/confirm.html

Comment 46

[Jean Gong :jgong]

What's the latest status on this for 1.3? Thank you!

Comment 47

[[:fabrice] Fabrice Desré]

(In reply to Jean Gong from comment #46)
> What's the latest status on this for 1.3? Thank you!

I'm waiting for everything to be r+ before I give approval. There are many attachments, if some are obsolete please mark them as such.

Comment 48

[Hema Koka [:hema]]

Tom, 

Thanks so much for your contributions to get this feature completed. Could you please provide an ETA on when we can update the background image to address the opacity concern. We are trying to see if we are able to get this wrapped up before our 1.3 feature complete date (12/9). If not, it will have to move on to the next release vehicle. Please provide an update.

Thanks
Hema

Comment 49

[David Flanagan [:djf]]

I have serious reservations about comment #9 however. The floating back button was a solution (before Jacqueline's time, maybe?) to a serious usability problem. Switching back to the grid button does not seem like a good idea, and I'm not willing to land that as part of this patch without more UX discussions.

In any case the back button would more properly be handled as a separate bug, not tacked on to this info display bug.

I've added some additional review comments on github to Tom's patch.  Since this bug is still assigned to me and the deadline is approaching, I'm going to start working on it myself, I guess.

Comment 50

[David Flanagan [:djf]]

I wonder if we can get the background right by actually using the building block that Prezmik refers to in comment #45.

Comment 51

[David Flanagan [:djf]]

Geez. The icons attached in comment 28 are 170kb each.  And they're in this pull request in that form!

Comment 52

[David Flanagan [:djf]]

Attachment: link to patch on github

This new pull request is based on Tom's, but has a number of changes:

1) it is based on the dialogs in shared/style/confirm.css, so we automatically get the right backgrounds and buttons, and don't have the CSS headaches that have held this feature up for a month.

2) The feature has been moved into separate info.js and info.css files which are lazy-loaded when needed.

3) I've removed the back button change that Jacqueline requested in comment #9. I think we have user research showing that a grid icon in the lower left just does not work for users. We have the floating back button for a good reason.  If UX really has changed its mind about this, they can file a new bug to change the back button and not tack it on to this feature.

4) The info is now a <dl> with <dt> and <dd> elements, which makes more semantic sense than a <ul> with <li><span/><span/></li> elements.

5) The icons supplied by Peter in comment 28 had a bizarrely large file size. I've used the icons from the video app instead: they are visually indistinguishable.

6) I've modified the video app to use a confirm dialog with a <dl> list and the same new info.css file that the gallery app does. So now both apps use identical markup and styles

Punam: you were reviewing Tom's version of the patch, so I'm asking you to review this one as well.  

John: I'm also flagging you for review because this patch modifies the video app, and because your Monday starts well before Punam's does. (I'd like to land this on Monday before the FC deadline)  If you have time to review this, that would be great. But if not, Punam can do it.

Fabrice: I'm setting needinfo for you because this cleaned up patch will need your approval to land.  Note that I've deleted most of the attachments, so it should be simpler to understand what is going on.

Comment 53

[David Flanagan [:djf]]

Attachment: screenshot of the video app displaying file information

Comment 54

[David Flanagan [:djf]]

Attachment: screenshot of the gallery app displaying file information

Przemek: would you review these two screenshots and give UX approval to land?

As noted above, I'm now using the building blocks directly, so the dialog background and the Close buttons should be exactly the same as anything from shared/style/confirm.css.

I'm pretty sure that I implemented Peter's spec from comment 28 precisely. I tried to measure a screenshot and the pixels distances match Peter's spec to the best of my ability to judge text baselines by eye.

Comment 55

[David Flanagan [:djf]]

Jacqueline,

I've set needinfo just to let you know that I have not removed the floating back button as you requested in comment #9.

As noted above in comment #52, I think going back to the grid icon would be a bad idea. If you really want to get rid of the floating back button though, please file a separate bug for that and we can argue about it there :-)

Comment 56

[Przemek Abratowski [:przemek] UX]

Hi David,

This looks great, the only difference that I see between your screenshot and Peters spec on comment 28 is that the lines of text are left aligned at 20px in your spec and 30px in Peters. Everything else, background and buttons looks the same.

Przemek

Comment 57

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8344154 [details]
screenshot of the video app displaying file information

Hi David,

This looks great, the only difference that I see between your screenshot and Peters spec on comment 28 is that the lines of text are left aligned at 20px in your spec and 30px in Peters. Everything else, background and buttons looks the same.

Przemek

Comment 58

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8344155 [details]
screenshot of the gallery app displaying file information

Hi David,

This looks great, the only difference that I see between your screenshot and Peters spec on comment 28 is that the lines of text are left aligned at 20px in your spec and 30px in Peters. Everything else, background and buttons looks the same.

Przemek

Comment 59

[David Flanagan [:djf]]

Attachment: screenshot of the video app displaying file information

Comment 60

[David Flanagan [:djf]]

Attachment: screenshot of the gallery app displaying file information

Przemik,

Thanks for spotting that. Its a trivial change to add 1rem of padding on the left for the text. I've attached two new screenshots showing the results.

Comment 61

[David Flanagan [:djf]]

I've updated the github pull request with a new version of info.css for both gallery and video apps that adds the extra indent for the text.

Comment 62

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8344287 [details]
screenshot of the video app displaying file information

Thank you David, this is great.

Comment 63

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8344288 [details]
screenshot of the gallery app displaying file information

Thank you David, this is great.

Comment 64

[Punam Dahiya [:pdahiya]]

Comment on attachment 8344152 [details] [review]
link to patch on github

Making info-view a confirm dialog form element, has made styles neat and simplified in both gallery and video app. Looked at all the flows and the patch looks good. Thanks for working on it, r+ from me.

Comment 65

[[:fabrice] Fabrice Desré]

David, approval for 1.3 from me.

Comment 66

[David Flanagan [:djf]]

The tree is reopened. I've rebased and pushed the new commit to github in order to restart the Travis job.

Comment 67

[David Flanagan [:djf]]

Restarting Travis again. It was green on all tests except the test_agent tests which had an unrelated failure.  Previously I was green on the test_agent tests and had a failure on the marionette tests.  If I don't get all green this time, I'm going to take those past two runs and count all the overlapping greens and call it good enough.

Comment 68

[David Flanagan [:djf]]

Landed to master: https://github.com/mozilla-b2g/gaia/commit/55cc0a04d9041231f169dc669e2f5256eecd8855

Comment 69

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8344152 [details] [review]
link to patch on github

It uses building block for info dialog. Thanks about this.

Comment 71

[Jacqueline Savory [:jsavory] UX]

Removing ni? flag as bug is fixed.