883028 - [Gallery] Crop option is missing when set as wallpaper from gallery

Status: RESOLVED WONTFIX

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

Description

[Leo]

1. Title: When setting as wallpaper from gallery, crop option is missing.
2. Precondition: Battery should be low
3. Tester's Action: (1) Open Gallery
                    (2) Open any image Or select any one image
		    (3) Press shared button
		    (4) selet "set as wallpaper" option 
4. Detailed Symptom (ENG.): No crop option is available 
5. Expected: It should has the crop option before setting as wallpaper 
6. Reproducibility: Y
1) Frequency Rate: 100%
7. Gaia Master/v1-train: Reproduced on Gaia master
8. Gaia Revision:  a25fb6bd1b0284ce3e197e88567d433c7815ee52

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

I don't remember there's cropping option when setting as wallpaper from gallery...is it?

Comment 2

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

It has a cropping in gallery but not in wallpaper. The set as wallpaper is handled by wallpaper app.

Comment 3

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

There is a cropping option when choosing wallpaper from homescreen's option menu(long press on homescreen). But there is no cropping option from gallery to use share menu to set wallpaper.

Comment 4

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

Leo,

do you mean you want a cropping option like choosing wallpaper from home screen??

BTW, is this related to this bug??
2. Precondition: Battery should be low

It should always be without cropping option no matter how much battery is.

Comment 5

[Leo]

(In reply to John Hu [:johnhu] from comment #4)
> Leo,
> 
> do you mean you want a cropping option like choosing wallpaper from home
> screen??
> 
> BTW, is this related to this bug??
> 2. Precondition: Battery should be low
> 
> It should always be without cropping option no matter how much battery is.

Yes, It should allow to cropping while setting as wallpaper from Gallery.
It is not related to low battery condition.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Uh, are you taking the wrong role?

Comment 7

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

Sorry. I push wrong role...

Comment 8

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

Attachment: patch for this bug

Use ImageEditor to crop the wallpaper.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 764592 [details]
patch for this bug

I haven't looked at the quality of the code at all. I'm giving r- here because I feel strongly that this is too big a patch to be landing in version 1.1.

Comment 10

[David Flanagan [:djf]]

This is a new feature request, not a bug report.

It was filed on June 13th and was given Leo+ status and a June 24th deadline with no triage from Mozilla. I'm resetting to Leo? and removing the target deadline.

We already resolved a similar bug a month or two ago. It used to be that the wallpaper app, when given an image with a different aspect ratio than the screen, would letter box it, cutting equal amounts off the left and right (or the top and bottom).  Now, the user can pan the image before clicking "Set as wallpaper" to select what part of the image they want to use.

And if the user really wants to zoom in and only display a part of an image as wallpaper, then they can easily edit the image in the Gallery first, before setting it as wallpaper.

Adding full image cropping functionality to the wallpaper app may well be a good idea, but it is not something we should try to squeeze into the v1.1 release at the last minute.

Triage: there is no way this should block the release: there are easy alternatives to achieve the desired outcome.

Hema: would you add this bug to the Media team's backlog for UX review and consideration for v1.2?

Comment 11

[Hema Koka [:hema]]

Added this to the backlog and will let Sri do the priority call.

Comment 12

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

jongsoo.oh, 

I think you should tell us the reason you mark it as QE3 because David already told his concern about this bug: there are easy alternatives to achieve the desired outcome.

Please write your reason and need info David.

Comment 13

[Wayne Chang [:wchang]]

Resetting target/flags after discussion with partner.

Comment 15

[Hema Koka [:hema]]

Need Ux input on image cropping functionality for Wallpaper app (See comment 10)

Comment 16

[Rob MacDonald [:robmac]]

John, for the UX review, is it possible for you to package this as per James Burkes' instructions?

https://github.com/jrburke/gaia-dev-zip#for-the-developer

We've been having issues with github and this really simplifies the process for us. Just flag me when it's available.

Comment 17

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

Hi Rob, 

I haven't an acceptable patch for this bug, yet. Hema put need info flag to request your thought about this feature. Would you mind to tell us more about crop options in wallpaper?

Comment 18

[Sri Kasetti]

This is added to the product backlog

Comment 19

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

Remove myself since I am not working on this for a long time.

Comment 20

[Hema Koka [:hema]]

Nice to have but not targeted for 1.3 (removing 1.3 nom and is added to the backlog)

Comment 21

[ranjith253]

Attachment: UXscenario-ImageCrop from wallpaper application

Hi,
I have two UX proposals  for adding crop option in the wallpaper application.

 Scenario1 : This scenario is same as proposed patch by johnhu , user can view both crop & wallpaper preview in the same screen

Scenarion2: User has a intermediate crop view before wallpaper preview .

I request the UX engineer to review the attached scenario videos

Comment 22

[Wayne Chang [:wchang]]

Adding FxOS UX team.

Comment 23

[Stephany Wilkes]

Removing flag for our team, since Rob is already here. Adding Jacqueline, though, who is the Reviewer for this area.

Comment 24

[Sri Kasetti]

This is a target feature for 1.4. We already discussed during the product grooming session.
UX can review the proposal above...

Comment 25

[Jacqueline Savory [:jsavory] UX]

I think the first scenario will work better as it does not create an extra step for the user. 

However, in the attached video the bottom crop handles look as if they are being covered by the buttons. For this screen, could we display the entire image including crop handles?

Comment 26

[ranjith253]

Attachment: CropUX.jpg

Yes we can show entire image & crop handlers in the screen,for reference i am attaching the cropUX.jpg .

Comment 27

[ranjith253]

Attachment: Pointer to Pull Request.html

Hi Mr.chang,

Please assign Mozilla engineer to review my proposed patch for Crop option in Wallpaper app.

Comment 28

[David Flanagan [:djf]]

Comment on attachment 8356063 [details]
Pointer to Pull Request.html

I haven't looked carefully at this patch yet, but am giving r- because it copies gallery/js/ImageEditor.js wholesale and includes lots and lots of unused code.  If we want to copy the cropping UX from Gallery (and I'm not sure we do), we should at least remove all of the webgl-related code.  And also the free-form cropping (e.g. the corner drag handles) code.  The cropping code should be at least 50% smaller than it is, I think.

