Skip to content

Conversation

@anthonyburchell
Copy link
Contributor

@anthonyburchell anthonyburchell commented Sep 29, 2019

Description

https://core.trac.wordpress.org/ticket/48028
Excerpt from the ticket above:

Fixes: #17642.

The "Media Library" panel of the "Featured Image" modal dialog contains a link labelled with "Edit image". This link takes users to a new window/tab, but that link does not inform users that it opens a new window or tab.
A link does not naturally convey itself to assistive technology that it opens in a new tab without link text that describes it.
It is important to convey that a link opens in a new tab because of the change of context may not be clear to everyone.

Screen Shot 2019-09-29 at 4 35 55 PM

## Types of changes/ How has this been tested?

This PR adds a new editState function to initialize the available edit frames. This function also respects the allowedTypes prop and has been tested with the video, gallery, and featured image media flows. Additional testing there would be appreciated.

PENDING BUG: edit image state for gallery seems to be messing up because of the multiple nature of this screen. Leaving this draft until I have a fix.

@anthonyburchell anthonyburchell changed the title adds edit state to frames to fix new tab issue CORE 48028 adds edit state to frames to fix new tab issue Sep 29, 2019
@anthonyburchell anthonyburchell changed the title CORE 48028 adds edit state to frames to fix new tab issue 17642 adds edit state to frames to fix new tab issue Sep 30, 2019
@anthonyburchell anthonyburchell marked this pull request as ready for review September 30, 2019 02:15
@antpb antpb added the [Feature] Media Anything that impacts the experience of managing media label Sep 30, 2019
@antpb antpb added this to the WordPress 5.x milestone Sep 30, 2019
anthonyburchell added 2 commits October 11, 2019 12:53
…re. Also removes the declaration of the button text as the default already is 'select'
@anthonyburchell
Copy link
Contributor Author

The Core side changes have been merged in https://core.trac.wordpress.org/changeset/46461

No need to apply the Core patch now. Just test this PR and all should be working again.

@gziolo
Copy link
Member

gziolo commented Oct 14, 2019

For the record, in master when clicking on Edit Image it would open the link in the new tab. This PR ensures that Edit Image screen loads in the Media modal. In general, it works great 👍

I tested it with the latest WordPress core changes in trunk. There is some edge case where you see only a blank page in the modal after clicking on the Edit Image link. I'm not 100% sure how to reproduce it, but it feels like it happens after you close the modal on the Edit Image page and you try to edit the same image from another part of the UI:

edit-image

I also can observe that there is some unexpected behavior with the focus when navigating back from editing the image when you want to select the featured image:

Screen Shot 2019-10-14 at 13 23 31

Somehow, the visual focus stays on the Back button which is no longer there.

@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Oct 14, 2019
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small stylistic comments, but the code overall looks good to me.

@gziolo
Copy link
Member

gziolo commented Oct 15, 2019

@anthonyburchell - we should figure out why you see this white screen inside the modal from time to time before we proceed.

There is also this question, whether we should remove target="_blank" from the Edit Image link as it seems to be no longer necessary.

@afercia
Copy link
Contributor

afercia commented Jan 31, 2020

The related Trac ticket is milestoned for WordPress 5.4. See https://core.trac.wordpress.org/ticket/48028

Any chance to get this rebased, reviewed, and if everything goes well, merged? /Cc @youknowriad @anthonyburchell @joemcgill

@anthonyburchell
Copy link
Contributor Author

Thanks for the ping @afercia! I rebased and fixed some linting issues. Looks like that is passing now.

The feedback provided in this comment is still valid. It is possible to get a blank white screen after clicking edit in one frame (gallery) and opening edit in another (maybe featured). I still have some investigation to get that solved. It's a pretty edge case instance so I leave it to y'all to thumbs up moving forward and making a separate issue to solve that.

@anthonyburchell
Copy link
Contributor Author

Also, it may be worth mentioning that I don't think this needs to be included in Beta 1. This is a bug fix to a broken part of the edit image functionality so maybe we can land this for Beta 2?

@antpb antpb added the Backport to WP 6.9 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 13, 2020
@anthonyburchell
Copy link
Contributor Author

An update here, I found that the edit image bug has existed since before this PR. I think that calls for a trac side change to fix the close action. To reproduce, click edit image on a selected image in the gallery library modal, close window. open another frame (featured image maybe), attempt to edit same image, see white screen. It's possible to do this in older versions.

With that in mind, I dont see any blockers to merging this PR before 5.4 Beta 2. Please let me know if theres anything I can do to bring it to 100%. :)

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it again and I couldn't reproduce issues listed a few months back. Nice work. Let's get it in 🎉

@gziolo gziolo merged commit 78de49e into WordPress:master Feb 16, 2020
@gziolo
Copy link
Member

gziolo commented Feb 16, 2020

With that in mind, I dont see any blockers to merging this PR before 5.4 Beta 2. Please let me know if theres anything I can do to bring it to 100%. :)

@jorgefilipecosta - I added this PR to the list of PRs to include in WP 5.4 release.

jorgefilipecosta pushed a commit that referenced this pull request Feb 17, 2020
* adds edit state to frames to fix new tab issue

* moves default state build into else of gallery condition and adds gallery toolbar logic to reach parity with Core.

* fixes selection object reference to proper state

* improves and adds documentation

* removes buildAndSetEditFrame as it should be added in core and not here. Also removes the declaration of the button text as the default already is 'select'

* truly removes editState fu function

* fixes linting issues

* Adjusts PR to factor in review suggestions
jorgefilipecosta pushed a commit that referenced this pull request Feb 17, 2020
* adds edit state to frames to fix new tab issue

* moves default state build into else of gallery condition and adds gallery toolbar logic to reach parity with Core.

* fixes selection object reference to proper state

* improves and adds documentation

* removes buildAndSetEditFrame as it should be added in core and not here. Also removes the declaration of the button text as the default already is 'select'

* truly removes editState fu function

* fixes linting issues

* Adjusts PR to factor in review suggestions
@mcsf
Copy link
Contributor

mcsf commented Feb 18, 2020

@anthonyburchell @gziolo @jorgefilipecosta:
There are two bugs introduced in the Gallery block by this PR:

Gallery: can't insert select items from media library

gallery-media-insert

Also notice that, when an image is selected in the media modal, the action button at the bottom-right corner reads "Insert gallery" instead of "Create a new gallery".

Gallery: can't edit collection of images

  1. Open a post that has a Gallery block with images.
  2. Select the block.
  3. Click on "Media Library" underneath the images.
    Expected: The media modal opens up.
    Observed: Nothing happens in the UI, and a TypeError is raised:
TypeError: t is null                      media-views.min.js:2:7343

@anthonyburchell
Copy link
Contributor Author

@mcsf you rock. Thank you for finding this. Here is the fix for the issue you identified: #20287

Would greatly appreciate some testing on this to verify gallery works as intended. 🙏🏼

@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.9 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Media Anything that impacts the experience of managing media [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Media Library 'featured image' dialogue missing link text that describes 'opens in new tab'.

8 participants