Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#35191 closed defect (bug) (fixed)

Remove title attributes: the Tools > Import screen

Reported by: afercia's profile afercia Owned by: ocean90's profile ocean90
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.4
Component: Import Keywords: has-screenshots, has-patch, title-attribute
Focuses: ui, accessibility Cc:

Description

See related #24766 and all the following tickets about title attributes.

In the Import screen, users can install and run plugins ("importers") to import posts or comments from other systems. This is a very unique screen in WordPress, with some peculiarities.
The title attributes on the plugin name are the only way the importer status (installed, not installed, active...) is communicated to users. These title attributes are also "dynamic", meaning they change depending on the plugin status. See screenshot below:

https://cldup.com/uc_szP3Wp3.png

It is worth reminding that only some user groups can access the information provided by title attributes and other user groups won't have any clue about the importer status. Since it's a relevant information, should be provided in plain text.

Also, the descriptions could be improved: please notice that even when an importer is installed, the description still says "install". When it's installed and activated, the description is different (it comes from the plugin main file headers). This is more clear in the second screenshot below:

https://cldup.com/4mtpkmoQsG.png

I'd propose to consider to:

  • get rid of the table used for layout purposes
  • use more semantic markup: it's a list of plugins, so should be marked up as a list (screen readers will announce the number of items in the list)
  • display the importer plugin status in plain text
  • unify the descriptions from the wp.org API, the ones in the core, and the ones in the plugin headers

Would greatly appreciate feedback and help aboht the visual part from UI and design people :)

Attachments (14)

35191.patch (8.7 KB) - added by afercia 8 years ago.
35191.2.patch (8.2 KB) - added by afercia 8 years ago.
35191.3.patch (8.1 KB) - added by afercia 8 years ago.
import-subsubsub-nav.png (6.0 KB) - added by ramiy 8 years ago.
Maybe we can add sub navigation?
35191.4.patch (8.5 KB) - added by afercia 8 years ago.
35191.3.diff (9.6 KB) - added by swissspidy 8 years ago.
35191.4.6.diff (9.5 KB) - added by afercia 8 years ago.
35191.9.diff (12.9 KB) - added by swissspidy 8 years ago.
35191.10.diff (12.6 KB) - added by ocean90 8 years ago.
35191.10.png (264.1 KB) - added by ocean90 8 years ago.
35191.10-mobile.png (232.2 KB) - added by ocean90 8 years ago.
35191.11.diff (14.5 KB) - added by ocean90 8 years ago.
35191.12.diff (15.7 KB) - added by ocean90 8 years ago.
35191.13.diff (19.8 KB) - added by afercia 8 years ago.

Download all attachments as: .zip

Change History (63)

@afercia
8 years ago

#1 @afercia
8 years ago

  • Keywords has-patch ui-feedback added

First pass. Tried to implement all the items above. It's a very initial step, with the content basically unstyled. Designers welcome!

https://cldup.com/-juCzc2vGy.png

#2 @afercia
8 years ago

  • Owner set to afercia
  • Status changed from new to assigned

This ticket was mentioned in Slack in #design by afercia. View the logs.


8 years ago

#4 @afercia
8 years ago

Recap: discussed on Slack, this ticket needs a first design proposal to move on. cc @michaelarestad @melchoyce

#5 @afercia
8 years ago