Also, I think more UX work is needed here. With the current design, there does not appear to be any way to preview the wallpaper at its cropped size before setting it.  This means, for example, that if you crop heavily, you might end up with a wallpaper image that is smaller than the actual screen size.  (Is there any code to prevent that?) Then, when displayed on the screen, it would look pixellated. 

Maybe we need to put the cancel and set buttons into a header bar and then put crop and undo crop buttons at the bottom of the screen?

Another approach to cropping (completely different than what Gallery does) would be to use pinch gestures to zoom in and swipe gestures to pan. We'd display the image in a MediaFrame object and at the full size of the screen (with cancel and save buttons overlaid). The user would zoom and pan the image until they got it just how they wanted it and then click Save, and we'd set the wallpaper to just the currently displayed region of the image.  Both Gallery and Camera already have code to handle the zooming and panning, and it would be a lot simpler to re-use that code than to reuse the entire image editor.

Setting needinfo for Jacqueline to request more UX input here.

Comment 29

[ranjithatbugzilla]

(In reply to David Flanagan [:djf] from comment #28)
> Comment on attachment 8356063 [details]
> Pointer to Pull Request.html
> 
> I haven't looked carefully at this patch yet, but am giving r- because it
> copies gallery/js/ImageEditor.js wholesale and includes lots and lots of
> unused code.  If we want to copy the cropping UX from Gallery (and I'm not
> sure we do), we should at least remove all of the webgl-related code.  And
> also the free-form cropping (e.g. the corner drag handles) code.  The
> cropping code should be at least 50% smaller than it is, I think.
  Thanks for your inputs, i will try to remove the unused code.
> Also, I think more UX work is needed here. With the current design, there
> does not appear to be any way to preview the wallpaper at its cropped size
> before setting it.  This means, for example, that if you crop heavily, you
> might end up with a wallpaper image that is smaller than the actual screen
> size.  (Is there any code to prevent that?) Then, when displayed on the
> screen, it would look pixellated. 
  
 I think in the existing gallery crop UX also user has no option to preview the wallpaper before setting it,do we need to handle this in gallery also?. If I am not wrong I have not noticed any pixellated effect after doing heavy cropping & setting it as wallpaper from gallery.By adopting the Gallery UX scenario it gives uniformity across the applications for crop option.
   
> Maybe we need to put the cancel and set buttons into a header bar and then
> put crop and undo crop buttons at the bottom of the screen?

> ok for me,but when we consider uniformity undo crop option should be added in gallery also.

> Another approach to cropping (completely different than what Gallery does)
> would be to use pinch gestures to zoom in and swipe gestures to pan. We'd
> display the image in a MediaFrame object and at the full size of the screen
> (with cancel and save buttons overlaid). The user would zoom and pan the
> image until they got it just how they wanted it and then click Save, and
> we'd set the wallpaper to just the currently displayed region of the image. 
> Both Gallery and Camera already have code to handle the zooming and panning,
> and it would be a lot simpler to re-use that code than to reuse the entire
> image editor.

 I think this will restrict the user from cropping the image smaller than the screen size , but user looses the freedom of choosing the particular area in the image like user needs to select a small area of for setting its a wallpaper. 

> Setting needinfo for Jacqueline to request more UX input here.

Comment 30

[ranjithatbugzilla]

Bugzilla is updated with following comment 

https://bugzilla.mozilla.org/show_bug.cgi?id=883028#c29

Comment 31

[ranjithatbugzilla]

Sorry above comment is not related this thread.

Comment 32

[ranjithatbugzilla]

Hi Djf,

I have started working on clean up of unused code, i made following changes.

1. Removed Web-gl related code .
2. Removed Corner drag handles code.
3. Handled croponly code.
4. Free from cropping  code is removed.

Please add any other items if i have missed in the above list .

Do we need to handle image resize while changing device orentation?, as u know this is not taken care while setting wallpaper from gallery (i .e wallpaper app -> gallery scenario).

Comment 33

[ranjith253]

Attachment: Uploading work in progress patch file for this bug

The patch contains required changes as suggested by djf, 
looking forward for feedback comments .

Comment 34

[David Flanagan [:djf]]

Comment on attachment 8367827 [details] [diff] [review]
Uploading work in progress patch file for this bug

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

This is better than last time, but I think there is still a *lot* of simplification to be done in imageCropEditor.js.

Here are some issues to think about:

  - do you want to allow the user to create an cropped image that is too small?
 
  - do you want to have some kind of "apply" button that applies the crop and shows what it would look like, but allows the user to continue cropping?  We can do that when editing an image in the Gallery, but it doesn't look like we can do that with this design.

I am also worried that you are putting a lot of work into this without any kind of UX spec. Note that I won't be able to land a patch like this until we have a design from Mozilla's UX team.

::: apps/wallpaper/js/imageCropEditor.js
@@ +2,5 @@
> +'use strict';
> +
> +/*
> + * imageCropEditor.js: simple image editing and previews in a <canvas> element.
> + *

Update these comments, please.  There is no image editing here anymore, just cropping, right?  Maybe you should change the filename, too.  image_cropper.js or something.

@@ +26,5 @@
> + * Bug 935273: if previewURL is passed then we're using the imageCropEditor
> + *  only to display a crop overlay. We avoid decoding the full-size
> + *  image for as long as we can. Also, if we're only cropping then we
> + *  can avoid all of the webgl stuff.
> + */

You're not using the previewURL here, so you should remove all comments and code related to that, I think.

@@ +40,5 @@
> +  this.previewCanvas = document.createElement('canvas');
> +  this.previewCanvas.id = 'edit-preview-canvas'; // for stylesheet
> +  this.container.appendChild(this.previewCanvas);
> +  this.previewCanvas.width = this.previewCanvas.clientWidth;
> +  this.previewCanvas.height = this.previewCanvas.clientHeight;

For this app, it might be simpler to make both canvas elements static parts of share.html rather than creating them dynamically here.

@@ +53,5 @@
> +  // Start loading the image into a full-size offscreen image
> +  this.original = new Image();
> +  this.original.src = imageURL;
> +  this.preview = new Image();
> +  this.preview.src = null;

Since you are only cropping here and not editing, you really only need one image. The extra code for maintaining a separate preview might just be making things more complicated. If you have the full-size image, you can use CSS transformations to display it at the right size and position.  Really, you should only need to use a canvas.toBlob() at the very end when the user is done cropping.

@@ +70,5 @@
> +    });
> +  };
> +}
> +
> +imageCropEditor.prototype.generateNewPreview = function(callback) {

As mentioned above, you probably don't need this function at all, if you are clever with how you display the full-size image.

@@ +141,5 @@
> +    this.finishEdit(callback);
> +  }
> +};
> +
> +imageCropEditor.prototype.finishEdit = function(callback) {

I don't think you need edit() or finishEdit() either.

@@ +319,5 @@
> +    case 'nw':
> +      cx = left;
> +      cy = top;
> +      break;
> +    }

