881152 - hdpi/xhdpi resolution assets and layout for PDF Viewer app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::PDF Viewer, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This bug represents fixes needed in PDF Viewer app in order to deliver bug 881126:

-- Have visual supply us with @1.5x and the correct @2x images.
-- On v1.1hd, modify CSS/JS so that apps will display @1.5x/@2x images correctly.
-- On master, drop the @1.5x and @2x images and make sure there is no
more to tweak besides changes made on bug 830644.

As HIDPI support never landed on v1-train, I expect the patch for both
branches on this bug will be different. The CSS/JS patch on v1.1hd will be based on the patch on master, but I won't expect it to be as simple as a |git cherry-pick|; so this is
where the engineering work lays.

The patch on top of v1-train should land on v1.1hd.

Comment 1

[Wayne Chang [:wchang]]

Setting hd+ for 1.1hd specific requirement

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This should be fairly simple.

Comment 3

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

According to https://github.com/mozilla/pdf.js/pull/3269#issuecomment-20631691,
the assets and maybe CSS tweaks needed for this bug would be complete with Bug 876906.

However, I don't see why Bug 876906 is related to hi-resolution support.

Hi bdahl, 
May I know if we have an estimate when the assets would be done?
Thanks.

Comment 4

[Brendan Dahl [:bdahl]]

The bug is not related, but from that bug we will update pdf.js on gaia which will include hidpi support from https://github.com/mozilla/pdf.js/pull/3269.  

Just to be clear there are still some images missing and I will not be providing those as I did not create them.

Comment 5

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Brendan,

Thanks for the clarification.

Hi Patryk,

Are you the one we should consult to get the 2x/1.5x images for pdf.js or we should contact Eric Pang directly?

The assets we need are here,
https://github.com/mozilla/pdf.js/tree/master/extensions/b2g/images

- icon_next_page.png 	
- icon_previous_page.png 	
- icon_zoom_in.png 	
- icon_zoom_out.png 	
- spinner.png

Thanks.

Comment 6

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Ping Eric as well.

Eric, can you help to provide the assets we need as Comment 5?
Thanks

Comment 7

[Patryk Adamczyk [:patryk] UX]

Amy will be able to provide graphics for this.

Comment 8

[Amy Lee [:amylee] UX]

Attachment: Requested graphics

Hi, 

Attached are the requested graphics. We don't have the vector of "spinner.png" since it was created by Telefonica. A request was sent for the graphic so I will attach it when I get it.

Comment 9

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Amy,

Could you also provide the following graphics,

 1. @2x
   - icon_next_page.png 	
   - icon_previous_page.png 	   
   - icon_zoom_in.png 	
   - icon_zoom_out.png 	
   - spinner.png

 2. @1.5x
   - div_line_left.png
   - div_line_right.png
   - spinner.png

Thank you.

Comment 10

[Amy Lee [:amylee] UX]

Attachment: PDF Assets

Hi Rudy, 

Attached are the right/left div lines. I am waiting for the "spinner.png" graphic from Telefonica so I will send it over when I get it. I don't think we are supporting @2x graphic sizes. Patryk can you confirm?

Comment 11

[Amy Lee [:amylee] UX]

Attachment: Progress_Activity_Assets@1.5

Hi Rudy, 

Telefonica got back to me and they said that the "spinner.png" is out of date and should not be used. I've attached what we are using for the spinner graphic and here is a link to the guidelines. 

https://wiki.mozilla.org/Gaia/Design/BuildingBlocks#Progress_.26_Activity_Indicators

Comment 12

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Amy from comment #10)
> Created attachment 774613 [details]
> PDF Assets
> 
> Hi Rudy, 
> 
> Attached are the right/left div lines. I am waiting for the "spinner.png"
> graphic from Telefonica so I will send it over when I get it. I don't think
> we are supporting @2x graphic sizes. Patryk can you confirm?

Hi Amy,

I think we do need to support @2x.
For example, you may see we have some graphics @2x in browser app, 
https://github.com/mozilla-b2g/gaia/tree/master/apps/browser/style/images

Could you please help on that?
Thanks.

Comment 13

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Amy from comment #10)
> Created attachment 774613 [details]
> PDF Assets
> 
> Hi Rudy, 
> 
> Attached are the right/left div lines.

Hi Amy,

For the div lines, the 1x resolution is 2 x 34, so for @1.5x, I think we need "3 x 51". (right now, it's 2 x 51)

Could you help provide those assets again?
Thank you.

Comment 14

[Amy Lee [:amylee] UX]

(In reply to Rudy Lu [:rudyl] from comment #13)
> (In reply to Amy from comment #10)
> > Created attachment 774613 [details]
> > PDF Assets
> > 


Hi Rudy, 

The hairline looks too thick at 3px so we are keeping it at 2px. As for the @2x graphics this is Patryk's response:

"As far as the @2x.
This is only supported on the dev. preview phone, the Geeksphone Peak. I haven't heard that we are supporting these phones past release. We should focus on the @1.5x work anyway, as there is a hard deadline Aug 10th."

Comment 15

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

The 1.5x assets have been piled and I just sent a pull request to pdf.js
https://github.com/mozilla/pdf.js/pull/3479

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Rudy Lu [:rudyl] from comment #15)
> The 1.5x assets have been piled and I just sent a pull request to pdf.js
> https://github.com/mozilla/pdf.js/pull/3479

It had been merged!

Comment 17

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch to update pdf.js

This is the pull request to update pdf.js to 0.8.0352.
Thanks.

Comment 18

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Brendan, 

I found Bug 895312 should be a gecko issue and has been resolved.
Could you pleas help review attachment 777663 [details] or should I find someone else to help?

Thank you.

Comment 19

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 777663 [details]
Patch to update pdf.js

Hi Vivien,

Any chance you can help review this change to update pdf.js in Gaia?
Thank you.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 777663 [details]
Patch to update pdf.js

Sounds like the changes have landed on pdf.js upstream. Let's update our version.

Thanks folks for working together :)

Comment 21

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Landed to Gaia master,
4e16dc5dc63d706a3f40496a71f30e4bb4ae103e

v1.1.0hd branch, 
f4ad65e688dba4555ade50027687214c7e686b0b

--
Vivien,

Thanks for the review.