WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#41242 closed defect (bug) (fixed)

Image crop not working in mobile screen.

Reported by: yahil Owned by: adamsilverstein
Milestone: 4.9.5 Priority: normal
Severity: normal Version:
Component: Media Keywords: needs-patch commit fixed-major
Focuses: ui, administration Cc:

Description

Hello Team,

I am trying to crop the image from a mobile device, but it's not working. is there any solution?

Dashboard -> media -> select media -> edit image

Attachments (2)

41242.diff (1.0 KB) - added by adamsilverstein 4 years ago.
41242.2.diff (20.2 KB) - added by adamsilverstein 4 years ago.

Download all attachments as: .zip

Change History (36)

#1 follow-up: @adamsilverstein
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Hello, @yahil and thanks for the bug report.

Can you give some more specific information about the device/os/screen size you are using? Also, how are you reaching the crop feature, from the media library?

I tried testing this and saw several issues including the cropper not working and the details screen overlay blocking/covering the grid.

#2 @melchoyce
4 years ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #30634.

#3 @adamsilverstein
4 years ago

@melchoyce thanks for linking this up with the other ticket, I was guessing there was already one open.

I wonder if the reporter was referring to cropping in the customizer/site icon, or cropping from the media grid/media manager (click an image, then click edit). in the media manager context i tested and couldn't get cropping to work at all, i can't even start the selection process. in addition, there is a ui problem where the image details pane covers part of the grid.

#4 in reply to: ↑ 1 @yahil
4 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Yes, sure.
I am using a phone with below specifications:
Phone: Xiaomi Mi 4i smartphone
OS: Android
Display: 5.00-inch touchscreen

This issue is occurring with all mobile devices ( as per my knowledge ). However, it is working fine with desktop screens.

I think we should keep the issue open until the discussion on going.

Thanks.

Replying to adamsilverstein:

Can you give some more specific information about the device/os/screen size you are using? Also, how are you reaching the crop feature, from the media library?

#5 @yahil
4 years ago

@adamsilverstein @melchoyce here is step to reproduce the issue:

  1. Upload Media From Media tab
    • wp-admin -> Dashboard -> Media -> Add New
  1. Select Media [ media will open in popup/model ]
  1. Click on edit media [ Now there is option to crop, rotate left, rotate right, ..... ]

We can crop the image using double click but same things can't do in a mobile device.

If I am wrong, right me.
Thanks.

This ticket was mentioned in Slack in #core-media by karmatosed. View the logs.


4 years ago

#7 @adamsilverstein
4 years ago

I tested the patch for #30155 here and found I can now enter the cropping mode by clicking on the crop button, then the tool works as expected. Still looking into why the initial drag does nothing. One other note: when entering the edit image mode as you described, essentially from the media details page, the image itself sits below the image details section. When entering image edit mode from the media grid I see something different - the image sits below the image details, and is partially obscured. Probably need some css adjustment here:


Entering from media grid/manager:

https://cl.ly/0z3n2e2j3y1x/developwordpress.localhost-wp-admin-upload.php(iPhone%206).png


Entering from media edit screen:

https://d3vv6lp55qjaqc.cloudfront.net/items/423i143c1i0h08191z2j/developwordpress.localhost-wp-admin-post.php-post=62&action=edit(iPhone%206).png

#8 follow-up: @adamsilverstein
4 years ago

@yahil - can you please give 41242.diff a test to see if this resolves your issue?

#9 @adamsilverstein
4 years ago

in 41242.diff:

  • in initCrop, handle touch events in addition to mouse events
  • in imageSelect jQuery plugin, accept event.which of 0 which is what is provided by touch events

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


4 years ago

#11 in reply to: ↑ 8 @alexgso
4 years ago

I can confirm 41242.diff works for me.

For reference, I'm able to replicate this issue on a Microsoft Surface Laptop which features a touchscreen.

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


4 years ago