To recap: blocked by lack of a design proposal :(

#6 @GaryJ
8 years ago

  • Keywords needs-patch added; has-patch removed

The patch is going to need some screen reader text for each of the links, to distinguish them from each other when plugins are at the same state. i.e.

Run the<span class="screen-reader-text"> RSS</span> importer

<span class="screen-reader-text">Blogroll </span>Details and Installation

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


8 years ago

#8 @chriscct7
8 years ago

  • Milestone changed from 4.5 to Future Release

Punted per discussion in Slack.

#9 @afercia
8 years ago

To clarify after the discussion on Slack. Whether or not this screen will use a table for layout purposes (which is, well... primitive) there's the need of a design proposal anyways. The plugin state will be displayed in plain text so there's the need to make a decision, at a very least, about where this text should be displayed and how should be displayed.
Ideally, it should be as close as possible to the importer plugin name. Maybe highlighted in some way, since the "state" of an importer is an essential information for interaction.

#10 @afercia
8 years ago

Out of curiosity, tried a second version keeping the current table, see below:

https://cldup.com/xY1vdupiqL.png

At this point, wondering if this table should look as similar as possible to the table in the Plugins screen. Any thoughts welcome.

@afercia
8 years ago

#11 @afercia
8 years ago

  • Keywords has-patch added; ui-feedback needs-patch removed
  • Milestone changed from Future Release to 4.6

Refreshed patch, keeping the existing table as per the received (a while ago to be honest) design feedback. See the screenshot below. Importers plugins may have (like all plugins) three different states:

  • not installed
  • installed but not activated
  • installed and activated

Depending on their state, the available action is different. Instead of hiding all these information with a title attribute, everything is now visible and clear, in plain text. Any thoughts more than welcome.

https://cldup.com/8rxHdassUU.png

This ticket was mentioned in Slack in #design by afercia. View the logs.


8 years ago

#13 in reply to: ↑ 12 @mapk
8 years ago

Replying to slackbot:

At this point, wondering if this table should look as similar as possible to the table in the Plugins screen. Any thoughts welcome.

It makes sense to have this table look similar to the Plugins table seeing as these are basically plugins, right? With this thought in mind, do we need to add table headers, and plugin icons? Are there plugin icons for these?

Should the action links reside under the Plugin name like this?

https://cldup.com/OC04ftAO_C.png

Version 0, edited 8 years ago by mapk (next)

This ticket was mentioned in Slack in #design by hugobaeta. View the logs.


8 years ago

@afercia
8 years ago

#15 @afercia
8 years ago

35191.3.patch changes the link and "state" text position. As suggested, having the action link below the object name is a quite common pattern in WordPress. I'd like to remind this ticket is just about removing title attributes and consequently try to display the new link and text in the best possible way. Also, the wording could be improved. Further design improvements to this screen should preferably go in a separate ticket. Screenshot with the new patch applied:

https://cldup.com/6hrh3ePcDu.png

#16 @afercia
8 years ago

Discussed this ticket at Vienna's Contributors Day with @karmatosed and @hugobaeta and agreed the wording should be improved :) Agreed also to consider the proposed patch as a first step to bring some love to this screen. Major UI changes should go in a separate ticket for future iterations.
As soon as there's feedback about some better wording and the patch gets refreshed, I'd say this is ready for commit consideration.

#17 @karmatosed
8 years ago

I agree about it being a first patch as we all discussed. I think perhaps that at this stage is worth getting in over word changes. Those can come.

Last edited 8 years ago by karmatosed (previous) (diff)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

@ramiy
8 years ago

Maybe we can add sub navigation?

#19 @afercia
8 years ago

Maybe we can add sub navigation?

Yep, as mentioned also in previous comments, it would probably make sense to have this table look similar to the Plugins table, including the view links. By the way this would require to use the List Tables and it's definitely a major refactoring, a bit outside the scope of this ticket which simsply aims to remove the title attributes :)

@afercia
8 years ago

#20 @afercia
8 years ago

Was thinking one of the issues for the weird wording (and the install workflow) is that there's just one link to install and it first opens the details modal, then users can install from the modal. Using as link text "Install" wouldn't be so correct because it doesn't install... on the other hand, "Details and installation" sounds weird.
Splitting the link in two separate links "Install" and "Details" would allow to simplify and improve a bit the workflow:

  • users wouldn't be forced any longer to open the details modal, they can choose to install straight away or see the details
  • the wording would be simpler and effective
  • visually, the links would use a more familiar pattern, as for all the other action links

Would appreciate any feedback and a review about the install link :)

Screenshot:

https://cldup.com/ABIdUU_-Eg.png

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

This ticket was mentioned in Slack in #design by afercia. View the logs.


8 years ago

#23 @melchoyce
8 years ago

Looking better each iteration. :)

Can the "status" (not installed / installed and activated / etc.) get some sort of visual treatment? Either bold, or maybe italic? Something to highlight that it's a status and differentiate it from the description.

#24 @swissspidy
8 years ago

Quick reminder that we need to keep Shiny Updates in mind on this screen :-)

Having an Install/Activate button for each importer would make things a bit easier in that regard and make it look more familiar with the rest of the admin where things can be installed and activated.

#25 @afercia
8 years ago

@melchoyce 
Definitely yes :) also wondering if the status would look better before the description or after. Will try a couple of options.

@swissspidy
Got it :) Would appreciate your eyes on the install link (nonce, add query arg, escaping, etc,) to be sure I'm not messing up things :)

@swissspidy
8 years ago

#26 @swissspidy
8 years ago

Sure. 35191.3.diff fixes a few bugs in the previous patch and uses add_query_arg() and sprintf() more consistently. Not shiny yet though.

Having two links — Install and Details — makes sense. It's more consistent with the Add New Plugin screen. Still think there should be a button for installing though.