You don't need the cases for the corner handles since you'll never use them.

@@ +343,5 @@
> +  var right = region.right;
> +  var bottom = region.bottom;
> +  var aspectRatio = this.cropAspectWidth ?
> +    this.cropAspectWidth / this.cropAspectHeight :
> +    0;

You know that you'll always have an aspect ratio here.

@@ +361,5 @@
> +  else if (hit((left + right) / 2, bottom))
> +    drag('s');
> +  else if (hit(left, (top + bottom) / 2))
> +    drag('w');
> +  else if (!aspectRatio) {

So this else clause will never be used.

@@ +425,5 @@
> +      case 'nw':
> +        newleft = left + dx;
> +        newtop = top + dy;
> +        break;
> +      }

don't need the corner code here.

@@ +519,5 @@
> +
> +
> +// Pass no arguments for freeform 1,1 for square,
> +// 2,3 for portrait, 3,2 for landscape.
> +imageCropEditor.prototype.setCropAspectRatio = function(ratioWidth, ratioHeight) {

I think you should just build this into the constructor since you'll always be using it.

@@ +559,5 @@
> +
> +  // If we're only doing cropping and no editing, then we haven't
> +  // decoded the original image yet, so we have to do that now.
> +  var self = this;
> +  if (this.croponly) {

You're only cropping, but this code does not apply here, since you don't have a separate preview, I think.

::: apps/wallpaper/js/share.js
@@ +17,1 @@
>    var gestureDetector = new GestureDetector(preview);

I don't think you're using this gesture detector any more, are you?  You' have a different on in the crop editor

@@ +25,5 @@
>      url = URL.createObjectURL(blob);
> +    cropEditor = new imageCropEditor(url, preview, {}, function() {
> +     cropEditor.showCropOverlay();
> +     // use window size as limitation size.
> +     cropEditor.setCropAspectRatio(window.innerWidth, window.innerHeight);

In addition to setting the aspect ratio, I really think you'll want to specify a minimum size so the user doesn't crop in too far.

@@ +26,5 @@
> +    cropEditor = new imageCropEditor(url, preview, {}, function() {
> +     cropEditor.showCropOverlay();
> +     // use window size as limitation size.
> +     cropEditor.setCropAspectRatio(window.innerWidth, window.innerHeight);
> +    },null);

Our lint tools will probably complain about no space after the comma on this line.

@@ +34,3 @@
>  
>    function scaleImage() {
> +  cropEditor.getCroppedRegionBlob('image/jpeg',

indentation is off here.

@@ +34,5 @@
>  
>    function scaleImage() {
> +  cropEditor.getCroppedRegionBlob('image/jpeg',
> +                                      window.innerWidth,
> +                                      window.innerHeight,

For devices where there are more device pixels than CSS pixels, we need to return an image that matches the device pixel size of the screen, so multiply by window.devicePixelRatio here.

::: apps/wallpaper/manifest.webapp
@@ +39,4 @@
>        "description": "Gaia Wallpaper Picker"
>      }
>    },
> +  "orientation": "default",

This line is already on master, so you probably created the patch incorrectly

::: apps/wallpaper/share.html
@@ +13,5 @@
>      <script defer src="shared/js/gesture_detector.js"></script>
>      <!-- Specific code -->
> +   
> +   <script type="text/javascript" defer="" src="js/share.js"></script>
> +   <script type="text/javascript" defer="" src="js/imageCropEditor.js"></script> 

trailing white space on lines 15 and 17

Comment 35

[Wayne Chang [:wchang]]

Hi Rob,

Can you chime in here given David's concern re UX in comment 34?

Comment 36

[Rob MacDonald [:robmac]]

(In reply to David Flanagan [:djf] from comment #28)
> 
> Another approach to cropping (completely different than what Gallery does)
> would be to use pinch gestures to zoom in and swipe gestures to pan. We'd
> display the image in a MediaFrame object and at the full size of the screen
> (with cancel and save buttons overlaid). The user would zoom and pan the
> image until they got it just how they wanted it and then click Save, and
> we'd set the wallpaper to just the currently displayed region of the image. 
> Both Gallery and Camera already have code to handle the zooming and panning,
> and it would be a lot simpler to re-use that code than to reuse the entire
> image editor.
> 
> Setting needinfo for Jacqueline to request more UX input here.

Apologies for the delay as Jacqueline is on leave this week. 

I agree with David's suggestion above that, since we're not altering aspect ratios, zooming and panning would be a better approach. Not only is simpler, but it eliminates the need for a preview as the user is seeing the background at the actual size. Esther is looking into this now and will post a UX proposal to this bug in the next couple of days.

Comment 37

[ranjith253]

Hi Rob,

As you know UX scenario for wallpaper crop option already exist in gallery application.
Why con't we use the same UX here also , do you notice any concerns over here?.

I agree reusing of gallery ImageEditor code may be complex , but we have almost implemented this feature with existing code.

Reusing of zooming & panning code from gallery  may have some limitation as i mentioned in reply to Djf Comment 29.

I request you to consider these points also for UX design.

Comment 38

[Wayne Chang [:wchang]]

Attachment: Homescreen_Wallpaper_Cropper

Rob, David,

Currently(1.3) when we go from long pressing on homescreen, "Change Wallpaper" > "Select From: Gallery", when an image is selected the cropper as attached image is shown.

This cropper has a fixed aspect ratio and essentially allows the user to zoom and pan on the image only, but showing the entire image on screen. 

Not sure if there are technical barriers here, can this same experience be reused for consistency? So the user essentially sees the same cropper whether they go from homescreen to set wallpaper, or from gallery.

Comment 39

[Wayne Chang [:wchang]]

ni?robmac for comment 38 :)

Comment 40

[Rob MacDonald [:robmac]]

(In reply to Wayne Chang [:wchang] from comment #38)
> Created attachment 8376067 [details]
> Homescreen_Wallpaper_Cropper
> 
> Rob, David,
> 
> Currently(1.3) when we go from long pressing on homescreen, "Change
> Wallpaper" > "Select From: Gallery", when an image is selected the cropper
> as attached image is shown.
> 
> This cropper has a fixed aspect ratio and essentially allows the user to
> zoom and pan on the image only, but showing the entire image on screen. 
> 
> Not sure if there are technical barriers here, can this same experience be
> reused for consistency? So the user essentially sees the same cropper
> whether they go from homescreen to set wallpaper, or from gallery.

Hi there... NI'ing Jacqueline as she's back this week but here are my preliminary thoughts.

First off, whether the wallpaper selection originates from the home screen or from the gallery application, the crop and resize behaviour should behave the same way. The worst case scenario is that we have one that uses the cropping box and the other with the zooming and panning. I'd rather have the cropping box for both is this is the case.

Second, the zooming and panning solution is a better solution in my view for a couple of reasons. First, it eliminates the need for preview. Second, you can see you wallpaper in its actual size and, third, it doesn't compete with the edge gestures. I also find it better for fine-tuning. 

That said, if we can't get it to perform well, then the cropping tool is an acceptable back-up. Just as long as we're using them in both of the above scenarios (from home screen and from gallery).

Esther is putting together a spec for the pan and zoom scenario. I did see the implementation in the share from gallery scenario and it's close but requires a lot of polish. 

Also, my apologies for not getting to this one sooner. Hopefully we're not too late on this one. Let us know your timelines when you get a chance and we'll try our best to accommodate.

- Rob

Comment 41

[ranjith253]

Attachment: Uploading work in progress patch file for this bug

Hi Rob, Djf,

The proposed UX scenario by Djf (zoom & pan) may be very good option I completely  agree with that .

Considering the amount of time involved in getting UX scenario , implementation & performance check  may not fit in to our timeline .

I am uploading patch which has Djf review comment changes,I know that without proper UX input my patch won't land in master. But we need to work on best possible option available right now.

Comment 42

[ranjith253]

Adding to my above comment.

> First off, whether the wallpaper selection originates from the home screen
> or from the gallery application, the crop and resize behaviour should behave
> the same way. The worst case scenario is that we have one that uses the
> cropping box and the other with the zooming and panning. I'd rather have the
> cropping box for both is this is the case.

Share from gallery & homescreen to gallery the crop behavior is same, We are maintaining  the aspect ratio in both scenario. By considering above points its better to go ahead with current scenario

Comment 43

[Rob MacDonald [:robmac]]

Comment on attachment 8377387 [details] [diff] [review]
Uploading work in progress patch file for this bug

Flagging Jacqueline for this review as the owner for this feature. Thanks Jacqueline!

Comment 44

[Jacqueline Savory [:jsavory] UX]

I seem to struggle with installing packages onto my device through github. Would it be possible for you to provide either a screenshot, video or to package the patch using the instructions below? Thank you!

https://github.com/jrburke/gaia-dev-zip

Comment 45

[Rob MacDonald [:robmac]]

Attachment: Wallpaper cropping target state

While we recognize that a cropping tool is likely necessary in the current version, this attachment represents the future direction of the wallpaper selection tool.

Comment 46

[David Flanagan [:djf]]

Thanks for the fast work on this, Esther, Jacqueline and Rob!

Ranjith: now that we have a spec, I think it will surprise you how easy it is to implement it.  Since we already support panning and zooming in the gallery and camera preview mode, we have lots of re-usable code.

Use shared/js/media/media_frame.js to display the image.

Use shared/js/gesture_detector.js to detect pan and pinch events.

And copy the addPanAndZoomHandlers function from apps/camera/js/lib/panzoom.js to handle the pan and pinch events.  You can remove the orientation code from that function, making it even simpler.

Then, all you need to do, when the user clicks Done is figure out what portion of the image is displayed (probably from the MediaFrame.fit property), copy that part of the image into an offscreen canvas and call toBlob() on it.

media_frame.js and gesture_detector.js were designed for reuse, so a solution based on them will be much cleaner than the approach you've been attempting with the image editor code that was not designed for reuse.

Comment 47

[David Flanagan [:djf]]

Comment on attachment 8377387 [details] [diff] [review]
Uploading work in progress patch file for this bug

Clearing the feedback flag, since I suggested above that we implement this in a different way.

Comment 48

[Wayne Chang [:wchang]]

Hi Sri,
We're going to need a product decision on whether we should move to the new spec in comment 45 or reuse the current crop tool for Madai & 1.4 stage.
Can you provide your comment here so partner can continue the work?

Comment 49

[Sri Kasetti]

We should use the new UX spec attached by Rob in comment 45.
Implementing David's suggestion in comment 46 seems to be simple.

Comment 50

[ranjith253]

Clearing the flag , as the attached patch no longer holds good.

Comment 51

[ranjith253]

Hi,

I am expecting some inputs to handle following issues with existing crop design.

1. In the current design we are not maintaining any aspect ratio , saying that  how can I show a image whose  width   & height are greater then screen width & height?.Consider that i am showing it in a full screen mode if user clicks done button without any crop, the image blob cannot be copied to the canvas  blob which is of device with & height.If we scale down image to device width & height i may get pixelated image.

2.If the image width & height is same as device width & height zoom & pan has no effect , in this scenario user is forced to set the entire image a wallpaper.

3.Consider a image of size 1000 x 800 , how can user can  select a crop region of size 200 X 200 for a particular position (x,y)?. In the previous design it was possible to select a smallest portion (100 x 100) of the image as a crop region.

4.The fit property will always gives the entire width & height of the transformed image, as we are not maintaining aspect ration its tricky to calculate the exact width & height from a position (x,y) in the original image.

I am not sure how easy to handle all the above challenges, I think its better to revisit the previouse  UX design.

Comment 52

[David Flanagan [:djf]]

(In reply to ranjith253 from comment #51)
> Hi,
> 
> I am expecting some inputs to handle following issues with existing crop
> design.
> 
> 1. In the current design we are not maintaining any aspect ratio , saying
> that  how can I show a image whose  width   & height are greater then screen
> width & height?.Consider that i am showing it in a full screen mode if user
> clicks done button without any crop, the image blob cannot be copied to the
> canvas  blob which is of device with & height.If we scale down image to
> device width & height i may get pixelated image.

I don't understand your question. You will have decode the image at full size, and it will be sized and positioned with a CSS transform. When the user clicks the Done button, you'll copy the curently visible region of the image into a canvas and call toBlob().  (In fact, it might even be worth implementing this as a new method of the MediaFrame class).

You won't get anything pixellated if you scale a big image down. Only when you scale a small image up.


> 
> 2.If the image width & height is same as device width & height zoom & pan
> has no effect , in this scenario user is forced to set the entire image a
> wallpaper.

Yes. And I don't think that is a problem. I expect that most users will be using images from the camera which will be quite a bit bigger than the screen.  If we allowed users to zoom in further than that, then you would have a problem with pixellation.
> 
> 3.Consider a image of size 1000 x 800 , how can user can  select a crop
> region of size 200 X 200 for a particular position (x,y)?. In the previous
> design it was possible to select a smallest portion (100 x 100) of the image
> as a crop region.

And, as I've said a couple of times before, I think that was one of the serious flaws with the old design.

If you have an important use case for allowing users to create grainy pixellated wallpapers, you'll have to convince our UX team, but I suspect you won't be able to do that.

> 
> 4.The fit property will always gives the entire width & height of the
> transformed image, as we are not maintaining aspect ration its tricky to
> calculate the exact width & height from a position (x,y) in the original
> image.

I don't understand what you mean by "not maintaining aspect ratio".  I can help with the fit calculations if you show me specific code.  I would be surprised if the numbers you need are not in the fit object.  But if not we can modify MediaFrame as necessary to add what you need.
> 
> I am not sure how easy to handle all the above challenges, I think its
> better to revisit the previouse  UX design.

Comment 53

[ranjith253]

Attachment: wallpapercropapproach.zip

I have two different implementations approach based on the given UX design & comments provided by you for handling the UX design. 

I will try to explain each approach with an example.

Approach 1:

Using existing MediaFrame I will show my original image file of dimension  1680  x 1050 on window screen of width & height are 1080 x 1710. MediaFrame will fit the original image to window screen using css transformation. From here on I will follow UX design to show zoom & pan on the image. I am attaching “OriginalImage.jpg “ which shows this approach.

Limitations 

In the  existing MediaFrame frame work we   don’t have a method in MediaFrame to calculate crop region & copy blob in to canvas .As you mentioned earlier we can implement this as part of MediaFrame.

Query

Consider the example mentioned in the Approach 1, after showing the image preview user clicks done button without any cropping. For preparing the wallpaper blob I need to copy the image (1680 x 1050) in to the canvas (1080 x 1710).This canvas blob is used to set wallpaper image, check the “WallpaperView.jpg” in the attachment for the final wallpaper view. You can notice a stretched effect on the wallpaper view, I think this effect is due to difference in the aspect ratios of image & window screen. 

In summary if the image width & height, screen width & height aspect ratios are different how can we avoid stretched effect on the wallpaper view?

Approach 2:

In this approach we try show the original image (1680 x 1050) in full size as exactly same width & height of the original image (1x zoomed image).I think this scenario we can see while setting wallpaper from gallery to wallpaper in the existing UX .User can zoom-in further & rest of  my approach follows as per UX design.

Limitations

1.MediaFrame has no support for show the 1x zoomed image at first time of displaying the image.
2.Zooming in the image beyond the width & height of image is not supported by MediaFrame.

Query

Consider this scenario, if the user has zoom- out to the maximum then we have to fit the original image (1680 x 1050) in to screen (1080 x 1710) then the image may look same as I mentioned in the  attached file  “OriginalImage.jpg” , then user clicks done button.

1.Do I need to show exactly what I see on the screen (i.e top & bottom of image filled with black) on the wallpaper view?

2.If we are intended to show only the content of the image, I think we will same problem as mentioned in the Approach 1 the stretched effect, because after full zoom-out the crop region will be same original width & height of image (1680 x 1050). How can we handle this?

Apart from above issues I have some logic issues in calculating exact position, width & height of cropped image. Following code can help you in analyzing my logic.

var dx = Math.floor(Math.abs(this.frame.fit.left) / fitscale);
var dy = Math.floor(Math.abs(this.frame.fit.top)/ fitscale);
// frmImgWidth & frmImgHeight are original image width & height
var wd = Math.floor((frmImgWidth - dx));
var ht = Math.floor((frmImgHeight - dy));

// width & height are screen width & height
var limitWidth = Math.floor((width * fitbaseScale) / fitscale );
var limitHeight = Math.floor((height * fitbaseScale) / fitscale );

//ignore the crop region beyond screen width & height.
if(limitWidth <  wd)
{
  wd = limitWidth;
}
if (limitHeight <  ht)
{
  ht = limitHeight;
}
context.drawImage(this.frame.image,dx, dy, wd,
  	              ht, 0, 0, width, height);

For calculating dx & dy positions in the original image I am dividing the fit left & right attributes with current scale value which is floating a value. This way my dx & dy differ by some delta value & it increases as the scale value increases. How can I avoid this?

In summary I am expecting the answers to my queries, feedback on my different approaches & finally the logic handling.

Comment 54

[David Flanagan [:djf]]

Ranjith,

Before I try to figure out your specific questions, I don't understand why you are concerned about a use case where the image is smaller than the screen size.  Won't that be quite rare? Is the problem simpler if you start with a larger image?

Also, rather than attaching a zip file, I'd appreciate it if you'd just attach the images separately, and put any code in a pull request so that I can give you feedback on it through github's commenting system.

Comment 55

[ranjith253]

Hi David,

In my above comment I am trying to explain the problem statement with a use case for better understanding.The issue is same for both large & small images. Following is the exact problem statement

If the image width & height, screen width & height aspect ratios are different how can we avoid stretched effect on the wallpaper view? 

I will be providing pull request as you suggested, I think you can have better understanding after testing my pull request code with different image dimension.

Comment 56

[ranjith253]

Attachment: Pointer to Pull Request.html

Comment 57

[Hema Koka [:hema]]

Adding David for feedback. 

Wayne, is this tracked for madai work? What is the priority of this relative to other madai features

Comment 58

[Wayne Chang [:wchang]]

(In reply to Hema Koka [:hema] from comment #57)
> Adding David for feedback. 
> 
> Wayne, is this tracked for madai work? What is the priority of this relative
> to other madai features

Yes, this is part of madai with lower priority than camera work.

Thanks.

Comment 59

[David Flanagan [:djf]]

Hema and Wayne,

Thanks for clarifying priorities.  Ranjith: your pull request is in my queue and I'll get to it as soon as I can after 1.3-related and camera-related bugs.

Comment 60

[Jacqueline Savory [:jsavory] UX]

Removing feedback query for older version of design.

Comment 61

[Wayne Chang [:wchang]]

Hi David,

Just checking if your queue has freed up a little or would it be better if this is redirected to someone else?

Comment 62

[David Flanagan [:djf]]

Comment on attachment 8385254 [details]
Pointer to Pull Request.html

Ranjith,

Sorry it has taken me so long to look at this.

I'm confused by this proposed patch. I think that this bug is only about the case where we share an image from the gallery and select the "Wallpaper" option.
This missing crop UI is in the Wallpaper app, but you're modifying the crop behavior of the Gallery app.

The UX spec that Rob attached covers all possible wallpaper setting cases, and we may not actually be able to implement all of them.  I believe that the only one in scope for this bug is the share activity case, and when I recommended that you use MediaFrame with the pan and zoom handlers, I wanted you to do that in the wallpaper app.

I believe that there is also an unrelated bug where if you pick a wallpaper image from the camera and take a landscape photo you will get a distorted wallpaper. That has been fixed in our 1.3t branch but may still exist in other branches.  This distortion is not possible when picking from the gallery because the crop UX in that app enforces a fixed aspect ratio.

Comment 63

[David Flanagan [:djf]]

Actually, it looks like I'm wrong about the distorted wallpaper bug when picking a landscape photo from the camera. That bug does not exist on master anymore and is presumably not in 1.4 anymore either.  Let's just keep this bug focused on the share image activity and add cropping to that flow.

Comment 64

[ranjith253]

Hi David,

Thanks for the feedback comments, as per your suggestion I will be working on wallpaper share activity only.Before I can start with this , can you please address my concerns in the comment 53.I believe that logic remains same for both gallery  & wallpaper crop scenarios.

If your are still not clear with my concerns in the comment 53 , please test my logic with different image resolutions in gallery application.

Comment 65

[David Flanagan [:djf]]

Ranjith,

If I understand you (and I'm not sure that I do) you're concerned about the fact that the MediaFrame scales images down so that the entire image fits on the screen.  And this is particularly an issue when displaying a landscape mode photo in portrait mode.  

I think what you need to do is compute the minimum scale factor that is required to make the image fill the screen, and then use the zoom() method of MediaFrame to set that scale.  When you implement event handlers for zooming in and out, you'll want to make sure that you don't allow the user to go below that minimum zoom amount. 

A separate issue is how to handle the case where the original image is smaller than the screen size.  I suggest that you don't do any zooming at all in that case and just use a small wallpaper.  That is better, I think, than a pixelated wallpaper.

Comment 66

[BMO Automation]

Firefox OS is not being worked on