1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 13:16:14 +01:00
Commit graph

2064 commits

Author SHA1 Message Date
epriestley
71c89bd057 Pass all adjacent card PHIDs from the client to the server when moving a card
Summary:
Depends on D20321. Fixes T12175. Ref T13074. Now that before/after PHIDs are suggestions, we can give the server a more complete view of what the client is trying to do so we're more likely to get a good outcome if the client view is out of date.

Instead of passing only the one directly adjacent card PHID, pass all the card PHIDs that the client thinks are in the same group.

(For gigantic columns with tens of thousands of tasks this might need some tweaking -- like, slice both lists down to 10 items -- but we can cross that bridge when we come to it.)

Test Plan:
  - Dragged some cards around to top/bottom/middle positions, saw good positioning in all cases.
  - In two windows, dragged stuff around on the same board. At least at first glance, conflicting simultaneous edits seemed to do reasonable things.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13074, T12175

Differential Revision: https://secure.phabricator.com/D20322
2019-03-26 07:46:05 -07:00
epriestley
6138e50962 When moving cards on workboards, treat before/after cards as optional hints, not strict requirements
Summary:
Depends on D20320. Ref T12175. Ref T13074. Currently, when you move a card between columns, the internal transaction takes exactly one `afterPHID` or `beforePHID` and moves the card before or after the specified card.

This is a fairly strict interpretation and causes a number of practical issues, mostly because the user/client view of the board may be out of date and the card they're dragging before or after may no longer exist: another user might have moved or hidden it between the last client update and the current time.

In T13074, we also run into a more subtle issue where a card that incorrectly appears in multiple columns fatals when dropped before or after itself.

In all cases, a better behavior is just to complete the move and accept that the position may not end up exactly like the user specified. We could prompt the user instead:

> You tried to drop this card after card X, but that card has moved since you last loaded the board. Reload the board and try again.

...but this is pretty hostile and probably rarely/never what the user wants.

Instead, accept a list of before/after PHIDs and just try them until we find one that works, or accept a default position if none work. In essentially all cases, this means that the move "just works" like users expect it to instead of fataling in a confusing/disruptive/undesirable (but "technically correct") way.

(A followup will make the client JS send more beforePHIDs/afterPHIDs so this works more often.)

We could eventually add a "strict" mode in the API or something if there's some bot/API use case for precise behavior here, but I suspect none exist today or are (ever?) likely to exist in the future.

Test Plan:
  - (T13074) Inserted two conflicting rows to put a card on two columns on the same board. Dropped one version of it underneath the other version. Before: confusing fatal. After: cards merge sensibly into one consistent card.
  - (T12175) Opened two views of a board. Moved card A to a different column on the first view. On the second view, dropped card B under card A (still showing in the old column). Before: confusing fatal. After: card ended up in the right column in approximately the right place, very reasonably.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13074, T12175

Differential Revision: https://secure.phabricator.com/D20321
2019-03-26 07:45:24 -07:00
epriestley
686b03a1d5 Dim the action drop preview element when the cursor approaches
Summary:
Depends on D20308. Ref T5474. The element which previews what will happen when you drop a task somewhere can cover the bottom part of the rightmost column on a workboard.

To fix this, I'm trying to just fade it out if you put your cursor over it. I tried to do this in a simple way previously (":hover" + "opacity: 0.25") but it doesn't actually work because "pointer-events: none" stops ":hover" from working.

Instead, do this in Javascript. This is a little more complicated but: it works; and we can do the fade when you get //near// the element instead of actually over it, which feels a little better.

Test Plan:
  - Shrank window to fairly small size so that the preview could cover up stuff on the workboard.
  - Dragged a card toward the rightmost column.
  - Before: drop action preview covered some workboard stuff.
  - After: preview faded out as my cursor approached.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

Differential Revision: https://secure.phabricator.com/D20320
2019-03-25 14:35:55 -07:00
epriestley
c53ed72e4c Provide a clearer UI for "view all results" in partial result panels
Summary:
In some cases, we show a limited number of one type of object somewhere else, like "Recent Such-And-Such" or "Herald Rules Which Use This" or whatever.

We don't do a very good job of communicating that these are partial lists, or how to see all the results. Usually there's a button in the upper right, which is fine, but this could be better.

Add an explicit "more stuff" button that shows up where a pager would appear and makes it clear that (a) the list is partial; and (b) you can click the button to see everything.

Test Plan: {F6302793}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20315
2019-03-25 14:35:08 -07:00
epriestley
bfa5ffe8a1 Add a "Play Sound" workboard trigger rule
Summary:
Ref T5474. Allow columns to play a sound when tasks are dropped.

This is a little tricky because Safari has changed somewhat recently to require some gymnastics to play sounds when the user didn't explicitly click something. Preloading the sound on the first mouse interaction, then playing and immediately pausing it seems to work, though.

Test Plan: Added a trigger with 5 sounds. In Safari, Chrome, and Firefox, dropped a card into the column. In all browsers, heard a nice sequence of 5 sounds played one after the other.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

Differential Revision: https://secure.phabricator.com/D20306
2019-03-25 14:03:57 -07:00
epriestley
66c1d623c3 If the user cancels a workboard drop flow, put things back where they were
Summary:
Ref T13074. If you hit a prompt on a drop operation (today: MFA; in the future, maybe "add a comment" or "assign this task"), we currently leave the board in a bad semi-frozen state if you cancel the workflow by pressing "Cancel" on the dialog.

Instead, put things back the way they were.

Test Plan: Dragged an MFA-required card, cancelled the MFA prompt, got a functional board instead of a semi-frozen board I needed to reload.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13074

Differential Revision: https://secure.phabricator.com/D20305
2019-03-25 14:03:23 -07:00
epriestley
1277db9452 When users hover over a column trigger menu, show a "preview" with the rules instead of a tooltip
Summary:
Ref T5474. The first rough cut of triggers showed some of the trigger rules in a tooltip when you hover over the "add/remove" trigger menu.

This isn't great since we don't have much room and it's a bit finnicky / hard to read.

Since we have a better way to show effects now in the drop preview, just use that instead. When you hover over the trigger menu, preview the trigger in the "drop effect" element, with a "Trigger: such-and-such" header.

Test Plan:
  - This is pretty tough to screenshot.
  - Hovered over menu, got a sensible preview of the trigger effects.
  - Dragged a card over the menu, no preview.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

Differential Revision: https://secure.phabricator.com/D20304
2019-03-25 14:02:10 -07:00
epriestley
567dea5449 Mostly make the editor UI for triggers work
Summary:
Ref T5474. This provides a Herald-like UI for editing workboard trigger rules.

This probably has some missing pieces and doesn't actually save anything to the database yet, but the basics at least roughly work.

Test Plan: {F6299886}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

Differential Revision: https://secure.phabricator.com/D20301
2019-03-25 13:25:14 -07:00
epriestley
a5b3e33e3c Don't show workboard action previews if the action won't have any effect
Summary:
Ref T10335. When you (for example) drag a "Resolved" task into a column with "Trigger: change status to resolved.", don't show a hint that the action will "Change status to resolved." since this isn't helpful and is somewhat confusing.

For now, the only visibility operator is "!=" since all current actions are simple field comparisons, but some actions in the future (like "add subscriber" or "remove project") might need other conditions.

Test Plan:
Dragged cards in ways that previously provided useless hints: move from column A to column B on a "Group by Priority" board; drag a resolved task to a "Trigger: change status to as resolved" column. Saw a more accurate preview in both cases.

Drags which actually cause effects still show the effects correctly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10335

Differential Revision: https://secure.phabricator.com/D20300
2019-03-25 13:24:01 -07:00
epriestley
5dca1569b5 Preview the effects of a drag-and-drop operation on workboards
Summary:
Ref T10335. Ref T5474. When you drag-and-drop a card on a workboard, show a UI hint which lists all the things that the operation will do.

This shows: column moves; changes because of dragging a card to a different header; and changes which will be caused by triggers.

Not implemented here:

  - Actions are currently shown even if they have no effect. For example, if you drag a "Normal" task to a different column, it says "Change priority to Normal.". I plan to hide actions which have no effect, but figuring this out is a little bit tricky.
  - I'd like to make "trigger effects" vs "non-trigger effects" a little more clear in the future, probably.

Test Plan:
Dragged stuff between columns and headers, and into columns with triggers. Got appropriate preview text hints previewing what the action would do in the UI.