#13 @desrosj
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#14 @joemcgill
4 years ago

  • Keywords needs-patch commit added; has-patch needs-testing removed
  • Owner set to adamsilverstein
  • Status changed from reopened to assigned

41242.diff looks good to me. Will need to have the build files generated before/during committing.

#15 @SergeyBiryukov
4 years ago

  • Milestone set to 5.0

#16 @adamsilverstein
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42818:

Media: Fix image cropping on touch screen devices.

  • In initCrop, handle touch events in addition to mouse events.
  • In imageSelect jQuery plugin, accept event.which of 0 as provided by touch events.

Props yahil, alexgso, joemcgill.
Fixes #41242.

#17 @adamsilverstein
4 years ago

@SergeyBiryukov Saw you tagged this 5.0... do you think this is worth backporting or including in 4.9.5? Before this changing media cropping does not work correctly for touch devices (you need a mouse).

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


4 years ago

#19 @audrasjb
4 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to ship it with 4.9.5.

#20 @audrasjb
4 years ago

  • Milestone changed from 5.0 to 4.9.5

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


4 years ago

#22 @adamsilverstein
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 42874:

Media: Fix image cropping on touch screen devices.

  • In initCrop, handle touch events in addition to mouse events.
  • In imageSelect jQuery plugin, accept event.which of 0 as provided by touch events.

Props yahil, alexgso, joemcgill.
Merges [42818] to the 4.9 branch.
Fixes #41242.

#23 @mermel
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I noticed on the 4.9.5 RC released last night it looks like the minified file for wp-includes/js/imgareaselect/jquery.imgareaselect.js might be missing from this commit:
https://build.trac.wordpress.org/changeset/42704

#24 @adamsilverstein
4 years ago

Ah, thanks @mermel it looks like I missed building that file before the initial commit, will follow up to correct this.

#25 @SergeyBiryukov
4 years ago

@mermel Great catch! Looks like we don't have a build task for that file, and it has to be minified manually, like the last time it was changed in [33329].

#26 follow-up: @joemcgill
4 years ago

@adamsilverstein and @SergeyBiryukov – Can we automate the build process for this file?

#27 @adamsilverstein
4 years ago

Aha, well that explains why i might have missed it. Same question exactly .

#28 @SergeyBiryukov
4 years ago

I guess it was originally treated as an upstream library, but it hasn't been active in 5 years. Now that it was patched in [33329] and [42818], I agree we should add a build task.

#29 in reply to: ↑ 26 ; follow-up: @azaozz
4 years ago

Replying to joemcgill:

@adamsilverstein and @SergeyBiryukov – Can we automate the build process for this file?

This is still an external package/library. That's why the minified file is committed together with the source.

#30 in reply to: ↑ 29 @adamsilverstein
4 years ago

Replying to azaozz:

This is still an external package/library. That's why the minified file is committed together with the source.

Right, and the external library is no longer maintained and we are patching the source code. Thats why our build process should create the minified file, so committers don't have to manually build the file.

#31 follow-up: @adamsilverstein
4 years ago

In 41242.2.diff:

  • Add imgareaselect to uglify grunt process, builds wp-includes/js/imgareaselect/jquery.imgareaselect.min.js
  • updated jquery.imgareaselect.min.js

Does this look good @SergeyBiryukov ?

#32 in reply to: ↑ 31 @SergeyBiryukov
4 years ago

Replying to adamsilverstein:

Add imgareaselect to uglify grunt process

Should we also add it to precommit:js, next to uglify:masonry?

#33 @ocean90
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 42930:

Media: Update minified version of imgAreaSelect after [42874].

Adds imgAreaSelect to the uglify grunt process.

Props adamsilverstein.
Fixes #41242.

#34 @ocean90
4 years ago

In 42932:

Media: Update minified version of imgAreaSelect after [42874].

Adds imgAreaSelect to the uglify grunt process.

Merge of [42930] to the 4.9 branch.

Props adamsilverstein.
See #41242.

Note: See TracTickets for help on using tickets.