Page MenuHomePhabricator

Replace the "Choose Subtype" radio buttons dialog with a simpler "big stuff you click" sort of UI

Authored by epriestley on Dec 7 2018, 2:15 PM.



Ref T13222. Fixes T12588. See PHI683. In several cases, we present the user with a choice between multiple major options: Alamnac service types, Drydock blueprint types, Repository VCS types, Herald rule types, etc.

Today, we generally do this with radio buttons and a "Submit" button. This isn't terrible, but often it means users have to click twice (once on the radio; once on submit) when a single click would be sufficient. The radio click target can also be small.

In other cases, we have a container with a link and we'd like to link the entire container: notifications, the /drydock/ console, etc. We'd like to just link the entire container, but this causes some problems:

  • It's not legal to link block eleements like <a><div> ... </div></a> and some browsers actually get upset about it.
  • We can <a><span> ... </span></a> instead, then turn the <span> into a block element with CSS -- and this sometimes works, but also has some drawbacks:
    • It's not great to do that for screenreaders, since the readable text in the link isn't necessarily very meaningful.
    • We can't have any other links inside the element (e.g., details or documentation).
  • We can <form><button> ... </button></form> instead, but this has its own set of problems:
    • You can't right-click to interact with a button in the same way you can with a link.
    • Also not great for screenreaders.

Instead, try adding a linked-container behavior which just means "when users click this element, pretend they clicked the first link inside it".

This gives us natural HTML (real, legal HTML with actual <a> tags) and good screenreader behavior, but allows the effective link target to be visually larger than just the link.

If no issues crop up with this, I'd plan to eventually use this technique in more places (Repositories, Herald, Almanac, Drydock, Notifications menu, etc).

Test Plan

  • Left-clicked and command-left-clicked the new JS fanciness, got sensible behaviors.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Dec 7 2018, 2:15 PM
Owners added a subscriber: Restricted Owners Package.Dec 7 2018, 2:15 PM
epriestley requested review of this revision.Dec 7 2018, 2:17 PM
epriestley edited the summary of this revision. (Show Details)Dec 7 2018, 2:17 PM
epriestley edited the summary of this revision. (Show Details)
epriestley marked 2 inline comments as done.Dec 7 2018, 2:21 PM
epriestley added inline comments.

The phabricator-remarkup rule has some side effects with margins that prevented the list of options from laying out properly.

This change removes phabricator-remarkup from the body of all dialogs. It's possible this will do some weird things.

I couldn't find any weird things based on prodding it for a bit. When we add remarkup to dialogs, it is normally added inside its own container. I think this was just a legacy thing and that collateral damage will be minor or nonexistent, but this might need some amount of cleanup. In any case, removing this class is clearly more "correct" from a purity standpoint, since dialog bodies are often not remarkup.


This is an existing hack-around for the problem described in the summary, which links the text and separately links the icon in UIs like the /drydock/ console. Since the entire element how acts like a link, I removed this screenreader-hostile secondary supplemental link.

epriestley updated this revision to Diff 47410.Dec 7 2018, 2:23 PM
  • Remove some great but ultimately unnecessary debugging code.
amckinley accepted this revision.Dec 10 2018, 7:58 PM
This revision is now accepted and ready to land.Dec 10 2018, 7:58 PM
This revision was automatically updated to reflect the committed changes.
epriestley added inline comments.Dec 12 2018, 5:31 PM

D19871 corrects the accidental omission of $label here when simplifying the UI element.