(This is tricky to take a screenshot of since it only shows up while the mouse cursor is down.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10335, T5474

Differential Revision: https://secure.phabricator.com/D20299
2019-03-25 13:22:56 -07:00
epriestley
a5226366d3 Make notifications visually clearer, like Feed
Summary: See downstream <https://phabricator.wikimedia.org/T166358>. The notifications menu is missing some CSS to color and style values in stories like "renamed task from X to Y".

Test Plan:
Before:

{F6302123}

After:

{F6302122}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20310
2019-03-25 11:37:00 -07:00
epriestley
a6e17fb702 Improve workboard "Owner" grouping, add "Author" grouping and "Title" sort
Summary:
Depends on D20277. Ref T10333.

  - Put profile icons on "Group by Owner".
  - Add a similar "Group by Author". Probably not terribly useful, but cheap to implement now.
  - Add "Sort by Title". Very likely not terribly useful, but cheap to implement and sort of flexible?

Test Plan: {F6265396}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10333

Differential Revision: https://secure.phabricator.com/D20278
2019-03-12 14:30:38 -07:00
epriestley
c020f027bb Add an "Sort by Creation Date" filter to workboards and modularize remaining order behaviors
Summary:
Depends on D20274. Ref T10578. This is en route to an ordering by points, it's just a simpler half-step on the way there.

Allow columns to be sorted by creation date, so the newest tasks rise to the top.

In this ordering you can never reposition cards, since editing a creation date by dragging makes no sense. This will be true of the "points" ordering too (although we could imagine doing something like prompting the user, some day).

Test Plan: Viewed boards by "natural" (allows reordering both when dragging within and between columns), "priority" (reorder only within columns), and "creation date" (reorder never). Dragged cards around between and within columns, got apparently sensible behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10578

Differential Revision: https://secure.phabricator.com/D20275
2019-03-12 13:46:49 -07:00
epriestley
804be81f5d Provide better UI feedback about cards that can't be dragged or edited
Summary:
Depends on D20273. Fixes T10722. Currently, we don't make it very clear when a card can't be edited. Long ago, some code made a weak attempt to do this (by hiding the "grip" on the card), but later UI changes hid the "grip" unconditionally so that mooted things.

Instead:

  - Replace the edit pencil with a red lock.
  - Provide cursor hints for grabbable / not grabbable.
  - Don't let users pick up cards they can't edit.

Test Plan: On a workboard with a mixture of editable and not-editable cards, hovered over the different cards and was able to figure out which ones I could drag or not drag pretty easily. Picked up cards I could pick up, wasn't able to drag cards I can't edit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10722

Differential Revision: https://secure.phabricator.com/D20274
2019-03-12 13:43:46 -07:00
epriestley
1bdf446a80 Improve rendering of empty workboard columns in header views
Summary:
Depends on D20271. Ref T10333. When a column is empty but a board is grouped (by priority, owner, etc) render the headers properly.

When a column has headers, don't apply the "empty" style even if it has no cards. This style just makes some empty space so you can drag-and-drop more easily, but headers do the same thing.

Test Plan: {F6264611}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10333

Differential Revision: https://secure.phabricator.com/D20272
2019-03-12 13:40:20 -07:00
epriestley
9a8019d4a9 Modularize workboard column orders
Summary:
Depends on D20267. Depends on D20268. Ref T10333. Currently, we support "Natural" and "Priority" orders, but a lot of the particulars are pretty hard-coded, including some logic in `ManiphestTask`.

Although it's not clear that we'll ever put other types of objects on workboards, it seems generally bad that you need to modify `ManiphestTask` to get a new ordering.

Pull the ordering logic out into a `ProjectColumnOrder` hierarchy instead, and let each ordering define the things it needs to work (name, icon, what headers look like, how different objects are sorted, and how to apply an edit when you drop an object under a header).

Then move the existing "Natural" and "Priority" orders into this new hierarchy.

This has a minor bug where using the "Edit" workflow to change a card's priority on a priority-ordered board doesn't fully refresh card/header order since the response isn't ordering-aware. I'll fix that in an upcoming change.

Test Plan: Grouped workboards by "Natural" and "Priority", dragged stuff around within and between columns, grepped for all touched symbols.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10333

Differential Revision: https://secure.phabricator.com/D20269
2019-03-12 13:07:50 -07:00
epriestley
7d849afd16 Add a "WorkboardCardTemplate" class to make workboard client code easier to reason about
Summary:
Depends on D20266. Boards currently have several `whateverMap<cardPHID => stuff>` properties, but we can just move these all down into a `CardTemplate`, similar to the recently introduced `HeaderTemplate`.

The `CardTemplate` holds all the global information for a card, and then `Card` is specific for a particular copy in a column. Today, each `CardTemplate` has one `Card`, but a `CardTemplate` may have more than one card in the future (when we add subproject columns).

Test Plan: Viewed workboards in different sort orders and dragged stuff around, grepped for all affected symbols.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20267
2019-03-12 13:01:52 -07:00
epriestley
46ed8d4a5e On Workboards, sort groups by "natural order", not subpriority
Summary:
Depends on D20263. Ref T10333. I want to add groups like "Assignee" to workboards. This means you may have several tasks grouped under, say, "Alice".

When you drag the bottom-most task under "Alice" to the top, what does that mean?

Today, the only grouping is "Priority", and it means "change the task's secret/hidden global subpriority". However, this seems to generally be a somewhat-bad answer, and is quite complex. It also doesn't make much sense for an author grouping, since one task can't really be "more assigned" to Alice than another task.

Users likely intend this operation to mean "move it, visually, with no other effects" -- that is, user intent is to shuffle sticky notes around on a board, not edit anything substantive. The meaning is probably something like "this is similar to other nearby tasks" or "maybe this is a good place to start", which we can't really capture with any top-level attribute.

We could extend "subpriority" and give tasks a secret/hidden "sub-assignment strength" and so on, but this seems like a bad road to walk down. We'll also run into trouble later when subproject columns may appear on the board, and a user could want to put a task in different positions on different subprojects, conceivably.

In the "Natural" order view, we already have what is probably a generally better approach for this: a task display order particular to the column, that just remembers where you put the sticky notes.

Move away from "subpriority", and toward a world where we mostly keep sticky notes where you stuck them and move them around only when we have to. With no grouping, we still sort by "natural" order, as before. With priority grouping, we now sort by `<priority, natural>`. When you drag stuff around inside a priority group, we update the natural order.

This means that moving cards around on a "priority" board will also move them around on a "natural" board, at least somewhat. I think this is okay. If it's not intuitive, we could give every ordering its own separate "natural" view, so we remember where you stuck stuff on the "priority" board but that doesn't affect the "Natural" board. But I suspect we won't need to.

Test Plan:
  - Viewed and dragged a natural board.
  - Viewed and dragged a priority board.
  - Dragged within and between groups of 0, 1, and multiple items.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10333

Differential Revision: https://secure.phabricator.com/D20265
2019-03-12 12:48:12 -07:00
epriestley
00543f0620 Remove the ability to drag tasks up and down on (non-Workboard) priority list views
Summary:
Ref T13074. Today, in normal task list views in Maniphest (not workboards), you can (sometimes) reorder tasks if the view is priority-sorted.

I suspect no one ever does this, few users know it's supported, and that it was basically rendered obsolete the day we shipped workboards.

This also means that we need to maintain a global "subpriority" for tasks, which distinguishes between different tasks at the same priority level (e.g., "High") and maintains a consistent ordering on workboards.

As we move toward making workboards more flexible (e.g., group by author / owner / custom fields), I'd like to try moving away from "subpriority" and possibly removing it entirely, in favor of "natural order", which basically means "we kind of remember where you put the card and it works a bit like a sticky note".

Currently, the "natural order" and "subpriority" systems are sort of similar but also sort of in conflict, and the "subpriority" system can't really be extended while the "natural order / column position" system can.

The only real reason to have a global "subpriority" is to support the list-view drag-and-drop.

It's possible I'm wrong about this and a bunch of users love this feature, but we can re-evaluate if we get feedback in this vein.

(This just removes UI, the actual subpriority system is still intact and still used on workboards.)

Test Plan: Viewed task lists, was no longer able to drag stuff. Grepped for affected symbols. Dragged stuff in remaining grippable lists, like "Edit Forms" in EditEngine config.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13074

Differential Revision: https://secure.phabricator.com/D20263
2019-03-12 12:47:36 -07:00
epriestley
7574be5372 Remove opacity effects for left-side / right-side diff text selection
Summary:
These effects feel like they're possibly overkill, since other CSS rules make the selection reticle behave correctly and the implementation is relatively intuitive.

Or not, either way.

Test Plan: Selected text on either side of a 2-up diff, no more opacity effects.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20264
2019-03-12 12:40:01 -07:00
epriestley
40af472ff5 Make drag-and-drop on workboards interact with priority column headers
Summary:
Ref T10333. Ref T8135. Depends on D20247. Allow users to drag-and-drop cards on a priority-sorted workboard under headers, even if the header has no other cards.

As of D20247, headers show up but they aren't really interactive. Now, you can drag cards directly underneath a header (instead of only between other cards). For example, if a column has only one "Wishlist" task, you may drag it under the "High", "Normal", or "Low" priority headers to select a specific priority.

(Some of this code still feels a little rough, but I think it will generalize once other types of sorting are available.)

Test Plan: Dragged cards within and between priority groups, saw appropriate priority edits applied in every case I could come up with.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10333, T8135

Differential Revision: https://secure.phabricator.com/D20248
2019-03-09 10:33:26 -08:00
epriestley
14a433c773 Add priority group headers to workboard columns (display only)
Summary:
Ref T10333. When workboards are ordered (for example, by priority), add headers to the various groups. Major goals are:

  - Allow users to drag-and-drop to set values that no cards currently have: for example, you can change a card priority to "normal" by dragging it under the "normal" header, even if no other cards in the column are currently "Normal".
  - Make future orderings more useful, particularly "order by assignee". We don't really have room to put the username on every card and it would create a fair amount of clutter, but we can put usernames in these headers and then reference them with just the profile picture. This also allows you to assign to users who are not currently assigned anything in a given column.
  - Make the drag-and-drop behavior more obvious by showing what it will do more clearly (see T8135).
  - Make things a little easier to scan in general: because space on cards is limited, some information isn't conveyed very clearly (for example, priority information is currently conveyed //only// through color, which can be hard to pick out visually and is probably not functional for users who need vision accommodations).
  - Maybe do "swimlanes": this is pretty much a "swimlanes" UI if we add whitespace at the bottom of each group so that the headers line up across all the columns (e.g., "Normal" is at the same y-axis position in every column as you scroll down the page). Not sold on this being useful, but it's just a UI adjustment if we do want to try it.

NOTE: This only makes these headers work for display.

They aren't yet recognized as targets by the drag list UI, so you can't drag cards into an empty group. I'll tackle that in a followup.

Test Plan: {F6257686}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10333

Differential Revision: https://secure.phabricator.com/D20247
2019-03-09 10:32:55 -08:00
epriestley
be1e3b2cc0 When a user drags a card over a column, highlight the column border
Summary:
Ref T10334. Partly, this just improves visual feedback for all drag operations. After D20242, we can have cases where you (for example) drag a low-priority node to a very tall column on a priority-ordered workboard. In this case, the actual dashed-border-drop-target may not be on screen.

We might make the column scroll or put some kind of hint in the UI in this case, but an easy starting point is just to make the "yes, you're targeting this column" state a bit more clear.

Test Plan: Dragged tasks between columns, saw the border higlight on the target columns. This is very tricky to take a screenshot of.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10334

Differential Revision: https://secure.phabricator.com/D20245
2019-03-09 10:30:23 -08:00
epriestley
2260738de9 When dragging nodes between different columns on an ordered board, don't reorder them by making secondary edits
Summary:
Ref T10334. When a workboard is ordered by priority, dragging from column "A" to a particular place in column "B" currently means "move this task to column B, and adjust its priority so that it naturally sorts into the location under my mouse cursor".

Users frequently find this confusing / undesirable.

To begin improving this, make "drag from column A to column B" and "drag from somewhere in column A to somewhere else in column A" into different operations. The first operation, a movement between columns, no longer implies an ordering change. The second action still does.

So if you actually want to change the priority of a task, you drag it within its current column. If you just want to move it to a different column, you drag it between columns.

This creates some possible problems:

  - Some users may love the current behavior and just not be very vocal about it. I doubt it, but presumably we'll hear from them if we break it.
  - If you actualy want to move + reorder, it's a bit more cumbersome now. We could possibly add something like "shift + drag" for this if there's feedback.
  - The new behavior is probably less surprising, but may not be much more obvious. Future changes (for example, in T10335) should help make it more clear.
  - When you mouse cursor goes over column B, the card dashed-rectangle preview target thing jumps to the correct position in the column -- but that may not be under your mouse cursor. This feels pretty much fine if the whole column fits on screen. It may not be so great if the column does not fit on screen and the dashed-rectangle-thing has vanished. This is just a UI feedback issue and we could refine this later (scroll/highlight the column).

Test Plan:
  - Created several tasks at different priority levels, sorted a board by priority, dragged tasks between columns. Dragging from "A" to "B" no longer causes a priority edit.
  - Also, dragged within a column. This still performs priority edits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10334

Differential Revision: https://secure.phabricator.com/D20242
2019-03-09 10:24:14 -08:00
epriestley
a3ebaac0f0 Tweak the visual style of the ">>" / "<<" depth change indicators slightly
Summary:
Ref T13249.

  - When a line has only increased in indent depth, don't red-fill highlight the left side of the diff. Since reading a diff //mostly// involves focusing on the right side, indent depth changes are generally visible enough without this extra hint. The extra hint can become distracting in cases where there is a large block of indent depth changes.
  - Move the markers slightly to the left, to align them with the gutter.
  - Make them slightly opaque so they're a little less prominent.

Test Plan: See screenshots.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20251
2019-03-07 11:46:26 -08:00
epriestley
d192d04586 Make it more visually clear that you can click things in the "Big List of Clickable Things" UI element
Summary:
Ref T13259. An install provided feedback that it wasn't obvious you could click the buttons in this UI.

Make it more clear that these are clickable buttons.

Test Plan:
{F6251585}

{F6251586}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20238
2019-03-05 11:32:38 -08:00
epriestley
f1a035d5c2 In Differential, give the "moved/copied from" gutter a more clear visual look
Summary:
Depends on D20196. See PHI985. When empty, the "moved/copied" gutter currently renders with the same background color as the rest of the line. This can be misleading because it makes code look more indented than it is, especially if you're unfamiliar with the tool:

{F6225179}

If we remove this misleading coloration, we get a white gap. This is more clear, but looks a little odd:

{F6225181}

Instead, give this gutter a subtle background fill in all casses, to make it more clear that it's a separate gutter region, not a part of the text diff:

{F6225183}

Test Plan: See screenshots. Copied text from a diff, added/removed inlines, etc.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20197
2019-02-20 10:12:16 -08:00
epriestley
a33409991c Remove an old Differential selection behavior
Summary:
Ref T12822. Ref PHI878. This is some leftover code from the old selection behavior that prevented visual selection of the left side of a diff if the user clicked on the right -- basically, a much simpler attack on what ultimately landed in D20191.

I think the change from `th` to `td` "broke" it so it didn't interfere with the other behavior, which is why I didn't have to remove it earlier. It's no longer necessary, in any case.

Test Plan: Grepped for behavior name, selected stuff on both sides of a diff.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12822

Differential Revision: https://secure.phabricator.com/D20196
2019-02-20 10:09:34 -08:00
epriestley
cf048f4402 Tweak some display behaviors for indent indicators
Summary:
Ref T13161.

  - Don't show ">>" when the line indentation changed but the text also changed, this is just "the line changed".
  - The indicator seems a little cleaner if we just reuse the existing "bright" colors, which already have colorblind colors anyway.

Test Plan: Got slightly better rendering for some diffs locally.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161

Differential Revision: https://secure.phabricator.com/D20195
2019-02-19 15:34:30 -08:00
epriestley
fe7047d12d Display some invisible/nonprintable characters in diffs by default
Summary:
Ref T12822. Ref T2495. This is the good version of D20193.

Currently, we display various nonprintable characters (ZWS, nonbreaking space, various control characters) as themselves, so they're generally invisible.

In T12822, one user reports that all their engineers frequently type ZWS characters into source somehow? I don't really believe this (??), and this should be fixed in lint.

That said, the only real reason not to show these weird characters in a special way was that it would break copy/paste: if we render ZWS as "🐑", and a user copy-pastes the line including the ZWS, they'll get a sheep.

At least, they would have, until D20191. Now that this whole thing is end-to-end Javascript magic, we can copy whatever we want.

In particular, we can render any character `X` as `<span data-copy-text="Y">X</span>`, and then copy "Y" instead of "X" when the user copies the node. Limitations:

  - If users select only "X", they'll get "X" on their clipboard. This seems fine. If you're selecting our ZWS marker *only*, you probably want to copy it?
  - If "X" is more than one character long, users will get the full "Y" if they select any part of "X". At least here, this only matters when "X" is several spaces and "Y" is a tab. This also seems fine.
  - We have to be kind of careful because this approach involves editing an HTML blob directly. However, we already do that elsewhere and this isn't really too hard to get right.

With those tools in hand:

  - Replace "\t" (raw text / what gets copied) with the number of spaces to the next tab stop for display.
  - Replace ZWS and NBSP (raw text) with a special marker for display.
  - Replace control characters 0x00-0x19 and 0x7F, except for "\t", "\r", and "\n", with the special unicode "control character pictures" reserved for this purpose.

Test Plan:
- Generated and viewed a file like this one:

{F6220422}

- Copied text out of it, got authentic raw original source text instead of displayed text.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12822, T2495

Differential Revision: https://secure.phabricator.com/D20194
2019-02-19 15:21:44 -08:00
epriestley
efccd75ae3 Correct various minor diff copy behaviors
Summary:
Ref T12822. Fixes a few things:

  - Firefox selection of weird ranges with an inline between the start and end of the range now works correctly.
  - "Show More Context" rows now render, highlight, and select properly.
  - Prepares for nodes to have copy-text which is different from display-text.
  - Don't do anything too fancy in 1-up/unified mode. We don't copy line numbers after the `content: attr(data-n)` change, but that's as far as we go, because trying to do more than that is kind of weird and not terribly intuitive.

Test Plan:
  - Selected and copied weird ranges in Firefox.
  - Kept an eye on "Show More Context" rows across select and copy operations.
  - Generally poked around in Safari/Firefox/Chrome.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12822

Differential Revision: https://secure.phabricator.com/D20192
2019-02-19 15:18:45 -08:00
epriestley
37f12a05ea Behold! Copy text from either side of a diff!
Summary:
Ref T12822. Ref T13161. By default, when users select text from a diff and copy it to the clipboard, they get both sides of the diff and all the line numbers. This is usually not what they intended to copy.

As of D20188, we use `content: attr(...)` to render line numbers. No browser copies this text, so that fixes line numbers.

We can use "user-select" CSS to visually prevent selection of line numbers and other stuff we don't want to copy. In Firefox and Chrome, "user-select" also applies to copied text, so getting "user-select" on the right nodes is largely good enough to do what we want.

In Safari, "user-select" is only visual, so we always need to crawl the DOM to figure out what text to pull out of it anyway.

In all browsers, we likely want to crawl the DOM anyway because this will let us show one piece of text and copy a different piece of text. We probably want to do this in the future to preserve "\t" tabs, and possibly to let us render certain character codes in one way but copy their original values. For example, we could render "\x07" as "␇".

Finally, we have to figure out which side of the diff we're copying from. The rule here is:

  - If you start the selection by clicking somewhere on the left or right side of the diff, that's what you're copying.
  - Otherwise, use normal document copy rules.

So the overall flow here is:

  - Listen for clicks.
  - When the user clicks the left or right side of the diff, store what they clicked.
  - When a selection starts, and something is actually selected, check if it was initiated by clicking a diff. If it was, apply a visual effect to get "user-select" where it needs to go and show the user what we think they're doing and what we're going to copy.
  - (Then, try to handle a bunch of degenerate cases where you start a selection and then click inside that selection.)
  - When a user clicks elsewhere or ends the selection with nothing selected, clear the selection mode.
  - When a user copies text, if we have an active selection mode, pull all the selected nodes out of the DOM and filter out the ones we don't want to copy, then stitch the text back together. Although I believe this didn't work well in ~2010, it appears to work well today.

Test Plan: This mostly seems to work in Safari, Chrome, and Firefox. T12822 has some errata. I haven't tested touch events but am satisfied if the touch event story is anything better than "permanently destroys data".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161, T12822

Differential Revision: https://secure.phabricator.com/D20191
2019-02-19 15:17:07 -08:00
epriestley
d4b96bcf6b Remove hidden zero-width spaces affecting copy behavior
Summary:
Ref T13161. Ref T12822. Today, we use invisible Zero-Width Spaces to try to improve copy/paste behavior from Differential.

After D20188, we no longer need ZWS characters to avoid copying line numbers. Get rid of these secret invisible semantic ZWS characters completely.

This means that both the left-hand and right-hand side of diffs become copyable, which isn't desired. I'll fix that with a hundred thousand lines of Javascript in the next change: this is a step toward everything working better, but doesn't fix everything yet.

Test Plan:
  - Grepped for `zws`, `grep -i zero | grep -i width`.
  - Copied text out of Differential: got both sides of the diff (not ideal).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161, T12822

Differential Revision: https://secure.phabricator.com/D20189
2019-02-19 15:10:04 -08:00
epriestley
98fe8fae4a Use <td class="n" data-n="3"> instead of <th>3</th> for line numbers
Summary:
Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `<th>3</th>` was a perfectly fine way to render a line number cell containing the number "3".

But times have changed!

  - In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to `<td>`.
  - In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own.

The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately.

Test Plan:
  - Created, edited, deleted inlines in 1-up and 2-up views.
  - Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161, T12822

Differential Revision: https://secure.phabricator.com/D20188
2019-02-19 15:07:02 -08:00
epriestley
5310f1cdd9 Remove all whitespace options/configuration everywhere
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.

I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.

Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161, T3498

Differential Revision: https://secure.phabricator.com/D20185
2019-02-19 13:09:29 -08:00
epriestley
661c758ff9 Render indent depth changes more clearly
Summary:
Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like `diff -bw`, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use `diff -bw`.

However, this approach is very coarse and has a lot of problems, like detecting `"ab" -> "a b"` as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach.

We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to `diff -bw` it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration.

I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc.

Move toward detecting and rendering indent depth changes. Followup work:

  - These should get colorblind colors and the design can probably use a little more tweaking.
  - The OneUp mode is okay, but could be improved.
  - Whitespace mode can now be removed completely.
  - I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right.

Test Plan: {F6214084}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161

Differential Revision: https://secure.phabricator.com/D20181
2019-02-19 12:40:05 -08:00
epriestley
aa470d2154 Show user availability dots (red = away, orange = busy) in typeaheads, tokenizer tokens, and autocompletes
Summary:
Ref T13249. See PHI810. We currently show availability dots in some interfaces (timeline, mentions) but not others (typeheads/tokenizers).

They're potentially quite useful in tokenizers, e.g. when assigning tasks to someone or requesting reviews. Show them in more places.

(The actual rendering here isn't terribly clean, and it would be great to try to unify all these various behaviors some day.)

Test Plan:
{F6212044}

{F6212045}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20173
2019-02-19 10:57:20 -08:00
epriestley
3058cae4b8 Allow task statuses to specify that either "comments" or "edits" are "locked"
Summary:
Ref T13249. See PHI1059. This allows "locked" in `maniphest.statuses` to specify that either "comments" are locked (current behavior, advisory, overridable by users with edit permission, e.g. for calming discussion on a contentious issue or putting a guard rail on things); or "edits" are locked (hard lock, only task owner can edit things).

Roughly, "comments" is a soft/advisory lock. "edits" is a hard/strict lock. (I think both types of locks have reasonable use cases, which is why I'm not just making locks stronger across the board.)

When "edits" are locked:

  - The edit policy looks like "no one" to normal callers.
  - In one special case, we sneak the real value through a back channel using PolicyCodex in the specific narrow case that you're editing the object. Otherwise, the policy selector control incorrectly switches to "No One".
  - We also have to do a little more validation around applying a mixture of status + owner transactions that could leave the task uneditable.

For now, I'm allowing you to reassign a hard-locked task to someone else. If you get this wrong, we can end up in a state where no one can edit the task. If this is an issue, we could respond in various ways: prevent these edits; prevent assigning to disabled users; provide a `bin/task reassign`; uh maybe have a quorum convene?

Test Plan:
  - Defined "Soft Locked" and "Hard Locked" statues.
  - "Hard Locked" a task, hit errors (trying to unassign myself, trying to hard lock an unassigned task).
  - Saw nice new policy guidance icon in header.

{F6210362}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20165
2019-02-15 19:18:40 -08:00
epriestley
8810cd2f4d Add a standalone view for the Maniphest task graph
Summary:
See PHI1073. Improve the UX here:

  - When there are a small number of connected tasks, no changes.
  - When there are too many total connected tasks, but not too many directly connected tasks, show hint text with a "View Standalone Graph" button to view more of the graph.
  - When there are too many directly connected tasks, show better hint text with a "View Standalone Graph" button.
  - Always show a "View Standalone Graph" option in the dropdown menu.
  - Add a standalone view which works the same way but has a limit of 2,000.
    - This view doesn't have "View Standalone Graph" links, since they'd just link back to the same page, but is basically the same otherwise.
  - Increase the main page task limit from 100 to 200.

Test Plan:
Mobile View:

{F6210326}

Way too much stuff:

{F6210327}

New persistent link to the standalone page:

{F6210328}

Kind of too much stuff:

{F6210329}

Standalone view:

{F6210330}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: 20after4

Differential Revision: https://secure.phabricator.com/D20164
2019-02-15 14:43:38 -08:00
epriestley
8f8e863613 When users follow an email login link but an install does not use passwords, try to get them to link an account
Summary:
Ref T13249. See PHI774. When users follow an email login link ("Forgot password?", "Send Welcome Email", "Send a login link to your email address.", `bin/auth recover`), we send them to a password reset flow if an install uses passwords.

If an install does not use passwords, we previously dumped them unceremoniously into the {nav Settings > External Accounts} UI with no real guidance about what they were supposed to do. Since D20094 we do a slightly better job here in some cases. Continue improving this workflow.

This adds a page like "Reset Password" for "Hey, You Should Probably Link An Account, Here's Some Options".

Overall, this stuff is still pretty rough in a couple of areas that I imagine addressing in the future:

  - When you finish linking, we still dump you back in Settings. At least we got you to link things. But better would be to return you here and say "great job, you're a pro".
  - This UI can become a weird pile of buttons in certain configs and generally looks a little unintentional. This problem is shared among all the "linkable" providers, and the non-login link flow is also weird.

So: step forward, but more work to be done.

Test Plan: {F6211115}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20170
2019-02-15 14:41:31 -08:00
epriestley
2ca316d652 When users confirm Duo MFA in the mobile app, live-update the UI
Summary: Ref T13249. Poll for Duo updates in the background so we can automatically update the UI when the user clicks the mobile phone app button.

Test Plan: Hit a Duo gate, clicked "Approve" in the mobile app, saw the UI update immediately.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20169
2019-02-15 14:38:15 -08:00
epriestley
187356fea5 Let the top-level exception handler dump a stack trace if we reach debug mode before things go sideways
Summary:
Depends on D20140. Ref T13250. Currently, the top-level exception handler doesn't dump stacks because we might not be in debug mode, and we might double-extra-super fatal if we call `PhabricatorEnv:...` to try to figure out if we're in debug mode or not.

We can get around this by setting a flag on the Sink once we're able to confirm that we're in debug mode. Then it's okay for the top-level error handler to show traces.

There's still some small possibility that showing a trace could make us double-super-fatal since we have to call a little more code, but AphrontStackTraceView is pretty conservative about what it does and 99% of the time this is a huge improvement.

Test Plan: {F6205122}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20142
2019-02-11 15:36:19 -08:00
epriestley
a20f108034 When an edit overrides an object lock, note it in the transaction record
Summary:
Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.

Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.

Also, make the "MFA" and "Silent" emblems more easily visible.

Test Plan:
Edited a locked task, overrode the lock, got marked for it.

{F6195005}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: aeiser

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20131
2019-02-09 06:10:07 -08:00
epriestley
2b718d78bb Improve UI/UX when users try to add an invalid card with Stripe
Summary: Ref T13244. See PHI1052. Our error handling for Stripe errors isn't great right now. We can give users a bit more information, and a less jarring UI.

Test Plan:
Before (this is in developer mode, production doesn't get a stack trace):

{F6197394}

After:

{F6197397}

- Tried all the invalid test codes listed here: https://stripe.com/docs/testing#cards

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20132
2019-02-09 05:54:42 -08:00
epriestley
7469075a83 Allow users to be approved from the profile "Manage" page, alongside other similar actions
Summary:
Depends on D20122. Fixes T8029. Adds an "Approve User" action to the "Manage" page.

Users are normally approved from the "Approval Queue", but if you click into a user's profile to check them out in more detail it kind of dead ends you right now. I've occasionally hit this myself, and think this workflow is generally reasonable enough to support upstream.

Test Plan: {F6193742}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8029

Differential Revision: https://secure.phabricator.com/D20123
2019-02-07 15:04:23 -08:00
epriestley
c9ff6ce390 Add CSRF to SMS challenges, and pave the way for more MFA types (including Duo)
Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.

Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.

Test Plan: Implemented Duo, elsewhere.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13231, T13222

Differential Revision: https://secure.phabricator.com/D20028
2019-01-24 15:10:57 -08:00
epriestley
bb20c13651 Allow MFA factors to provide more guidance text on create workflows
Summary:
Depends on D20016. Ref T920. This does nothing interesting on its own since the TOTP provider has no guidance/warnings, but landing it separately helps to simplify an upcoming SMS diff.

SMS will have these guidance messages:

  - "Administrator: you haven't configured any mailer which can send SMS, like Twilio."
  - "Administrator: SMS is weak."
  - "User: you haven't configured a contact number."

Test Plan: {F6151283} {F6151284}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D20017
2019-01-23 14:10:16 -08:00
epriestley
22ad1ff2c5 Show the customized "Login" message on the login screen
Summary: Depends on D19992. Ref T13222. If administrators provide a custom login message, show it on the login screen.

Test Plan:
{F6137930}

  - Viewed login screen with and without a custom message.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19994
2019-01-18 19:54:02 -08:00
epriestley
6b6c991ad4 Allow Phortune accounts to customize their billing address and name
Summary:
See PHI1023. Ref T7607. Occasionally, companies need their billing address (or some other custom text) to appear on invoices to satisfy process or compliance requirements.

Allow accounts to have a custom "Billing Name" and a custom "Billing Address" which appear on invoices.

Test Plan: {F6134707}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T7607

Differential Revision: https://secure.phabricator.com/D19979
2019-01-16 16:16:27 -08:00
epriestley
3b94b3e812 Correct a zero-based month tooltip on burnup charts
Summary: See PHI1017. This is a trivial fix even though these burnups are headed toward a grisly fate.

Test Plan: Moused over some January datapoints, saw "1" instead of "0".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19967
2019-01-15 18:09:18 -08:00
epriestley
afa69eedd1 Remove an old digest in Celerity code and some obsolete configuration options
Summary:
Ref T12509. This upgrades a `weakDigest()` callsite to SHA256-HMAC and removes three config options:

  - `celerity.resource-hash`: Now hard-coded, since the use case for ever adjusting it was very weak.
  - `celerity.enable-deflate`: Intended to make cache inspection easier, but we haven't needed to inspect caches in ~forever.
  - `celerity.minify`: Intended to make debugging minification easier, but we haven't needed to debug this in ~forever.

In the latter two cases, the options were purely developer-focused, and it's easy to go add an `&& false` somewhere in the code if we need to disable these features to debug something, but the relevant parts of the code basically work properly and never need debugging. These options were excessively paranoid, based on the static resource enviroment at Facebook being far more perilous.

The first case theoretically had end-user utility for fixing stuck content caches. In modern Phabricator, it's not intuitive that you'd go adjust a Config option to fix this. I don't recall any users ever actually running into problems here, though.

(An earlier version of this change did more magic with `celerity.resource-hash`, but this ended up with a more substantial simplification.)

Test Plan: Grepped for removed configuration options.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D19941
2019-01-04 13:43:38 -08:00
epriestley
3963c86ad5 Pass timeline view data to comment previews, restoring Differential comment previews
Summary:
Ref T13222. In D19918, I refactored how timelines get "view data". Today, this is always additional data about which images/changesets/diffs are visible on the current revision/commit/mock, so we can tell if inline comments should be linked to a `#anchor` on the same page (if the inline is rendered there somewhere) or to a `/D123?id=1&vs=2` full link on a different page (if it isn't), but in general this could be any sort of state information about the current page that affects how the timeline should render.

Previously, comment previews did not use any specialized object code and always rendered a "generic" timeline story. This was actually a bug, but none of the code we have today cares about this (since it's all inline related, and inlines render separately) so it never impacted anything.

After the `TimelineEngine` change, the preview renders with Differential-specific code. This is more correct, but we were not passing the preview the "view data" so it broke.

This preview doesn't actually need the view data and we could just make it bail out if it isn't present, but pass it through for consistency and so this works like we'd expect if we do something fancier with view data in the future.

Test Plan: Viewed comment and inline comment previews in Differential, saw old behavior restored.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19943
2019-01-03 13:06:54 -08:00
epriestley
1729e7b467 Improve UI for "wait" and "answered" MFA challenges
Summary:
Depends on D19906. Ref T13222. This isn't going to win any design awards, but make the "wait" and "answered" elements a little more clear.

Ideally, the icon parts could be animated Google Authenticator-style timers (but I think we'd need to draw them in a `<canvas />` unless there's some clever trick that I don't know) or maybe we could just have the background be like a "water level" that empties out. Not sure I'm going to actually write the JS for either of those, but the UI at least looks a little more intentional.

Test Plan:
{F6070914}

{F6070915}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19908
2018-12-28 00:18:53 -08:00
epriestley
6c43d1d52c Remove "willRenderTimeline()" from ApplicationTransactionInterface
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.

`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.

The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.

Try to clean this up:

  - Stop requiring `willRenderTimeline()` to be implemented.
  - Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
  - Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
  - If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.

This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.

Test Plan:
  - Viewed audits, revisions, and mocks with inline comments.
  - Used "Show Older" to page a revision back in history (this is relevant for "View Data").
  - Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11351

Differential Revision: https://secure.phabricator.com/D19918
2018-12-20 14:55:07 -08:00
epriestley
55a1ef339f Fix a bad method call signature throwing exceptions in newer Node
Summary:
Ref T13222. See PHI996. Ref T10743. For context, perhaps see T12171.

Node changed some signatures, behaviors, and error handling here in recent versions. As far as I can tell:

  - The `script.runInNewContext(...)` method has never taken a `path` parameter, and passing the path has always been wrong.
  - The `script.runInNewContext(...)` method started taking an `[options]` parameter at some point, and validating it, so the bad `path` parameter now throws.
  - `vm.createScript(...)` is "soft deprecated" but basically fine, and keeping it looks more compatible.

This seems like the smallest and most compatible correct change.

Test Plan: Under Node 10, started Aphlict. Before: fatal error on bad `options` parameter to `runInNewContext()` (expected dictionary). After: notification server starts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T10743

Differential Revision: https://secure.phabricator.com/D19860
2018-12-10 16:01:00 -08:00
epriestley
68b1dee139 Replace the "Choose Subtype" radio buttons dialog with a simpler "big stuff you click" sort of UI
Summary:
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:
{F6053035}

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

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222, T12588

Differential Revision: https://secure.phabricator.com/D19855
2018-12-10 14:59:18 -08:00
epriestley
2e8a5e843f Recover when cookies are disabled in Firefox and accessing localStorage throws
Summary:
Ref T13216. See PHI985. If you disable cookies in Firefox, accessing `window.localStorage` throws an exception. Currently, this pretty much kills all scripts on the page.

Instead, catch and ignore this, as though `window.localStorage` was not defined.

Test Plan:
  - Set Firefox to "no cookies".
  - Loaded any page while logged out.
  - Before: JS fatal early in the stack.
  - After: page loads and JS works.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19832
2018-11-26 16:30:42 -08:00
epriestley
8b550ce2cd Don't allow the middle mouse button to start an inline comment
Summary:
Ref T13216. See PHI985. When you click a line number to start an inline comment, we intend to initiate the action only if you used the left mouse button (desktop) or a touch (tablet/device).

We currently have a `not right` condition for doing this, but it only excludes right clicks, not middle clicks (or other nth-button clicks). The `not right` condition was sligthly easier to write, but use an `is left` condition instead of a `not right` condition.

Test Plan:
  - In Safari, Firefox and Chrome:
    - Used left click to start an inline.
    - Used middle click to do nothing (previously: started an inline).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19836
2018-11-26 10:14:48 -08:00
epriestley
44c32839a6 When you "Request Review" of a draft revision, change the button text from "Submit Quietly" to "Publish Revision"
Summary:
See PHI975. Ref T13216. Ref T2543. Previously, see D19204 and PHI433.

When you're acting on a draft revision, we change the button text to "Submit Quietly" as a hint that your actions don't generate notifications yet.

However, this isn't accurate when one of your actions is "Request Review", which causes the revision to publish.

Allow actions to override the submit button text, and make the "Request Review" action change the button text to "Publish Revision".

The alternative change I considered was to remove the word "Quietly" in all cases.

I'm not //thrilled// about how complex this change is to adjust one word, but the various pieces are all fairly clean individually. I'm not sure we'll ever be able to use it for anything else, but I do suspect that the word "Quietly" was the change in D19204 with the largest effect by far (see T10000).

Test Plan:
  - Created a draft revision. Saw "Submit Quietly" text.
  - Added a "Request Review" action, saw it change to "Publish Revision".
  - Reloaded page, saw stack saved and "Publish Revision".
  - Removed action, saw "Submit Quietly".
  - Repeated on a non-draft revision, button stayed put as "Submit".
  - Submitted the various actions, saw them have the desired effects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216, T2543

Differential Revision: https://secure.phabricator.com/D19810
2018-11-15 20:50:21 -08:00
epriestley
ec452e548a Improve text overflow behavior for hovercards with (for example) long package names
Summary: See PHI977. Ref T13216. Some text, like long package names, may overflow hovercards. Add overflow CSS behaviors to remedy this.

Test Plan:
Before:

{F6012699}

After:

{F6012700}

(You can use `/search/hovercard/` to render hovercards in a handy standalone way.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216

Differential Revision: https://secure.phabricator.com/D19809
2018-11-15 20:43:10 -08:00
epriestley
ea6d2afa86 Fix flickering tooltips in Chrome when the tip container overlaps the triggering element
Summary:
Fixes T8440. See that task for discussion.

Ref T13216. See PHI976.

Test Plan:
In Chrome, hovered a timestamp and moved the mouse up to the "overlap" area (see T8440). Before: flickered like crazy. After: no flickering.

(I couldn't reproduce the original issue in modern Firefox or Safari.)

Reviewers: amckinley, avivey

Reviewed By: avivey

Maniphest Tasks: T8440, T13216

Differential Revision: https://secure.phabricator.com/D19808
2018-11-15 10:43:55 -08:00
epriestley
5e1d94f336 Remove nonfunctional AJAX embed behavior for Slowvote
Summary:
See <https://hackerone.com/reports/434116>. Slowvote has a piece of Javascript that attempts to let you vote on `{V123}` polls inline.

It does not work: nothing ever triggers it (nothing renders a control with a `slowvote-option` sigil).

At least for now, just remove it. It has a completely separate pathway in the controller and both pathways are buggy, so this makes fixing them easier.

Test Plan: Voted in plurality and approval polls via Slowvote and the embedded widget.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19773
2018-11-06 09:20:07 -08:00
epriestley
cfd9fa7f55 Add an explicit "max-width" to PHUIDocumentPro pages to force large tables to scroll
Summary:
Ref T13202. See <https://discourse.phabricator-community.org/t/phriction-page-controls-lost-after-creating-very-wide-table/1961>.

If you put a very wide table in the markup for a new-layout Phriction page, it can push the actions element off screen to the right.

Tables already get a scrollbar if encouraged strongly enough; add a `max-width` to encourage them.

Test Plan:
  - Viewed pages with a large wrappable and non-wrappable content on mobile, tablet, and desktop.

{F5915976}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13202

Differential Revision: https://secure.phabricator.com/D19723
2018-10-01 13:15:59 -07:00
epriestley
550028a882 Allow Phriction document edits to be saved as drafts
Summary:
Depends on D19661. Ref T13077. See PHI840.

When a user edits a page normally, add a "Save as Draft" button. Much of this change is around making that button render and behave properly: it needs to be an `<input type="submit" ...>` so browsers submit it and we can figure out which button the user clicked.

Then there are a few minor rules:

  - If you're editing a page which is already a draft, we only give you "Save as Draft". This makes edits to update/revise a draft more natural.
  - Highlight "Publish" if it's a likely action that you might want to take.

Internally, there are two types of edits. Both types create a new version with the new content. However:

  - A "content" edit sets the version shown on the live page to the newly-created version.
  - A "draft" edit does not update the version shown on the live page.

Test Plan: Edited a published document, edited the draft. Published documents. Reverted documents.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19662
2018-09-12 13:30:40 -07:00
epriestley
e19c555913 Support (basic) commenting on Phriction documents
Summary:
Depends on D19659. Fixes T1894. Ref T13077. See PHI840.

  - Add an EditEngine, although it currently supports no fields.
  - Add (basic, top-level-only) commenting (we already had the table in the database).

This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.

This also moves us incrementally toward putting all editing on top of EditEngine.

Test Plan: {F5877347}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13077, T1894

Differential Revision: https://secure.phabricator.com/D19660
2018-09-12 13:20:52 -07:00
epriestley
185e72c881 Add aural section headers for "Event Timeline", "Add Comment", and "Comment Preview"
Summary: See PHI871. Ref T13197. These sections are only divided visually and don't have textual headers. Add aural headers.

Test Plan: {F5875471}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13197

Differential Revision: https://secure.phabricator.com/D19654
2018-09-11 13:30:10 -07:00
epriestley
6dc721009d Layout Phriction actions without floats, to avoid conflicts with floating content
Summary:
Ref T13195. If a Phriction page begins with a code block, the `clear: both;` currently makes it clear the action list.

Instead, use table-cell layout on desktops.

Test Plan: Viewed a Phriction page with an initial code block on desktop/tablet/mobile/printable layouts. Now got more sensible layouts in all cases.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: GoogleLegacy

Maniphest Tasks: T13195

Differential Revision: https://secure.phabricator.com/D19649
2018-09-10 11:19:53 -07:00
epriestley
876638e428 Add a UI element for navigating between versions of a Phriction document
Summary: Depends on D19621. Ref T13077. Fixes T4815. This adds previous/current/next/draft buttons and makes navigation between unpublished and published versions of a document more clear.

Test Plan: {F5841997}

Reviewers: amckinley

Maniphest Tasks: T13077, T4815

Differential Revision: https://secure.phabricator.com/D19622
2018-08-29 13:49:15 -07:00
epriestley
4afb6446d9 Allow DocumentView to render with a curtain, and make Phriction use a curtain
Summary:
Depends on D19616. Ref T13077. Fixes T8172. In the last round of design updates, a lot of actions got stuffed into "Actions" menus.

I never really got used to these and think they're a net usability loss, and broadly agree with the feedback in T8172. I'd generally like to move back toward a state where actions are available on the page, not hidden in a menu.

For now, just put a curtain view on these pages. This could be refined later (e.g., stick this menu to the right hand side of the screen) depending on where other Phriction changes go.

(Broadly, I'm also not satisfied with where we ended up on the fixed-width pages like Diffusion > Manage, Config, and Instances. In contrast, I //do// like where we ended up with Phortune in terms of overall design. I anticipate revisiting some of this stuff eventually.)

Test Plan:
  - Looked at Phriction pages on desktop/tablet/mobile/printable -- actions are now available on the page.
  - Looked at other DocumentView pages (like Phame blogs) -- no changes for now.

Reviewers: amckinley

Maniphest Tasks: T13077, T8172

Differential Revision: https://secure.phabricator.com/D19617
2018-08-28 14:58:05 -07:00
epriestley
614f9ba1fb Allow unit test results to specify that their details are formatted with remarkup when reporting to "harbormaster.sendmessage"
Summary: Ref T13189. See PHI710. Ref T13088. Fixes T9951. Allow callers to `harbormaster.sendmessage` to specify that the test details are remarkup so they can use rich formatting and include links, files, etc.

Test Plan: {F5840098}

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13189, T13088, T9951

Differential Revision: https://secure.phabricator.com/D19615
2018-08-28 13:26:11 -07:00
epriestley
415de4ce37 Repaint filetree more consistently for mobile/device views
Summary:
Ref T13189. See <https://discourse.phabricator-community.org/t/diffusion-differential-mobile-layout-broken-when-enabling-file-tree/1751>.

We currently call a nonexistent `resetdrag()` which does nothing. Some sequences of interactions can result in a blank left column in mobile/device widths.

Repaint the filetree away more consistently on device change.

Test Plan: Viewed a revision, toggled filetree off + on, resized to narrow width. Before: bad left margin, JS console error. After: proper repaint at device breakpoint, no JS console error.

Reviewers: amckinley

Maniphest Tasks: T13189

Differential Revision: https://secure.phabricator.com/D19611
2018-08-27 10:32:47 -07:00
epriestley
8a6d767843 Fix a minor text alignment issue for static text comment actions like "Accept Revision"
Summary:
Ref T13187. See PHI836. The "action" comment actions in Differential (Accept, Reject, etc) render a single line of descriptive text. This is currently slightly misaligned.

Give it similar sizing information to the label element to the left, so it lines up properly.

Test Plan:
Note that "Request Review" and "This revision will be..." are now aligned:

{F5828077}

Reviewers: amckinley

Maniphest Tasks: T13187

Differential Revision: https://secure.phabricator.com/D19600
2018-08-24 10:12:06 -07:00
epriestley
75a5dd8d8c Add more accessibility labels for screen readers
Summary:
Depends on D19594. See PHI823. Ref T13164.

  - Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
  - Add a label for the floating header display options menu in Differential.
  - Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.

Test Plan:
Viewed a revision with `?__aural__=true`:

  - Saw "Remove Action: ..." label.
  - Saw "Display Options" label.
  - Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19595
2018-08-17 13:31:51 -07:00
epriestley
9a15129b40 Remove 750ms timeout on owners path validation
Summary:
Ref T13164. See PHI748. Path validation has a 750ms timeout which blames to rP5038ab850c, in 2011.

Production path validation is sometimes taking more than 750ms, particularly on the initial page load where we may validate many paths simultaneously.

I have no idea why we have this timeout, and it isn't consistent with how we perform other AJAX requests. Just remove it.

Test Plan:
  - Reproduced issue in production, saw all validation calls failing at 750ms. Actual underlying calls succeed, they just take more than 750ms to resolve.
  - Loaded path validator locally, got green checkmark.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19575
2018-08-13 13:52:26 -07:00
epriestley
e5906f4e12 In Differential standalone views, disable some keyboard shortcuts which don't work
Summary:
Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files.

We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI.

Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled.

Test Plan:
  - Viewed a revision in normal and standalone views.
  - No changes in normal view, and all keys still work ("N", "P", etc).
  - In standalone view, "?" no longer shows nonfunctional key commands.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19571
2018-08-13 08:59:05 -07:00
epriestley
8374201620 Add a more specific CSS rule to make Spaces headers in projects colored red
Summary:
Depends on D19551. Ref T13164. Projects use a special kind of header setup that has a more specific CSS rule to make content black. Add an even more specific rule to make it red.

(This could probably be disentangled a bit and isn't necessarily the cleanest fix, but I poked at it for a few minutes and didn't come up with anything cleaner.)

Test Plan: Viewed projects in spaces, saw the space names colored red properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19552
2018-07-31 10:23:35 -07:00
epriestley
4e84d4d458 Allow the haunted comment panel ("Z") to take up more vertical room
Summary:
Ref T13151. See PHI685. When you haunt the panel, we only let it take up part of the screen. Let it take up slightly more of the screen so that it's more likely to fit completely on-screen without needing to scroll.

The behavior when it does scroll is fine (you get a scrollbar if your OS/browser is set up to show them) so this is a bit trivial/silly, but seems fine and doesn't have a big JS maintenance cost or anything.

Test Plan: Pressed "Z", resized my window to a weird tiny useless size, got slightly better (I guess) behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19480
2018-06-07 13:19:35 -07:00
epriestley
7729c51cc4 Fix an issue where scrolling down, then up, then down fails to show changeset header in Differential
Summary: Ref T13151. See PHI616. There's a bug where the current banner changeset isn't cleared correctly when we hide the banner.

Test Plan:
  - View revision with several changesets.
  - Scroll down slowly through first changeset until banner appears.
  - Scroll up until banner disappears.
  - Scroll back down.
  - Before: banner fails to reappear (code still thinks it's visible and we don't want to update it).
  - After: banner reappears correctly.

Reviewers: amckinley, jmeador

Reviewed By: jmeador

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19474
2018-06-07 12:02:18 -07:00
epriestley
a894c99935 Add "max-width: 100%;" to stop large images from overflowing the new rendering engine UI
Summary:
Fixes T13148. Ref T13105. The new document rendering engine for images let them overflow the UI bounds.

Add `max-width: 100%;` to keep them contained.

Test Plan:
  - Viewed a very wide image in Safari, Firefox and Chrome. Saw sensible rendering.
  - Also viewed a normal image, saw normal behavior.

Reviewers: amckinley, avivey

Reviewed By: avivey

Maniphest Tasks: T13148, T13105

Differential Revision: https://secure.phabricator.com/D19457
2018-06-01 14:53:10 -07:00
epriestley
31ee49b14d Fix Javascript busy loop when trying to delete tokens from an empty tokenizer
Summary:
Fixes T13147. In D19437, I changed this logic to support deleting the `""` (empty string) token, but `[].pop()` returns `undefined`, not `null`, if the list is empty and I didn't think to try deleting an empty input.

Fix the logic so we don't end up in a loop if the input is empty.

Test Plan:
  - In any browser, deleted all tokens in a tokenizer; then pressed delete again.
  - Before: tab hangs in an infinte loop.
  - After: smooth sailing.

Reviewers: amckinley, avivey

Reviewed By: avivey

Maniphest Tasks: T13147

Differential Revision: https://secure.phabricator.com/D19456
2018-06-01 14:51:06 -07:00
epriestley
d280b24239 Fix "arc paste" to stop creating pastes with an empty string ("") as the "language"
Summary:
See PHI652. When you `echo x | arc paste` today, you end up with a Paste object that has the empty string as its "language".

This is normally not valid. Pastes where the language should be autodetected should have the value `null`, not the empty string.

This behavior likely changed when `paste.create` got rewritten in terms of `paste.edit`. Adjust the implementation so it only adds the LANGUAGE transaction if there's an actual language.

Also, fix an issue where you can't use the "delete" key to delete tokens with the empty string as their value.

Test Plan:
  - Created a paste with `echo x | arc paste`, got a paste in autodetect mode instead of with a bogus language value.
  - Created a paste with `echo x | arc paste --lang rainbow`, got a rainbow paste.
  - Deleted an empty string token with the keyboard.
  - Deleted normal tokens with the keyboard.
  - Edited subscribers/etc normally with the keyboard and mouse to make sure I didn't ruin anything.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19437
2018-05-09 13:22:58 -07:00
epriestley
26c0db8dd7 Allow navigation breadcrumbs to be marked as "always visible" so they show up on phones
Summary:
See PHI624. Some of the mobile navigation and breadcrumbs in support pacts aren't as good as they could be.

In particular, we generally collapse crumbs on mobile to just the first and last crumbs. The first crumb is the application; the last is the current page.

On `/PHIxxx` pages, the first crumb isn't very useful since the Support landing page is two levels up: you usually want to go back to the pact, not all the way back to the Support landing page.

We also don't need the space since the last crumb (`PHIxxx`) is always small.

Allow Support and other similar applications to tailor the crumb behavior more narrowly if they end up in situations like this.

Test Plan:
  - With an additional change to instances (see next diff), viewed a support issue page (`/PHI123`) on mobile and desktop.
  - Saw a link directly back to the pact on both mobile and desktop.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19438
2018-05-09 13:21:47 -07:00
epriestley
afc3099ee7 Add a view option to disable blame in Diffusion and fix some view transition bugs
Summary:
See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off.

Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame.

Test Plan:
  - Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL.
  - Viewed a Jupyter notebook, toggled to "Source" view, saw blame.
  - Viewed stuff in Files (no blame UI options).
  - Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13105

Differential Revision: https://secure.phabricator.com/D19414
2018-04-30 15:32:23 -07:00
epriestley
28517110c6 Fix an issue in the new Harbormaster build log view where clicking the "^" icon doesn't work right
Summary:
Ref T13130. See PHI617.

The new build log UI has tags like `<a href="...">Show More Above <span icon>^</span></a>`. If you click the little "^" icon, the event target is the `<span />` instead of the `<a />` so we expand on the wrong node.

Instead, select the `<a />` by sigil explicitly.

Test Plan: Viewed new log UI in Harbormaster, clicked "^" icon and text, got the same (correct) behavior on both.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19410
2018-04-27 11:51:59 -07:00
epriestley
33da9f833f Fix odd line number line wrapping on embedded pastes ({Pxxx})
Summary: Ref T13126. After SourceView changes, embedded pastes with the `{Pxxx}` syntax are line-wrapping line numbers in Safari, at least. Put a stop to this.

Test Plan: Viewed a `{Pxxx}` with more than 10 lines. Before: weird line wrapping; after: nice consistent display.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13126

Differential Revision: https://secure.phabricator.com/D19393
2018-04-20 14:20:20 -07:00
epriestley
19403fdb8e Improve color use in "[+++- ]" element for colorblind users
Summary:
Ref T13127. Users with red/green colorblindness may have difficulty using this element in its current incarnation.

We could give it different behavior if the "Accessibility" option is set for red/green colorblind users, but try a one-size-fits-all approach since the red/green aren't wholly clear anwyay.

Test Plan: {F5530050}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13127

Differential Revision: https://secure.phabricator.com/D19385
2018-04-19 17:24:44 -07:00
epriestley
70d67a3908 Fix the most significant "phantom notification" badness
Summary:
Ref T13124. Ref T13131. Fixes T8953. See PHI512.

When you receieve a notification about an object and then someone hides that object from you (or deletes it), you get a phantom notification which is very difficult to clear.

For now, test that notifications are visible when you open the menu and clear any that are not.

This could be a little more elegant than it is, but the current behavior is very clearly broken. This unbreaks it, at least.

Test Plan:
  - As Alice, configured task stuff to notify me (instead of sending email).
  - As Bailey, added Alice as a subscriber to a task, then commented on it.
  - As Alice, loaded home and saw a notification count. Didn't click it yet.
  - As Bailey, set the task to private.
  - As Alice, clicked the notification bell menu icon.
    - Before change: no unread notifications, bell menu is semi-stuck in a phantom state which you can't clear.
    - After change: bad notifications automatically cleared.

{F5530005}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13131, T13124, T8953

Differential Revision: https://secure.phabricator.com/D19384
2018-04-19 17:24:19 -07:00
epriestley
665529ab60 Restore coverage reporting to Diffusion browse UI
Summary:
Depends on D19377. Ref T13125. Ref T13124. Ref T13105. Coverage reporting in Diffusion didn't initially survive the transition to Document Engine; restore it.

This adds some tentative/theoretical support for multiple columns of coverage, but no way to actually produce them in the UI. For now, the labels, codes, and colors are hard coded.

Test Plan:
Added coverage with `diffusion.updatecoverage`, saw coverage in the UI:

{F5525542}

Hovered over coverage, got labels and highlighting.

Double-checked labels for "N" (Not Executable) and "U" (Uncovered). See PHI577.

Faked some multi-column coverage, but you can't currently get this yourself today:

{F5525544}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13125, T13124, T13105

Differential Revision: https://secure.phabricator.com/D19378
2018-04-17 14:51:47 -07:00
epriestley
21bb0215db Remove obsoleted "diffusion-browse-file" behavior for coverage
Summary: Ref T13105. After moving Diffusion to DocumnentEngine, this no longer has callers. It will become part of the document behavior.

Test Plan: Grepped for calls to the `diffusion-browse-file` behavior, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19377
2018-04-17 14:51:12 -07:00
epriestley
37a03402bc When following a link to a particular line ("/example.txt$12"), scroll to that line
Summary:
Depends on D19349. Ref T13105. This was the behavior in Diffusion before with a little hard-coded snippet.

Remove that snippet ("diffusion-jump-to") and add a more general-purpose snippet to SourceView.

This is a tiny bit hacky still (and probably doesn't work quite right with Quicksand) but gets things working again and works in all of Files, Paste, and Diffusion.

Test Plan: Followed links to particular lines in Paste, Files and Diffusion; got scrolled to the right place.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19350
2018-04-11 17:29:22 -07:00
epriestley
5b3a351852 Use pseudoelements, not Zero Width Space, to implement copy/paste behavior in Paste/Diffusion
Summary:
Depends on D19348. Ref T13105. When copying text from Paste or Diffusion, we'd like to copy only source, not line numbers.

We currently accomplish this with zero-width spaces plus a trigger that fires on "copy" in Paste and Diffusion. This is quite gross.

In the new-style Harbormaster logs, we use an approach that seems slightly better: CSS psuedoelements.

This isn't a complete solution (see also PHI504 / T5032) but puts us in a slightly better place.

Use it in Paste/Files/Diffusion too.

This gives us good behavior in all browsers in Files and Paste.

This gives us good behavior in Chrome and Firefox in Diffusion. Safari will copy (but not visually select) blame information in Diffusion. I think we can live with that for now.

Test Plan: Selected and copy/pasted stuff in Diffusion, Files, and Paste. Got good behavior everywhere except Safari + Diffusion.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19349
2018-04-11 17:28:46 -07:00
epriestley
c5c53e277a Make line selection in source code views less fragile and more consistent
Summary:
Depends on D19347. Ref T13105. See PHI565. The "highlight lines" behavior is interacting poorly with the new blame element in Diffusion.

Make the behavior a little simpler and hopefully more robust.

Test Plan:
  - Clicked commit/revision links in Diffusion, saw the links get followed instead of the lines highlighted.
  - Highlighted lines in Diffusion, saw just the line/code highlight instead of the whole thing.
  - Highlighted lines in Paste and new-style Harbormaster build logs, saw consistent behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19348
2018-04-11 17:27:34 -07:00
epriestley
ac570fd4bc When you make the file tree huge, scroll to the right, and then toggle it, stop it from growing
Summary: Depends on D19346. Ref PHI568. I love Javascript.

Test Plan:
  - Viewed a revision.
  - Dragged file tree view really wide.
  - Scrolled document to the right.
  - Toggled file tree off and on by pressing "f" twice.
    - Before patch: file tree grew wider and wider after it was toggled.
    - After patch: file tree stayed the same size after it was toggled.
  - Dragged to various widths and reloaded to make sure the "sticky across reloads" behavior still works.
  - Scrolled right, dragged the tree a bit, then reloaded and didn't see it flip out.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19347
2018-04-11 17:25:31 -07:00
epriestley
55619e8964 Restore an explicit white background color to files in Paste
Summary:
Ref T13105. Previously, the "source code" view in Paste rendered on a brown/orange-ish background. I've been using this element in more contexts (Files, Diffusion) and removed the colored background to make text (particularly syntax-highlighted text) easier to read and reduce visual noise with the new blame colors.

In Diffusion the view is in a box with a white background so removing the background left us with white, but in Paste it's just directly on the page so the background was bleeding through. Instead, set it to white explicitly.

Test Plan: Viewed source files in Files, Diffusion and Paste; saw text on a white background.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19346
2018-04-11 17:21:33 -07:00
epriestley
d6ef32a7b7 Give the "Filetree" UI element an explicit background color
Summary:
See PHI568. If you make the file tree UI very wide so that the page generates a horizontal scrollbar and then scroll the page, the page content can paint underneath the menu.

The menu already has a z-index to make it render above the content, but doesn't actually have a background. Give it a background.

The "transparent" rule was added in D16346 but I don't see any reason why we actually need it there, so I think this probably won't break anything.

Test Plan: {F5518822}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120

Differential Revision: https://secure.phabricator.com/D19344
2018-04-11 10:42:41 -07:00
epriestley
4c4a5a7656 Fix the wrapping/padding behavior of Remarkup code block headers more thoroughly?
Summary: Ref T13118. The first fix there fixed Safari, but made Chrome weird. Try this?

Test Plan: Viewed a code block with `name=...` in Safari, Firefox and Chrome and saw consistent display without weird wrappping.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13118

Differential Revision: https://secure.phabricator.com/D19319
2018-04-10 04:37:14 -07:00
epriestley
472bc3d90a Colorize lines in blame under DocumentEngine, to show relative age of changes
Summary:
Depends on D19313. Ref T13105. Fixes T13015. We lost the coloration for ages in the switch to Document Engine.

Restore it, and use a wider range of colors to make the information more clear.

Test Plan: Viewed some blame, saw a nice explosion of bright colors. This is a cornerstone of good design.

Maniphest Tasks: T13105, T13015

Differential Revision: https://secure.phabricator.com/D19314
2018-04-09 06:11:47 -07:00
epriestley
cf75d63b49 When lines 12, 13, 14, etc all blame to the same change, only show it once
Summary:
Depends on D19312. Ref T13105. For readability, render only one link for each contiguous block of changes.

Also make the actual rendering logic a little more defensible.

Test Plan: Viewed some files with blame, saw one render per chunk instead of one per line.

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19313
2018-04-09 06:11:06 -07:00
epriestley
eb80f0a2d9 When you swap between document rendering engines, populate or redraw blame if appropriate
Summary: Depends on D19311. Ref T13105. Currently, blame only renders on the initial request. Instead, redraw blame after swapping views.

Test Plan: Swapped from "Source -> Hexdump -> Source" and "Hexdump -> Source". Saw blame on source in all cases.

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19312
2018-04-09 06:10:41 -07:00
epriestley
11664277b3 Make DocumentEngine source line linking behavior better when blame is shown
Summary: Ref T13105. The line linker behavior currently has trouble identifying the line number when blame is active. Improve this, albeit not the most cleanly.

Test Plan: Selected lines with blame on.

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19310
2018-04-09 06:09:40 -07:00