WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 8 months ago

#40152 closed defect (bug) (fixed)

Crop Image button off-screen on mobile

Reported by: Cybr Owned by: swissspidy
Milestone: 4.7.4 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-screenshots fixed-major
Focuses: ui, administration Cc:

Description

Tested on Safari 9 on an iPhone 6S+, confirmed on actual iPhone 7+.

The following CSS line requires a media query to become bigger than 33% (i.e. 100%?):

.media-modal-content .media-toolbar-primary.search-form {
    width: 33%;
}

http://i.imgur.com/zTa0ou3.png

Attachments (7)

40152.diff (565 bytes) - added by karinedo 9 months ago.
Tentative fix, moving skip button to the left
CropImage_button_offscreen_on_mobile_fixed.jpg (54.0 KB) - added by mehul0810 9 months ago.
Patch 40152.diff Works for me
40152.patch (1.2 KB) - added by sagarprajapati 9 months ago.
40152.2.diff (1.0 KB) - added by Cybr 8 months ago.
40152.2.patch (1.7 KB) - added by sagarprajapati 8 months ago.
40152.3.patch (1.7 KB) - added by sagarprajapati 8 months ago.
Remove unnecessary space from the code
40152.3.diff (1.6 KB) - added by swissspidy 8 months ago.

Download all attachments as: .zip

Change History (42)

#1 @adamsilverstein
9 months ago

  • Keywords good-first-bug added

@karinedo
9 months ago

Tentative fix, moving skip button to the left

#2 @swissspidy
9 months ago

This sounds like a duplicate of #40132, which in turn was marked as a duplicate of #30634. I'm not sure it's really the same issue though.

#3 @Cybr
9 months ago

#30634 is about moving/resizing the selection field.
#40132 seems to be the same as this ticket, but on a different device.

#4 @swissspidy
9 months ago

#40132 was marked as a duplicate.

#5 @swissspidy
9 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.7.4
  • Version trunk deleted

@mehul0810
9 months ago

Patch 40152.diff Works for me

#6 @mehul0810
9 months ago

  • Resolution set to worksforme
  • Status changed from new to closed

Checked Patch 40152.diff and it works for me. Please check attached screenshot (i.e. CropImage_button_offscreen_on_mobile_fixed.jpg)

#7 @ocean90
9 months ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

#8 follow-up: @Cybr
9 months ago

Setting it to 100% will cause all buttons to disappear if div with class media-toolbar-secondary is filled in with anything on all screens.

#9 in reply to: ↑ 8 ; follow-up: @adamsilverstein
9 months ago

Replying to Cybr:

Setting it to 100% will cause all buttons to disappear if div with class media-toolbar-secondary is filled in with anything on all screens.

@Cybr I don't quite understand your comment, what do you mean by " if div with class media-toolbar-secondary is filled in with anything", can you give an example or steps to reproduce?

#10 in reply to: ↑ 9 @Cybr
9 months ago

Replying to adamsilverstein:

@Cybr I don't quite understand your comment, what do you mean by " if div with class media-toolbar-secondary is filled in with anything", can you give an example or steps to reproduce?

I can't give you an example, because I don't really understand the correct structure for third party implementation on this part.

However, what I do know is that there are two div elements inside .media-frame-toolbar > .media-toolbar:

<div class="media-frame-toolbar">
	<div class="media-toolbar">
		<div class="media-toolbar-secondary"></div>
		<div class="media-toolbar-primary search-form">
			<button type="button" class="button media-button button-secondary button-large media-button-skip">Skip Cropping</button>
			<button type="button" class="button media-button button-primary button-large media-button-insert">Crop Image</button>
		</div>
	</div>
</div>

When you fill in .media-toolbar-secondary with any flow-forming content (like text), and set .media-toolbar-primary to 100%, then media-toolbar-primary will overflow.
This is because media-toolbar-secondary is output first (i.e. it should be the 67% of the left side) and there's no room for a second row.

Last edited 9 months ago by Cybr (previous) (diff)

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


9 months ago

#12 @swissspidy
9 months ago

  • Keywords good-first-bug removed
  • Owner set to swissspidy
  • Status changed from reopened to assigned

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


9 months ago

#14 @swissspidy
9 months ago

  • Keywords needs-patch added; has-patch removed

The patch works well as long as the buttons aren't too wide. When using de_DE, it looks like this:

https://cldup.com/Ixu2dQ1Xe7.png

https://cldup.com/MQh6e6iYdJ.png

I haven't tested RTL support just yet, but of course this needs to work there as well.

#15 @sagarprajapati
9 months ago

  • Keywords has-patch added; needs-patch removed

Hi @swissspidy and All

I have attached patch for the solution of above issue. Please check it.

Thanks

Last edited 9 months ago by sagarprajapati (previous) (diff)

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


9 months ago

#17 follow-up: @swissspidy
8 months ago

Thanks @sagarprajapati! This patch indeed resolves that issue. It's just unfortunate that the bottom bar is now 120px high even if the buttons are on one line. But that whole media CSS is a rabbit hole it seems…

