WordPress.org

Make WordPress Core

Opened 23 months ago

Closed 17 months ago

Last modified 17 months ago

#35457 closed defect (bug) (fixed)

Theme installer upload button improvements

Reported by: afercia Owned by: afercia
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch has-screenshots
Focuses: ui, accessibility, javascript Cc:

Description

See related #35429.

The "Upload Theme/Browse" button is built with two links with href="#" attributes but since it doesn't point to any resource and just "does something" it should be marked up as a button element.

Also, should use an aria-expanded attribute to be dynamically updated to give feedback about the status of the expandable uploader panel.

Worth noting that, when using a keyboard, activating the button causes a focus loss since the button is the currently focused element and then gets hidden to reveal the other button. This should definitely be avoided because keyboard users will be forced to start navigating again from the root of the document (especially noticeable using Chrome).

Attachments (7)

35457.patch (3.7 KB) - added by afercia 19 months ago.
35457.2.patch (4.7 KB) - added by afercia 19 months ago.
35457.3.patch (4.5 KB) - added by afercia 19 months ago.
35457.4.patch (4.6 KB) - added by afercia 18 months ago.
35457.5.patch (4.7 KB) - added by afercia 18 months ago.
35457.6.patch (4.7 KB) - added by afercia 17 months ago.
35457.7.patch (735 bytes) - added by afercia 17 months ago.

Download all attachments as: .zip

Change History (36)

#1 @afercia
23 months ago

  • Keywords needs-patch added
  • Milestone set to 4.5
  • Owner set to afercia
  • Status changed from new to assigned

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


22 months ago

#3 @afercia
22 months ago

Related (for consistency): #35429.

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


21 months ago

#5 @chriscct7
21 months ago

  • Milestone changed from 4.5 to Future Release

Punted because blocked ticket #35429 has been punted.

@afercia
19 months ago

#6 follow-up: @afercia
19 months ago

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

First pass:

  • restores the themes.router.navigate() stuff to support the upload route that was removed in [37221]
  • makes the button a button
  • adds a notice displayed when JavaScript is off (as done in the Media screen), see screenshot below
  • adds some hide-if-no-js CSS classes

Also, I'd propose to don't hide the themes when the uploader is open, for consistency with the Plugins screen and maybe because it is not needed?

About the themes.router.navigate()` I'm not sure it is really useful and actually used, I'd propose to consider to remove it completely. /cc @ocean90 @obenland

In the screenshot below, the theme installer screen with JS off, compared to the Media grid with JS off and the proposed new notice:

https://cldup.com/TbJ_kIplyH.png

#7 in reply to: ↑ 6 ; follow-up: @obenland
19 months ago

Replying to afercia:

About the themes.router.navigate()` I'm not sure it is really useful and actually used, I'd propose to consider to remove it completely.

I'm not sure if we need that to maintain the browser history

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


19 months ago

#9 follow-up: @afercia
19 months ago

Noticed that on WordPress 4.3 when the Theme Uploader was shown, all the themes previews below were hidden:

https://cldup.com/LpCHCIpDrB.png

This changed in WordPress 4.4, not sure if intentional, and now all the themes preview are still displayed when the uploader is open.

In 4.3 the Plugin Uploader was in a dedicated page, with nothing displayed below:

https://cldup.com/D1W6IFlKEP.png

So there was some consistency between the two screens.

Now on trunk both Theme and Plugin screens don't hide all the stuff below when the uploader is displayed. Personally, I'd prefer the actual behavior :) not sure this change was intentional though.

#10 in reply to: ↑ 9 @ocean90
19 months ago

Replying to afercia:

This changed in WordPress 4.4, not sure if intentional, and now all the themes preview are still displayed when the uploader is open.

This change is actually a regression which was introduced by [34891]. The CSS selectors in https://core.trac.wordpress.org/browser/trunk/src/wp-admin/css/themes.css?rev=37247&marks=1106-1107#L1105 don't work anymore.

#11 @afercia
19 months ago

OK so this should be fixed and #35429 reopened to make the plugin uploader consistent with the theme one. By the way, I wouldn't recommend to toggle the button text from "Upload" to "Browse". In combination with aria-expanded the result is confusing for screen reader users. VoiceOver for example will read out:
1) when the uploader is closed:

Upload Theme, collapsed, button

2) when is open:

Browse, expanded, button

Other screen readers read out 2) in a different order ("expanded" and then "browse") but it's confusing anyways. What is this "Browse thing" that is now "expanded", while actually the expanded thing is the uploader?

@afercia
19 months ago

#12 @afercia
19 months ago

Refreshed patch:

  • makes the upload button a button
  • adds an aria-expanded attribute on the button: as a consequence the button text must always be "Upload Theme" since this button behaves like a toggle
  • no more complicated CSS selectors, uses a new CSS class .hide-if-upload-view to hide elements
  • restores the themes.router.navigate() "upload" route mistakenly removed in [37221]
  • adds a notice displayed only when JavaScript is off, as the Media Library grid screen already does
  • adds some hide-if-no-js to various elements