Also, I'd change the wording a bit, i.e. "Active" instead of "Installed and activated" and "Inactive" instead of "Installed and not activated". Should at least be "… (in)active" instead of "… (in)activated", but "Active" and "Inactive" is more in line with the the admin screen.

#27 @afercia
8 years ago

  • Keywords needs-refresh added

Okay, can't upload a new patch due to continuous "database is locked" errors on Trac. Will comment and try again tomorrow.

Thanks @swissspidy nice touch! Noticed a few issues, the most important about the "Details" link introduced in the previous patch. When an importer is installed and active, the plugin slug is missing from its data so the Details link will be wrong. This is caused by the previous loop that builds the $importers array and just skips the already installed importers. As far as I see there are two options:

  • don't display the "Details" link when an importer in installed and active
  • always set the plugin slug in the previous loop before the two continue

I'd be fine with the former but would appreciate any feedback. Also, fixed a couple of typos and simplified the links and status text.

Not sure about the changes in pluggable.php cc @swissspidy

About the visual part, tried a couple of different options. Worth considering the "status text" could also be completely removed. Just let me know and sorry if the screenshot are a bit big as file size.

I'd recommend to open new, separate, tickets for shininess and further accessibility improvements (aria-labels, etc.) since we're already far beyond the scope of this ticket :)

https://cldup.com/GflzSuTnE0.png

https://cldup.com/GHcoPCWKmt.png

@afercia
8 years ago

#28 @afercia
8 years ago

  • Keywords needs-refresh removed

Finally succeeded in uploading a new patch, it was named 35191.4.diffbut I see it got renamed in 35191.4.6.diff... don't ask me where are the other 5 ones :)

#29 follow-up: @swissspidy
8 years ago

Whoops, pluggable.php should be left unchanged. Must have slipped in somehow.

Regarding the shinyness: In trunk this screen is already shiny. Why break it and open a new ticket to fix it again?

#30 in reply to: ↑ 29 @afercia
8 years ago

Replying to swissspidy:

Regarding the shinyness: In trunk this screen is already shiny. Why break it and open a new ticket to fix it again?

Ops, I've missed it was already shiny. I'm all for adding shininess to the new link then :)

@swissspidy
8 years ago

#31 @swissspidy
8 years ago

No worries. 35191.9.diff makes the new design shiny.

Oops, I now see that I need to fix the docs in updates.js, so please do not commit that as-is yet :)

#32 @afercia
8 years ago

35191.9.diff works like a charm :) However, when installing an importer, the "status" text should be updated from "Not installed" to "Inactive" or maybe consider to remove it entirely"

#33 @swissspidy
8 years ago

+1 to removing that status altogether for more consistency. The links should already clearly indicate the current status IMHO.

@ocean90
8 years ago

@ocean90
8 years ago

#34 @ocean90
8 years ago

+1 to removing the status.

35191.10.diff:

  • Removes the extra status line.
  • IMO we can also combine "Activate" and "Run" to "Run Importer" since both actions start the importer. (De)activation is something for wp-admin/plugins.php.
  • Instead of having the importer name in bold I've increased the font size and changed the font color.

Edit: Is the change to wp_get_popular_importers() necessary? If so, we have to update the API (https://meta.trac.wordpress.org/browser/sites/trunk/api.wordpress.org/public_html/core/importers/1.0/index.php#L13) too, based on the WP version the user is using.

Last edited 8 years ago by ocean90 (previous) (diff)

#35 follow-up: @afercia
8 years ago

I was avoiding to use "Run Importer" because the "Categories and Tags Converter" doesn't actually "import" anything but "converts" something :) Oh well it's a minor thing, but wondering why this converter should be listed in a screen dedicated to "importing" in the first place.

About the fonts change: nice touch, and consistent with the Plugins screen.

Edit: Is the change to wp_get_popular_importers() necessary? I

Not strictly necessary for removing the title attributes :) Just an attempt to make the description consistent with the plugin state. Having a plugin installed and the description that still says "Install the bla bla..." can be a bit confusing.
By the way, there are various inconsistencies between the plugin headers data, the descriptions from wp_get_popular_importers() and the API, even in the plugins name see for example "Blogroll" vs. "OPML Importer". I'd say that at least the name should be always the same?

#36 in reply to: ↑ 35 ; follow-up: @ocean90
8 years ago

Replying to afercia:

I was avoiding to use "Run Importer" because the "Categories and Tags Converter" doesn't actually "import" anything but "converts" something :) Oh well it's a minor thing, but wondering why this converter should be listed in a screen dedicated to "importing" in the first place.