#18 @sagarprajapati
8 months ago

Hi @swissspidy we should cut the long text using CSS and set ... After few later.

For example if text of the button is 'the button text is too long' then we should set it to 'the button..'.

#19 @swissspidy
8 months ago

Truncating text isn't really a11y-friendly and also confusing for users and translators. Fixing the layout is the better approach.

#20 in reply to: ↑ 17 @Cybr
8 months ago

Replying to swissspidy:

But that whole media CSS is a rabbit hole it seems…

This is where CSS3 flexbox comes in handy. But only if you wish to drop IE10 support, though.

Alternatively, use inline-block and/or tables; where there's a min-height with padding. But, no max-height nor default height.

Edit: I'll try to brew up something from these suggestions... might take an hour or so.

Last edited 8 months ago by Cybr (previous) (diff)

@Cybr
8 months ago

#21 @Cybr
8 months ago

Well, I was introduced to the awful world of absolute alignment. This essentially means it has no room for changes.
Nevertheless, the attached diff makes it work on all screens as originally intended, with mobile and secondary toolbar support:

If it overflows (not shown in the images) then the secondary toolbar will be slightly moved upwards.
Unfortunately, because we're working with absolute alignment, the rest won't flow with it.

But, it's much better (and at the very least usable again) compared to before.

http://i.imgur.com/jg57Fv7.jpg
http://i.imgur.com/ksfDyv8.jpg
http://i.imgur.com/57XlGYU.jpg
http://i.imgur.com/AQ8exgf.jpg

Last edited 8 months ago by Cybr (previous) (diff)

#22 @sagarprajapati
8 months ago

Hi @Cybr

Patch work well when button is not too wide. when button is wide it will go up in the iPhone 5. Please check screenshot.

http://imgur.com/mzZf3FQ

http://imgur.com/a/BWgdA

Thanks

#23 @Cybr
8 months ago

@sagarprajapati Yes! Exactly what I meant with:

If it overflows (not shown in the images) then the secondary toolbar will be slightly moved upwards.

But it also affects the primary bar :)

There just isn't enough space.

And because of the absolute values, the bottom bar has to be expanded vertically and statically (media queries) on mobile to support the occasional i18n hick-up.
Alternatively, it requires a JS overflow listener to adjust the heights. Or, even a complete rewrite from absolute values to real flow.

I don't believe we can get closer to a resolution than this without doing any of the above.

Last edited 8 months ago by Cybr (previous) (diff)

#24 @sagarprajapati
8 months ago

Hi @Cybr and @swissspidy

I have updated the patch and now if the text of the button is larger then the first will move up with proper spacing.

http://imgur.com/a/IH3gy
http://imgur.com/a/aLhMs
http://imgur.com/a/DCzNz

Thanks

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


8 months ago

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


8 months ago

@sagarprajapati
8 months ago

Remove unnecessary space from the code

#27 @mayurk
8 months ago

  • Keywords has-screenshots added

Hi All,

I have tested latest patch on the following devices and browsers. I have also attached screenshot for review. Please check it.

1) Android Firefox
2) Android Chrome
3) Ipad Chrome
4) Ipad Firefox
5) Iphone6 Chrome
6) Iphone6 Firefox

Screenshots:

https://www.dropbox.com/s/xu0q0ybbwuv87ru/android%20chrome.png?dl=0
https://www.dropbox.com/s/w58c0umkmk3400d/android%20firefox.png?dl=0
https://www.dropbox.com/s/522a29ggwxqp5yi/2017-04-03.png?dl=0
https://www.dropbox.com/s/i0yoi4doh7lwhzf/2017-04-04.png?dl=0
https://www.dropbox.com/s/2pe7s4nmrlpbtjm/2017-04-06.png?dl=0
https://www.dropbox.com/s/qhf86800j9t8doh/2017-04-07.png?dl=0

Thanks

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


8 months ago

#29 @mayurk
8 months ago

  • Keywords has-patch has-screenshots removed

Hello,

I just checked latest patch on the browserstack tool & it's completely working fine. I am still ready for any suggestion and ready for any help.

#30 @mayurk
8 months ago

  • Keywords has-patch has-screenshots added

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


8 months ago

#32 @mikeschroder
8 months ago

40152.3.patch fixes the issue in Chrome.
Tested with 2012 and header image selection/crop in resized window.

@swissspidy
8 months ago

#33 @swissspidy
8 months ago

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

In 40428:

Media: Ensure Crop Image is always visible.

Previously, the crop button in the media modal after uploading header images or similar was hidden and the task could not be completed.

Props karinedo, sagarprajapati, Cybr, mayurk.
Fixes #40152.

#34 @swissspidy
8 months ago

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

#35 @swissspidy
8 months ago

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

In 40429:

Media: Ensure Crop Image is always visible.

Previously, the crop button in the media modal after uploading header images or similar was hidden and the task could not be completed.

Props karinedo, sagarprajapati, Cybr, mayurk.
Fixes #40152.

Merges [40428] to the 4.7 branch.

Note: See TracTickets for help on using tickets.