The part related to the Add Plugin screen should be handled in #35429, going to reopen it.

#13 @afercia
19 months ago

  • Keywords ui-feedback added

Looking back at this, I'm not so convinced hiding the list of themes (same for Plugins) is the best option. This is also inconsistent with what happens in the Media Library grid view, where all the attachments stay visible (see screenshot below). I'd greatly appreciate UI/design feedback. Personally, I'd say Themes, Plugins, Media screens should behave in the same way. Any thoughts more than welcome.

https://cldup.com/q4bNlLSwBq.png

Side note: in the Media screen, the "drop zone" should probably be moved above the toolbar.

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


19 months ago

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


19 months ago

#16 in reply to: ↑ 7 @afercia
19 months ago

Replying to obenland:

I'm not sure if we need that to maintain the browser history

I'm not sure the "upload" route is working as intended. When browsing themes clicking on "Featured Popular Latest Favorites" these views get added to the browser's history. In fact, clicking the browser's "back" and "forward" buttons, works. But when clicking on the upload button, the history breaks. Seems it always worked this way.

Maybe I'm missing something but I don't see what this "upload" route should actually do. It would be great to have a closer look at this by someone more expert than me with this kind of things.

Personally, I'd be in favor of removing it and just let the upload button be a toggle. This way browser's history won't break. Also, I wouldn't hide the theme previews.

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


19 months ago

#18 @afercia
19 months ago

I'm sorry this ticket is getting a bit complicated :) Investigated a bit about the upload route and to see the original behavior before the latest changes in 4.4 and 4.5 you should check the Theme Installer on WordPress 4.3 or previous versions.

The upload route was added in [27636]. Reading the related ticket the only reference I could find is:
https://core.trac.wordpress.org/ticket/27055#comment:33

Add a route listener for ?upload, push ?upload to the url stack.

By the way, this route uses the replace option set to true so the URL is updated without creating an entry in the browser's history, see http://backbonejs.org/#Router-navigate
This way, while navigating through "Featured Popular Latest Favorites" and then clicking on the "Upload Theme" button, the last entry in the history gets replaced without creating a new one for "upload". At this point, when clicking the browser's "Back" button:

  • the uploader panel stays open
  • history is broken (will skip the last entry)

Moreover, clicking the "Upload Theme" button to close the uploader panel, will remove any route breaking again the browser's history.

I'd propose to:

  • completely remove the upload route
  • let the uploader be just something that expands and collapses above the themes list: it's just a part of the UI that gets revealed and it shouldn't look like a new page
  • don't hide the themes

This way, the browser's history will work regardless of the theme uploader expanded/collapsed state, not to mention code will be a bit simplified.

@afercia
19 months ago

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


19 months ago

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


19 months ago

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


19 months ago

@afercia
18 months ago

#22 @afercia
18 months ago

  • Keywords commit added; ui-feedback removed

As per discussion, the themes list won't be hidden any longer when the uploader is open. Refreshed patch, looks good to me.

@afercia
18 months ago

#23 @afercia
18 months ago

Refreshed patch to reset the CSS outline on the button. Especially noticeable on OS X/webkit browsers.

#24 @afercia
18 months ago

Screenshots with 35457.5.patch applied, the Theme uploader:

https://cldup.com/HH9ev02fsa.jpg

compared with the Plugin uploader:

https://cldup.com/MUlgSLEgss.jpg

@afercia
17 months ago

#25 @afercia
17 months ago

Refreshed patch to fix the button line-height in IE8-11.

#26 @afercia
17 months ago

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

In 37742:

Accessibility: Theme Installer, make the "Upload Theme" button... a button.

UI controls that "do something" on a page shouldn't be links. This link behaves
like a toggle to expand the uploader panel and should be a button element with
an aria-expanded attribute. Also:

  • improves consistency with the Plugin uploader
  • keeps the themes list visible when the uploader is open
  • displays a notice when JavaScript is off
  • adds some hide-if-no-js CSS classes
  • removes the themes.router.navigate() "upload" route: seems unnecessary and breaks history

Fixes #35457.

#27 @afercia
17 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

On trunk, the Upload Theme button color is black. It should be blue as for all the other page-title-action buttons/links:

https://cldup.com/2JkCnEozwA.png

Buttons that are links inherit the color from the general link CSS rule, while buttons need to have the color expliticly set. Patch incoming.

@afercia
17 months ago

#28 @afercia
17 months ago

35457.7.patch should fix this but it doesn't make me so happy. The only other way to fix this I can think of would be using !important for the white color on the :hover state but !important should be avoided. On the other hand, it would reflect the design intent which is to have the "white" on hover override the other state colors. Open to suggestions.

#29 @ocean90
17 months ago

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

In 37968​:

Themes: After [37742], fix the color of the "Upload Theme" button to match other page title actions.

Props afercia.
Fixes #35457.

Last edited 17 months ago by ocean90 (previous) (diff)
Note: See TracTickets for help on using tickets.