Good point. I'd ignore that for now and agree that the converter shouldn't be there... There is also this notice: https://cloudup.com/cBFBTVqNMMK Probably worth to change that to "Run Importer" too.

@afercia Is there anything else missing? Maybe aria-labels for the install/details links?

Last edited 8 years ago by ocean90 (previous) (diff)

#37 in reply to: ↑ 36 @afercia
8 years ago

Replying to ocean90:

Good point. I'd ignore that for now and agree that the converter shouldn't be there... There is also this notice: https://cloudup.com/cBFBTVqNMMK Probably worth to change that to "Run Importer" too.

Yep, the same string is used also in the old screen, now displayed only when JS is off:

https://cldup.com/g62dj2zlgP.png

@afercia Is there anything else missing? Maybe aria-labels for the install/details links?

Yep, all links should use an aria-label to add the plugin name and give better context but will open a new ticket for further a11y improvements.

This ticket was mentioned in Slack in #accessibility by ocean90. View the logs.


8 years ago

#39 @afercia
8 years ago

Hm with the last patch applied I'm getting a Cheatin’ uh? screen when I click the Blogroll "Run Importer" link.

@ocean90
8 years ago

@ocean90
8 years ago

#40 follow-up: @ocean90
8 years ago

@afercia I can't reproduce the "Cheatin’ uh?" message. Maybe try again with 35191.12.diff?

35191.12.diff adds aria labels and passes the current WP version to the API.

#41 in reply to: ↑ 40 @afercia
8 years ago

Replying to ocean90:

@afercia I can't reproduce the "Cheatin’ uh?" message. Maybe try again with 35191.12.diff?

Seems my VVV admin user doesn't have the manage_links cap (not sure why) so this check in the OPML_Import plugin's class is triggering a die() with the "Cheatin' uh?" message

if ( !current_user_can('manage_links') )
	wp_die(__('Cheatin&#8217; uh?', 'opml-importer'));

#42 @afercia
8 years ago

OK so looks like the Blogroll (OPML Importer) works just if the old Link Manager is enabled, either via the "Link Manager" plugin or for installations that never disabled it. While it makes sense because links are going to be imported in the... Link Manager :) on the other hand a more useful message would be nice, maybe.

Last edited 8 years ago by afercia (previous) (diff)

#43 follow-up: @afercia
8 years ago

About the aria-labels, yep they add some very useful context for assistive technologies users, nice improvement! However, they should be updated in sync with the link text, for example initially it's:
... aria-label="Install “Blogger”">Install Now</a>
which is correct, then after the shiny install only the link text gets updated so the aria-label and the link are no more in sync:
... aria-label="Install “Blogger”">Run Importer</a>
only after the plugin gets activated and there's a page reload, the aria-label gets updated too:
... aria-label="Run “Blogger”">Run Importer</a>

#44 in reply to: ↑ 43 @ocean90
8 years ago

Replying to afercia:

However, they should be updated in sync with the link text

Yep, I wasn't sure about that because that isn't the case for the other shiny updates actions. But it looks like @swissspidy fixed that in https://core.trac.wordpress.org/attachment/ticket/37290/37290.4.diff.

#45 @swissspidy
8 years ago

37290.4.diff:ticket:37290 doesn't change anything on the import screen I think.

However, fixing this should be as simple as replacing title with aria-label inside wp.updates.installImporterSuccess. Feel free to change & test that today, as I'm not around.

@afercia
8 years ago

#46 @afercia
8 years ago

In 35191.13.diff tried to improve a bit the aria-label to be in sync with the success/error/cancel plugin install status. Worth noting something similar should be done for other plugin install (and maybe also themes?) workflows... but not in this ticket :)

Fixed a bug where $message.data( 'name' ) was undefined because the install link missed a data-name attribute.

Also, added several missing %s to translators comment and improved a docblock summary (was copied and pasted from another function).

#47 @ocean90
8 years ago

  • Owner changed from afercia to ocean90
  • Status changed from assigned to accepted

#48 @ocean90
8 years ago

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

In 38075:

Import: Enhance accessibility on the Import screen.

  • Remove title attributes.
  • Show "Install Now" and "Details" links if the importer isn't installed yet.
  • Show a "Run Importer" link if the importer is installed. It also handles activation if the plugin isn't activated.
  • Add aria-label attributes to each link.
  • Unify the importer descriptions to make them independent from the plugin state. The API was changed in [meta3690].
  • Adjust JavaScript callbacks for ajaxified importer installs.

Props afercia, swissspidy, ocean90.
See #24766.
Fixes #35191.

#49 @afercia
7 years ago

  • Keywords title-attribute added
Note: See TracTickets for help on using tickets.