Theme installer upload button improvements

[afercia]

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).

[afercia]

Related (for consistency): #35429.

[chriscct7]

Punted because blocked ticket #35429 has been punted.

[afercia]

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:

[obenland]

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

[afercia]

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

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:

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.

[ocean90]

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.

[afercia]

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]

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.

[afercia]

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.

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

[afercia]

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.

[afercia]

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]

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

[afercia]

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

[afercia]

Screenshots with 35457.5.patch​ applied, the Theme uploader:

compared with the Plugin uploader:

[afercia]

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

[afercia]

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.

[afercia]

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

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]

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.

[ocean90]

In 37968​:

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

Props afercia.
Fixes #35457.