#40152 closed defect (bug) (fixed)
Crop Image button off-screen on mobile
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.7.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Media | Keywords: | has-patch has-screenshots fixed-major |
Focuses: | ui, administration | Cc: |
Attachments (7)
Change History (44)
#5
@
8 years ago
- Keywords has-patch added
- Milestone changed from Awaiting Review to 4.7.4
- Version trunk deleted
#6
@
8 years 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)
#8
follow-up:
↓ 9
@
8 years 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:
↓ 10
@
8 years 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
@
8 years 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.
This ticket was mentioned in Slack in #core by swissspidy. View the logs.
8 years ago
#12
@
8 years 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.
8 years ago
#15
@
8 years ago
- Keywords has-patch added; needs-patch removed
Hi @swissspidy
I have attached patch for the solution of above issue. Please check it.
Thanks
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#17
follow-up:
↓ 20
@
8 years 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
@
8 years 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
@
8 years 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
@
8 years 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.
#21
@
8 years 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.
#22
@
8 years 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.
Thanks
#23
@
8 years 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.
#24
@
8 years 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 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#27
@
8 years 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 years ago
#29
@
8 years 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.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#32
@
8 years ago
40152.3.patch fixes the issue in Chrome.
Tested with 2012 and header image selection/crop in resized window.
#34
@
8 years ago
- Keywords fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
#36
@
5 years ago
Noticed that [40428] added a height: auto;
without removing a height: 60px;
already existing in the same ruleset. The added property just overrides the previous one (which should have been removed).
P.S. Same for the bottom
value:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/css/media-views.css?rev=46866&marks=745-754#L744
Tentative fix, moving skip button to the left