1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-08 16:02:40 +01:00
Commit graph

3643 commits

Author SHA1 Message Date
epriestley
017ef1927c Revert use of "user-select: all" to modify tab selection behavior
Summary:
Reverts D21419. See PHI1814. Previously, I used "user-select: all" to group sequences of spaces for selection.

However, this has a side effect: the sequence is now selected with a single click. I didn't read the docuementation on the CSS property thoroughly and missed this in testing, since I was focused on drag-selection behavior.

This behavior is enough of a net negative that I think we're in a worse state overall; revert it.

Test Plan: Straight revert.

Differential Revision: https://secure.phabricator.com/D21429
2020-07-24 13:41:26 -07:00
epriestley
37ffb71c4d In source views, wrap display tabs in "user-select: all" to improve cursor selection behavior
Summary:
Ref T2495. See PHI1814. Currently, Phabricator replaces tabs with spaces when rendering diffs.

This may or may not be the best behavior in the long term, but it gives us more control over expansion of tabs than using tab literals.

However, one downside is that you can use your mouse cursor to select "half a tab", and can't use your mouse cursor to distinguish between tabs and spaces. Although you probably shouldn't be doing this, this behavior is less accurate/correct than selecting the entire block as a single unit.

A specific correctness issue with this behavior is that the entire block is copied to the clipboard as a tab literal if you select any of it, so two different visual selection ranges can produce the same clipboard content.

This particular behavior can be improved with "user-select: all", to instruct browsers to select the entire element as a single logical element. Now, selecting part of the tab selects the whole thing, as though it were really a tab literal.

(Some future change might abandon this approach and opt to use real tab literals with "tab-size" CSS, but we lose some ability to control alignment behavior if we do that and it doesn't have any obvious advantages over this approach other than cursor selection behavior.)

Test Plan:
  - In Safari and Firefox, dragged text to select a whitespace-expanded tab literal. Saw browsers select the whole sequence as though it were a single tab.
  - In Chorme, this also mostly works, but there's some glitchiness and flickering. I think this is still a net improvement, it's just not as smooth as Safari and Firefox.

Maniphest Tasks: T2495

Differential Revision: https://secure.phabricator.com/D21419
2020-07-17 15:10:06 -07:00
epriestley
a7b3ba5a6f When long monospaced character sequences appear in Remarkup tables, break rather than scrolling
Summary:
Ref PHI1798. If you put an SSH public key in a table cell with monospaced formatting and then print the table, the cell scrolls and not all of the content appears in your physical printed document.

Generally, the current scrolling behavior for monospaced text seems never-desirable: I can't imagine any cases where we want the table cell to scroll. (There's more of an argument for complex cases where a table cell has, say, an embedded paste.)

Add `line-break: anywhere` to break monospaced text inside these cells.

Test Plan: In Safari, Firefox, and Chrome, viewed a ##|`MMMMM....`|## table. Saw scrolling before and wrapping/breaking after.

Differential Revision: https://secure.phabricator.com/D21370
2020-06-29 16:18:05 -07:00
epriestley
3635a11f84 When cancelling an edit of an inline with content, don't hide the inline
Summary: See PHI1753. This condition got rewritten for suggested edits and accidentally inverted.

Test Plan:
  - Create a comment, type text, save draft, edit comment, cancel.
  - Before: comment hides itself.
  - After: comment properly cancels into pre-edit draft state.

Differential Revision: https://secure.phabricator.com/D21286
2020-05-22 15:40:25 -07:00
epriestley
87fb35abb7 Prevent keyboard selection of change blocks inside edit suggestions
Summary: Ref T13513. When a revision has inlines with edit suggestions, pressing "j" and "k" can incorrectly select the blocks inside the diffs inside the inlines.

Test Plan: Used "j" to cycle through changes in a revision with inline comments with edit suggestions, didn't get jumped into the suggestion diffs.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21283
2020-05-21 15:37:51 -07:00
epriestley
66566f878d Make "Open in Editor" use the simple line number of the current selected block
Summary:
Ref PHI1749. Instead of opening files to the last unchanged line on either side of the change, open files to the "simple" line number of the selected block.

For inlines, this is the inline line number.

For blocks, this is the first new-file line number, or the first old-file line number if no new-file line number exists in the block.

This may not always be what the user is hoping for (we can't know what the state of their working copy is) but should produce more obvious behavior.

Test Plan:
  - In Diffusion, used "Open in Editor" with and without line selections. Saw same behavior as before.
  - Used "n" and "r" to leave an inline with the keyboard, saw same behavior as before.
  - Used "\" and "Open in Editor" menu item to open a file with:
    - Nothing selected or changeset selected (line: 1).
    - An inline selected (line: inline line).
    - A block selected (line: first line in block, per above).

Differential Revision: https://secure.phabricator.com/D21282
2020-05-21 15:31:16 -07:00
epriestley
d2d7e7f5ff Clean up Diffusion behaviors for inline edit suggestions
Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.

Test Plan:
  - Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
  - Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21278
2020-05-20 14:28:12 -07:00
epriestley
10f241352d Render inline comment suggestions as real diffs
Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing.

Test Plan: {F7495053}

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21277
2020-05-20 14:27:40 -07:00
epriestley
846562158a Roughly support inline comment suggestions
Summary:
Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work.

Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source.

Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21276
2020-05-20 14:26:37 -07:00
epriestley
00430fdbe1 Make server components of inline comment content handling state-oriented
Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.

Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21275
2020-05-20 14:25:59 -07:00
epriestley
87bc30526b Make inline content "state-oriented", not "string-oriented"
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.

This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.

Test Plan: Created, edited, submitted, cancelled, etc., comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21273
2020-05-20 14:24:11 -07:00
epriestley
770a5c8412 Fix "r" and "R" both replying with quote on inline comments
Summary: Ref T13513. The code which added "r" and "R" to the inline menu accidentally discarded the difference between the keystrokes.

Test Plan:
  - Clicked an inline, pressed "r", got new empty inline (previously: inline with quote).
  - Clicked an inline, pressed "R", got a new quoted inline.
  - Repeated steps with the menu items, got the expected behaviors.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21268
2020-05-19 09:28:36 -07:00
epriestley
93b08f0e83 Fix an out-of-order access issue with inlines
Summary: Ref T13513. On `secure`, I caught an issue where inlines may be accessed directly before they're constructed. Instead, access them through the relevant accessor.

Test Plan: Will deploy.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21263
2020-05-15 13:55:58 -07:00
epriestley
e959f93489 Use a more consistent inline highlighting style with fewer redraws
Summary:
Ref T13513. The on-hover-inline reticle has switched over to have cell-based behavior. Switch the on-hover-line-number reticle to use the same behavior.

Also, clean up the dirty/redraw loop slightly: we no longer need to dirty on resize, and we don't need to redraw if the range isn't actually dirty.

Test Plan: Highlighted lines and line ranges. Hovered over inlines.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21262
2020-05-15 12:44:40 -07:00
epriestley
c666cb9f0b Reduce the frequency of DOM scans to rebuild inlines when scrolling revisions
Summary:
Ref T13513. See PHI1734, which raises a concern about the performance of large revisions near the 100-change threshold.

Currently, `getInlines()` is called whenever the scroll position transitions between two changesets, and it performs a relatively complicated DOM scan to lift inlines out of the document.

This shows up as taking a small but nontrivial amount of time in Firefox profiles and should be safely memoizable.

Test Plan:
  - Under Firefox profiling, scrolled through a large revision.
  - Before change: `getInlines()` appeared as the highest-cost thing we're explicitly doing on profiles.
  - After change: `getInlines()` was no longer meaningfully represented on profiles.
  - Created inlines, edited inlines, etc. Didn't identify any broken behavior.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21261
2020-05-15 09:37:41 -07:00
epriestley
3ee6b5393c Improve offset/range inline behavior for rich diffs and unified diffs
Summary:
Ref T13513. The way I'm highlighting lines won't work for Jupyter notebooks or other complex content blocks, and I don't see an obvious way to make it work that's reasonably robust.

However, we can just ignore the range behavior for complex content and treat the entire block as selected. This isn't quite as fancy as the source behavior, but pretty good.

Also, adjust unified diff behavior to work correctly with highlighting and range selection.

Test Plan:
  - Used range selection in a Jupyter notebook, got reasonable behavior (range is treated as "entire block").
  - Used range selection in a unified diff, got equivalent behavior to 2-up diffs.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21257
2020-05-14 16:02:32 -07:00
epriestley
fbd57ad832 Give selected inline comments are more obvious selected state
Summary:
Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state.

Also fix a couple of minor issues with select interactions and offset comments.

Test Plan: Selected inlines, saw obvious visual cues.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21256
2020-05-14 14:35:55 -07:00
epriestley
b021da71a5 When users click headers to select diff UI elements, don't eat the events
Summary: Ref T13513. Currently, clicking inline or changeset headers eats the click events. This doesn't serve any clear purpose, and means these clicks do not clear text selections from the document, which is unusual.

Test Plan:
  - Selected some text in a diff.
  - Clicked a changeset header to select it.
  - Before patch: text selection and context menu were retained.
  - After patch: text selection was cleared and context menu was dismissed.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21255
2020-05-14 14:34:38 -07:00
epriestley
42f1a95a12 Fix a flash of document selection when "oncopy" and "inline on range" behaviors interact
Summary:
Ref T13513. In Safari, do this:

  - view a 2-up diff with content on both sides;
  - select more than one line on the right side; and
  - use your mouse to click "New Inline Comment" in the context menu that pops up.

The mousedown event for the "New Inline Comment" click removes the "copy selection" behavior and creates a flash where both sides of the diff are selected.

This doesn't happen with (most) normal clicks because mouse down on a non-grabbable element clears the document selection.

To avoid this, don't reset the copy selection behavior if the user mouses down on an "<a />". This might not be robust, but seems simple and plausible as a fix.

Test Plan:
  - See above.
  - Before patch: flash of overbroad selection when clicking "New Inline Comment".
  - After patch: no selection flash.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21254
2020-05-14 14:29:46 -07:00
epriestley
f45519c060 When cancelling an inline comment edit, exit the edit state after the response arrives
Summary: Ref T13513. This fixes a bug where clicking a line number, then clicking "Cancel" causes the paths panel to briefly update with an extra inline comment counted on the file.

Test Plan:
  - Clicked a line number.
  - Typed some text.
  - Clicked "Cancel".
  - Before patch: paths panel flashes "1".
  - After patch: paths panel stays stable.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21253
2020-05-14 14:28:20 -07:00
epriestley
cfb5de6fa7 Distinguish more carefully between "null" inline offsets and "0" inline offsets
Summary:
Ref T13513. Currently, when creating an inline by selecting a line range, slightly careless handling leads to an inline with "0" offsets (by passing "undefined" to the server). This causes the block to highlight every line except the last one as fully bright, which is incorrect.

An inline with "0" offsets and an inline with no offsets are different. Be more careful about passing offsets around and rendering them.

Test Plan:
  - Used the line numbers to add an inline to lines 4-8 of a change.
  - Hovered the inline.
  - Saw all four lines marked as "dull"-highlighted (previously: three bright lines, one dull line).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21252
2020-05-14 14:26:54 -07:00
epriestley
2f5398796e Store inline comment offset information and show it when highlighting comments
Summary:
Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.

When hovering the comment, highlight the original range.

Test Plan: {F7480926, size=full}

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21250
2020-05-13 17:21:53 -07:00
epriestley
ebef22ccc1 Improve select-to-comment behavior in Firefox and on unified diffs
Summary:
Ref T13513.

  - Firefox represents multiple selected rows as a discontinuous range. Accommodate this.
  - Unified diffs don't have a "copy" marker. Do something sort-of-reasonable for them.

Test Plan:
  - Selected multiple lines of content in Firefox, got an option to add a comment.
  - Selected content in unified mode, got an option to add a comment.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21249
2020-05-13 17:19:53 -07:00
epriestley
42378ea393 Allow users to create inline comments by directly selecting text directly
Summary: Ref T13513. Support direct text selection for inlines. This is currently just an alternate way to get to the same place as using line numbers, but can preserve offset/range information in the future.

Test Plan:
  - Selected some text, hit "c", clicked "New Inline Comment", got sensible comments on both sides of a diff in Safari, Chrome, and (with limitations) Firefox.
  - Caveats: no unified support, doesn't work across lines in Firefox.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21248
2020-05-13 17:15:18 -07:00
epriestley
c063e0e5ec Add "View Raw Remarkup" to inline comments
Summary: Ref T13513. Ref T11401. Support viewing raw remarkup for inlines.

Test Plan: Viewed raw remarkup on inlines.

Maniphest Tasks: T13513, T11401

Differential Revision: https://secure.phabricator.com/D21246
2020-05-13 17:14:20 -07:00
epriestley
419b7ceebb Move inline comment actions into a dropdown menu
Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.

Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).

Maniphest Tasks: T13513, T11401

Differential Revision: https://secure.phabricator.com/D21244
2020-05-13 17:13:18 -07:00
epriestley
1da54837ea Improve line breaking behavior in Firefox and Chrome under complex conditions
Summary: See <https://github.com/phacility/phabricator/pull/854>. In some situations, `line-break: anywhere` produces better behavior than `word-break: break-all`. It never appears to produce worse behavior.

Test Plan:
  - Break behavior changes if a line contains "<span />" elements caused by syntax highlighting. This CSS adjustment only appears to apply to text with internal "<span />" elements.
  - This specifically impacts certain internal breakpoints adjacent to punctuation, so the test case is highly specific. Generic test cases with latin word characters do not evidence any behavioral changes.
  - This change appears to have no impact on Safari, which uses the better behavior in all cases.
  - Before Patch: In Firefox and Chrome, this specific change breaks awkwardly. There is more room for text to fit on the broken line:

Firefox

{F7480567}

Chrome

{F7480568}

  - After Patch: Firefox and Chrome break the line better. Here's Firefox:

{F7480569}

  - Additional context:

Safari Behavior (Unchanged)

{F7480570}

Chrome with no highlighting (desirable behavior). Firefox does the same thing.

{F7480571}

Also tested other cases, which seem never-worse in any browser.

{F7480574}

Differential Revision: https://secure.phabricator.com/D21247
2020-05-13 11:54:42 -07:00
epriestley
acc1fa1655 Make "View as Document Type..." only show valid options
Summary:
Ref T13513. Currently, "View as Document Type..." lists every available engine.

This is hard to get completely right because we can't always rebuild the document ref accurately in the endpoint, but try harder to fake something reasonable.

Test Plan: Used "View as Document Type..." on Jupyter notebooks, was given "Jupyter" and "Source" as options.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21241
2020-05-12 14:25:37 -07:00
epriestley
0cca40db3b When creating an inline, save the current document engine
Summary:
Ref T13513. As part of inline metadata, save the document engine the change is being rendered with.

This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else.

The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks.

Test Plan:
  - Created inlines and replies on normal soure code, saw no document engine annotated in the database.
  - Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database.
  - Swapped document engines between Jupyter and Source, etc.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21240
2020-05-12 14:25:09 -07:00
epriestley
e7ebd5d9d1 Make "Delete" from inline comment previews function correctly while editing comments
Summary: Ref T13513. Currently, if you're editing a comment, "delete" doesn't put the comment into the correct state. This action is normally only reachable from comment previews, since an editing inline has no "delete" button.

Test Plan:
  - Started editing an inline, clicked "Delete", got a deletion.
  - Created an inline, typed text,
  - Deleted a normal comment via preview.
  - Deleted a normal comment via the on-inline action.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21238
2020-05-08 08:54:48 -07:00
epriestley
b804e8cffa Make "View" from inline comment previews correctly jump to "isEditing" inlines
Summary:
Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.

Update this behavior so it works on inlines in either "Viewing" or "Editing" states.

Test Plan:
  - Clicked "View" on a normal inline, got jumped/selected.
  - Clicked "View" on an editing inline, got jumped/selected.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21237
2020-05-08 08:52:42 -07:00
epriestley
24ba66f106 Persist "Show Changeset" and improve path text selection
Summary:
Ref T13513. Currently:

  - If you click the "Show Changeset" button, your state change doesn't actually get saved on the server.
  - It's hard to select a changeset path name for copy/paste because the "highlight the header" code tends to eat the event.

Instead: persist the former event; make the actual path text not be part of the highlight hitbox.

Test Plan:
  - Clicked "Show Changeset", reloaded, saw changeset visibility persisted.
  - Selected changeset path text without issues.
  - Clicked non-text header area to select/deselect changesets.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21236
2020-05-08 06:59:10 -07:00
epriestley
1656a2ff08 Allow inline comment storage objects to generate their own runtime objects
Summary:
Ref T13513. Currently, inline storage objects ("TransactionComment") can't directly generate a runtime object ("InlineComment").

Allow this transformation to be performed in a genric way so clunky code which does it per-object-type can be removed, lifted, or simplified.

Simplify an especially gross callsite in preview code.

Test Plan: Previewed inline comments.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21226
2020-05-07 15:57:49 -07:00
epriestley
6b69102990 Fix a JS issue when the anchor element on a page has no container
Summary: See D21213. If there's no matching element, `findAbove()` throws. Handle these cases correctly.

Test Plan: Visited `#toc` on a revision, no longer saw a JS error.

Differential Revision: https://secure.phabricator.com/D21222
2020-05-04 15:57:31 -07:00
epriestley
07e160bde1 When cancelling an unsaved editing inline after a reload, don't cancel into an empty state
Summary:
Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.

Instead, provide the text when the editor opens. This fixes a cancel interaction.

Test Plan:
  - Create an inline, type text, don't save.
  - Reload page.
  - Cancel.
  - Before: cancelled into empty state.
  - After: cancelled into deleted+undo state.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21219
2020-05-04 15:20:31 -07:00
epriestley
3a76248071 When loading a page with inlines, don't select/focus inlines which we immediately upgrade to "editing"
Summary:
Ref T13513. This is a bit clumsy, but the cleanest way to implement "isEditing" inlines today is to send them down as normal inlines and then simulate clicking "edit" on them.

When we do, don't focus the resulting editor: focusing it makes the page scroll around and highlight things in essentially random order as the editors load in.

Test Plan: Reloaded a page with some open editors, wasn't scrolled to them.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21217
2020-05-04 15:15:27 -07:00
epriestley
fe501bd7f7 Save drafts for inline comments currently being edited
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.

This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.

Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21216
2020-05-04 13:19:42 -07:00
epriestley
63bfad0ff4 Refine unusual inline comment client interactions
Summary: Ref T13513. Refine some inline behaviors, see test plan.

Test Plan:
  - Edit a comment ("A"), type text ("AB"), cancel, edit.
    - Old behavior: edit and undo states (wrong, and undo does not function).
    - New behavior: edit state only.
  - Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
    - Old behavior: "AB" (wrong: you never submitted this text).
    - New behavior: "A".
  - Create a comment, type text, cancel.
    - Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
    - New behavior: no counter.
  - Cancel editing an empty comment with no text.
    - Old behavior: Something buggy -- undo, I think?
    - New behavior: it just vanishes (correct behavior).

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21212
2020-05-04 13:15:01 -07:00
epriestley
67da18e374 When users submit "editing" inlines, warn them that their inlines will be saved
Summary: Ref T13513. This slightly expands the existing-but-hacky "warning" workflow to cover both "mentions on draft" and "submitting inlines being edited".

Test Plan:
  - Submitted changes to a revision with mentions on a draft, inlines being edited, both, and neither.
  - Got sensible warnings in the cases where warnings were appropriate.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21191
2020-05-04 13:13:15 -07:00
epriestley
b2ce0844b6 When a user clicks "Cancel" on an inline comment to leave the "Editing" state, save the state change
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.

This also serves to delete empty comments.

Test Plan:
  - Clicked a line number to create a new comment. Then:
    - Clicked "Cancel". Reloaded page, saw no more comment.
    - Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
    - Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21187
2020-05-04 13:11:23 -07:00
epriestley
b48a22bf50 Make "editing" state persistent for inline comments
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.

In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.

---

Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.

On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.

Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).

To simplify this:

  - Make `PhabricatorInlineCommentInterface` an abstract base class instead.
  - Lift as much code out of the `Audit` and `Differential` subclasses as possible.
  - Delete methods which no longer have callers, or have only trivial callers.

---

Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.

Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.

These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.

The `EditView` can not fully render its content. Move the content rendering code into the view.

---

Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.

This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.

---

Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.

Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.

This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.

---

Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".

Test Plan:
  - Created comments on either side of a diff.
  - Edited a comment, reloaded, saw edit stick.
  - Saved comments, reloaded, saw save stick.
  - Edited a comment, typed text, cancelled, "unedited" to get state back.
  - Created a comment, typed text, cancelled, "unedited" to get state back.
  - Deleted a comment, "undeleted" to get state back.

Weirdness / known issues:

  - Drafts don't autosave yet.
  - Fixed in D21187:
    - When you create an empty comment then reload, you get an empty editor. This is a bit silly.
    - "Cancel" does not save state, but should, once drafts autosave.
  - Mostly fixed in D21188:
    - "Editing" comments aren't handled specially by the overall submission flow.
    - "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.

Subscribers: jmeador

Maniphest Tasks: T13513

Differential Revision: https://secure.phabricator.com/D21186
2020-05-04 13:10:30 -07:00
epriestley
54ec566281 Restore highlighting when jumping to transactions using URI anchors
Summary:
At some point, the highlighting behavior for the timeline broke. When you follow a link to a particular timeline story, the story should be highlighted.

Prior to this change, the `<a />` tag itself highlights, but there's no associated CSS and it's too deep in the tree to do anything useful.

(Since this change is fairly straightforward, I gave up digging for the root cause before finding it.)

Test Plan:
  - Clicked a timeline story anchor, saw the story highlight.

Differential Revision: https://secure.phabricator.com/D21213
2020-05-04 10:04:04 -07:00
epriestley
1205070687 Use underlines instead of background color to show file moves/renames
Summary: Ref T13520. This is a style tweak that I think looks a little cleaner.

Test Plan: {F7410424}

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21205
2020-05-01 12:23:59 -07:00
epriestley
f21f1d8ab9 Update the diff table of contents to use hierarchical views and edit distance renames
Summary:
Ref T13520. Generally, make the table of contents look and more like the paths panel:

  - Show a hierarchy, with compression for single-sibling children.
  - Use the same icons, instead of "M D" and "(img)" stuff.
  - Use EditDistanceMatrix to do a piece-by-piece diff of paths changes.
  - Show path changes within the path list.

I'm not entirely sold on this, but it was complicated to write and I've never heard the term "sunk cost fallacy". I think this is mostly a net improvement, but may need some adjustments and followup.

Test Plan: Viewed various changes in Differential and Diffusion, saw a more usable table of contents.

Maniphest Tasks: T13520

Differential Revision: https://secure.phabricator.com/D21183
2020-04-28 12:27:37 -07:00
epriestley
5a460e4ea5 Stick the page footer in the right place on Formation View pages
Summary: Ref T13516. This isn't terribly clean, but get the page footer into the bottom of the content page on FormationView pages so it doesn't overlap into the side panel.

Test Plan: With and without a footer, viewed normal and FormationView pages. Saw footers in appropriate places at appropriate times.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21166
2020-04-24 11:25:31 -07:00
epriestley
4793bfcb7c Don't show the "file tree" view on tablets/phones
Summary: Ref T13516. Hide this UI on devices without the screen width to reasonably support it.

Test Plan: Viewed a revision at various window widths, saw the elements vanish at device widths and reappear at desktop widths.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21162
2020-04-23 13:40:42 -07:00
epriestley
d2572f8b33 Refine more Differential review state behaviors
Summary:
Ref T13516.

- Add an "Add Comment" navigation anchor.
- Make selection state more clear.
- Make hidden state tidier and more clear.
- Hide "View Options" in the hidden state to dodge all the weird behaviors it implies.
- Click to select/deselect changesets.
- When you open the view dropdown menu, then press "h", close the dropdown menu.

Test Plan: Fiddled with all these behaviors.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21161
2020-04-23 10:14:52 -07:00
epriestley
0ede616f31 Update the "View Options" menu for recent filetree changes
Summary:
Ref T13516. Minor improvements here:

  - Show key commands in the "View Options" dropdown.
  - Organize it slightly better.
  - Improve disabled item behaviors a little bit.
  - Add a "Browse Directory" action.
  - Rename "...in Diffusion" to "...in Repository".
  - Make "d", "D", and "h" use the same targeting rules as "\".
  - When you hide a file with the "h" menu item, select it.

Test Plan: Poked at the menu a lot, ran into less questionable behavior.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21160
2020-04-23 08:23:12 -07:00
epriestley
60de1506fe Make "hidden" changesets sticky, and show hidden state in the filetree
Summary:
Ref T13455. Make "hidden" a changeset property similar to other changeset properties.

We don't need to render this on the server, so we make a request (to update the setting) and just discard the response.

Test Plan: {F7375468}

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21158
2020-04-22 16:12:42 -07:00
epriestley
a72a66caa8 Mark "low importance" and "owned" changes in the filetree
Summary: Ref T13516. Mark low-importance changes (generated code, deleted files) and owned-with-authority changes in the filetree.

Test Plan: {F7375327}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21157
2020-04-22 11:22:34 -07:00
epriestley
ff88eb588e Show change information in file icons in the filetree
Summary: Ref T13516. Restores "deleted"/"added" information to the tree icons.

Test Plan: {F7375145}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21156
2020-04-22 08:38:29 -07:00
epriestley
9550ae6984 When a directory has a single directory child, collapse them into a single "a/b/" path entry
Summary:
Ref T13516. Instead of rendering trees like this:

  - a/
    - b/
      - c.txt

...render:

  - a/b/
    - c.txt

Test Plan: {F7374205}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21155
2020-04-22 08:37:03 -07:00
epriestley
12eddb18fb Entirely replace the old filetree UI with the "flank" UI
Summary:
Ref T13516. Deletes all old filetree / flex / active / collapse nav code in favor of the new code.

Restores the inline tips in the path tree.

Test Plan: {F7374175}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21154
2020-04-22 08:32:02 -07:00
epriestley
ba8071bbef Roughly style the new "flank" paths UI
Summary: Ref T13516. Apply basic UI styling to the new UI and make some more interaction work.

Test Plan: {F7374096}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21153
2020-04-22 08:31:40 -07:00
epriestley
8cd1f9a309 Generate file trees from changesets in the new flank UI
Summary: Ref T13516. Generate a tree structure based on the page changesets. Still missing styles and a whole lot of behavior.

Test Plan: {F7373967}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21152
2020-04-22 08:31:17 -07:00
epriestley
646280972b Glue the new FormationView on top of the older Filetree view in Differential
Summary: Ref T13516. This glues "FormationView" to "ChangesetList". The actual tree is not functional in any meaningful way yet.

Test Plan: {F7373838}

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21151
2020-04-22 08:29:04 -07:00
epriestley
fef2cdabfe Add a "FormationView" to support dynamic flank panels
Summary:
Ref T13516. Currently, the "File Tree" element is a semi-dynamic side panel that's implemented as a special mode of a side nav panel.

This implementation is fairly clunky, and arose from organic growth out of the side nav. As such, it has some weird behaviors, doesn't have builtin support for show/hide, and can't generalize easily.

Introduce a "FormationView" which supports loading a page up with piles of side panels in various modes.

Test Plan: No callers and no user-visible impact.

Maniphest Tasks: T13516

Differential Revision: https://secure.phabricator.com/D21150
2020-04-22 08:23:21 -07:00
epriestley
ef69c7969f Restore editor behavior to Diffusion and support "\" shortcut
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.

The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".

Test Plan:
  - Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
  - Clicked a line, hit "\", got the file opened to that line.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21149
2020-04-19 09:41:37 -07:00
epriestley
537ff68edd In Differential, make the "Open in Editor" keystroke work with no selection, or a change or inline selected
Summary:
Ref T13515. Currently, "Open in Editor" only works with a file-level selection.

  - If we have a change-level or inline-level selection, open the parent changeset.
  - If we have no selection, but the banner is showing something, open the fine shown in the banner.

Test Plan: With files, inlines, changes, and no selection, pressed "\". Saw files pop open in my external editor.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21148
2020-04-19 09:41:03 -07:00
epriestley
8bdc713352 Make the "Keyboard Shortcuts" dialog in Differential less hideous
Summary:
Ref T13515. Adding "\" ("Open in External Editor") made this slighlty worse, but it was already pretty bad.

Long ago the keys had a special style on them, but this got changed and dropped somewhere around D16568 -- although at the time, I think they still had a grey background (see T11654).

Some later change removed this background.

Put the background back and separate the keystrokes into groups.

Test Plan: {F7370615}

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21141
2020-04-19 09:01:07 -07:00
epriestley
c3c55d82ae Make "renderer", "engine", and "encoding" sticky across reloads in Differential and Diffusion
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.

Some complexity here arises from these concerns:

  - In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
  - In all other cases, we render the changeset with AJAX.

So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.

Then, some bookkeeping issues:

  - At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
  - Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".

Test Plan:
  - Viewed changes in Differential, Diffusion, and in standalone mode.
  - Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
  - Started typing a comment, cancelled it, hit the undo UI.

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21138
2020-04-19 08:59:09 -07:00
epriestley
3d966d8a41 Add an "Open in External Editor" keystroke to Differential
Summary: Ref T13515. See PHI1661. If a file is selected, add a keystroke to click the "Open in External Editor" link.

Test Plan: In Safari, Chrome, and Firefox: used "J" to select a file, then "\" to open it in an external editor. (In Safari and Chrome, this prompts.)

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21135
2020-04-17 10:06:46 -07:00
epriestley
925d2b051c Fix a "flickering" behavior with the menu bar transition animations in Chrome
Summary:
Fixes T13508. The "Notification" and "Messages" icons in the menu bar have a CSS transition animation on hover.

In Chrome, when this element moves up 2px, you can get a flicker in and out of the hover state if the user's cursor is at the very bottom of the element, since the bounding box for the element is rapidly sliding in and out of the area under the cursor.

To fix this: as we move the element up, also make it taller.

Test Plan: In Safari, Chrome, and Firefox: put my cursor at the very bottom of the element, no longer saw any animation flickering.

Maniphest Tasks: T13508

Differential Revision: https://secure.phabricator.com/D21133
2020-04-17 06:03:36 -07:00
epriestley
d3f4af4a3a Add more layout constraints to tokenizer CSS to prevent layout issues with Chinese glyphs in Firefox 73
Summary:
Fixes T13495. See that task for details.

Tokenizer tokens which contain Chinese glyphs are slightly taller than normal tokens in Firefox 73, and at some non-100% zoom levels in other browsers.

This cauess the tokenizer list to layout and line break oddly.

Fix this by clamping tokenizer sizes more aggressively. Specifying a `max-height` means they can no longer line wrap, so this also requires more specification of overflow behavior.

Test Plan:
Before:

{F7216435}

After:

{F7216439}

Maniphest Tasks: T13495

Differential Revision: https://secure.phabricator.com/D21026
2020-02-24 08:00:44 -08:00
epriestley
0e82bd024a Use the new "CurtainObjectRefList" UI element for subscribers
Summary:
Depends on D20966. Ref T13486. Curtains currently render subscribers in a plain text list, but the new ref list element is a good fit for this.

Also, improve the sorting and ordering behavior.

This makes the subscriber list take up a bit more space, but it should make it a lot easier to read at a glance.

Test Plan: Viewed object subscriber lists at varying limits and subscriber counts, saw sensible subscriber lists.

Maniphest Tasks: T13486

Differential Revision: https://secure.phabricator.com/D20967
2020-02-04 12:38:41 -08:00
epriestley
2a92fef879 Improve wrapping and overflow behavior for curtain panels containing long usernames
Summary:
Ref T13486. When a curtain element like "Author" in Maniphest has a very long username, the wrapping and overflow behavior is poor: the date is obscured.

Adjust curtain elements which contain lists of references to other objects to improve wrapping behavior (put the date on a separate line) and overflow behavior (so we get a "..." when a name overflows).

Test Plan: {F7179376}

Maniphest Tasks: T13486

Differential Revision: https://secure.phabricator.com/D20966
2020-02-04 12:31:18 -08:00
epriestley
6d4c6924d6 Update Herald rule creation workflow to use more modern UI elements
Summary: Ref T13480. Creating a rule in Herald currently uses the older radio-button flow. Update it to the "clickable menu" flow to simplify it a little bit.

Test Plan: Created new personal, object, and global rules. Hit the object rule error conditions.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20956
2020-02-04 07:37:54 -08:00
epriestley
26c2a1ba68 Move existing "Console" interfaces away from "setFixed(...)" on "TwoColumnView"
Summary: Depends on D20931. Ref T13362. Move all "Console"-style interfaces to use a consistent layout based on a new "LauncherView" which just centers the content.

Test Plan: Viewed all affected interfaces.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13362

Differential Revision: https://secure.phabricator.com/D20933
2020-02-04 06:52:23 -08:00
epriestley
7a1681b8da Don't use "line-through" style for completed items in remarkup checklists
Summary: Fixes T13482. Although this style makes physical sense by relationship to a written checklist, it seems to do more harm than good in practice.

Test Plan: Wrote a checklist with a checked-off item in remarkup, saw no more line-through.

Maniphest Tasks: T13482

Differential Revision: https://secure.phabricator.com/D20954
2020-01-29 08:59:51 -08:00
epriestley
d0b01a41f2 Fix two issues with missing whitespace when elements stack on top of each other while wrapping
Summary: Fixes T13476. Policy tags in object headers and "Visible To" controls in some dialog contexts may stack and wrap oddly. Improve spacing so they don't overlap visually when wrapping.

Test Plan: Viewed affected interfaces in narrow and wide windows.

Maniphest Tasks: T13476

Differential Revision: https://secure.phabricator.com/D20944
2020-01-15 08:52:56 -08:00
epriestley
de66a8ece1 Remove "stronger/weaker" policy color hints from object headers
Summary:
Fixes T13461. Some applications provide hints about policy strength in the header, but these hints are inconsistent and somewhat confusing. They don't make much sense for modern objects with Custom Forms, which don't have a single "default" policy.

Remove this feature since it seems to be confusing things more than illuminating them.

Test Plan:
  - Viewed various objects, no longer saw colored policy hints.
  - Grepped for all removed symbols.

Maniphest Tasks: T13461

Differential Revision: https://secure.phabricator.com/D20918
2019-11-18 22:05:26 -08:00
epriestley
338b4cb2e7 Prevent workboard cards from being grabbed by the "Txxx" object name text
Summary:
Fixes T13452. We currently give users mixed signals about the interaction mode of this text: the cursor says "text" but the behavior is "grab".

Make the behavior "text" to align with the cursor. An alternate variation of this change is to remove the cursor, but this is preferable if it doesn't cause problems, since copying the task ID is at least somewhat useful.

Test Plan: In Safari, Firefox, and Chrome: selected and copied object names from workboard cards; and dragged workboard cards by other parts of their UI.

Maniphest Tasks: T13452

Differential Revision: https://secure.phabricator.com/D20898
2019-11-08 08:29:53 -08:00
epriestley
e46e383bf2 Clean up "Revisions/Commits" table in Maniphest slightly
Summary: Ref T13440. Give the table more obvious visual structure and get rid of the largely useless header columns.

Test Plan: Viewed table, saw a slightly cleaner result.

Maniphest Tasks: T13440

Differential Revision: https://secure.phabricator.com/D20885
2019-10-31 12:29:53 -07:00
epriestley
c48f300eb1 Add support for rendering section dividers in tables; use section dividers for changes on tasks
Summary: Depends on D20883. Ref T13440. In most cases, all changes belong to the same repository, which makes the "Repository" column redundant and visually noisy. Show repository information in a section header.

Test Plan: {F6989932}

Maniphest Tasks: T13440

Differential Revision: https://secure.phabricator.com/D20884
2019-10-31 12:13:25 -07:00
epriestley
e1da1d86d6 Trim and URI encode symbol names before building URIs from them
Summary:
Fixes T13437. This URI construction was just missing URI encoding.

Also, trim the symbol because my test case ended up catching "#define\n" as symbol text.

Test Plan:
  - Configured a repository to have PHP symbols.
  - Touched a ".php" file with "#define" in it.
  - Diffed the change.
  - Command-clicked "#define" in the UI, in Safari/MacOS, to jump to the definition.
    - Before: taken to a nonsense page where "#define" became an anchor.
    - After: taken to symbol search for "#define".

Maniphest Tasks: T13437

Differential Revision: https://secure.phabricator.com/D20876
2019-10-29 09:48:42 -07:00
epriestley
633aa5288c Persist login instructions onto flow-specific login pages (username/password and LDAP)
Summary:
Fixes T13433. Currently, "Login Screen Instructions" in "Auth" are shown only on the main login screen. If you enter a bad password or bad LDAP credential set and move to the flow-specific login failure screen (for example, "invalid password"), the instructions vanish.

Instead, persist them. There are reasonable cases where this is highly useful and the cases which spring to mind where this is possibly misleading are fairly easy to fix by making the instructions more specific.

Test Plan:
  - Configured login instructions in "Auth".
  - Viewed main login screen, saw instructions.
  - Entered a bad username/password and a bad LDAP credential set, got kicked to workflow sub-pages and still saw instructions (previously: no instructions).
  - Grepped for other callers to `buildProviderPageResponse()` to look for anything weird, came up empty.

Maniphest Tasks: T13433

Differential Revision: https://secure.phabricator.com/D20863
2019-10-24 18:38:15 -07:00
epriestley
344a2e39be In Jupyter notebooks, apply intraline diffing to source code lines
Summary: Ref T13425. When we render a diff between two source lines, highlight intraline changes.

Test Plan: Viewed a Jupyter notebook with a code diff in it, saw the changed subsequences in the line highlighted.

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20851
2019-10-02 12:34:59 -07:00
epriestley
5afdc620db Make basic Juypter notebook rendering improvements and roughly support folding unchanged context
Summary:
Depends on D20843. Ref T13425. Add very basic support for "Show Hidden Context", in the form of folding it behind an unclickable shield. This isn't ideal, but should be better than nothing.

Prepare for "intraline" diffs on content blocks.

Fix newline handling in Markdown sections in Jupyter notebooks.

Remove the word "visibile" from the codebase.

Test Plan: {F6898192}

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20844
2019-09-30 10:41:21 -07:00
epriestley
2c06815edb When rendering Jupyter notebook diffs, split code inputs into individual blocks
Summary:
Ref T13425. Currently, code inputs and all outputs are grouped into a single block. This is fine for display notebooks but not great for diffing notebooks.

Instead, split source code input into individual lines with one line per block, and each output into its own block.

This allows you to leave actual line-by-line inlines on source, and comment on outputs individually.

Test Plan: {F6888583}

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20840
2019-09-25 21:05:18 -07:00
epriestley
281598d65c Use a hash-and-diff strategy to produce a diff layout for block-based documents
Summary:
Depends on D20835. Ref T13425. Ref T13414. When a document has a list of content blocks, we may not be able to diff it directly, but we can hash each block and then diff the hashes (internally "diff" also does approximately the same thing).

We could do this ourselves with slightly fewer layers of indirection, but: diff already exists; we already use it; we already have a bunch of abstractions on top of it; and it's likely much faster on large inputs than the best we can do in PHP.

Test Plan: {F6888169}

Maniphest Tasks: T13425, T13414

Differential Revision: https://secure.phabricator.com/D20836
2019-09-25 16:40:53 -07:00
epriestley
932d829af3 Improve behavior of inline comment highlight reticle for block diffs
Summary:
Depends on D20834. Ref T13425. After the change from "th" to "td" for accessibility, the algorithm picks which cells it should highlight slightly improperly (it picks too many cells since it can no longer find the line numbers).

Ideally, it would probably highlight //only// the source content, but there isn't an easy way to do this right now. Settle for an incremental improvement for the moment.

Test Plan: Hovered over line numbers, saw a more accurate highlight area.

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20835
2019-09-25 16:39:18 -07:00
epriestley
1c4450d39f Allow the Jupyter engine to elect to emit diffs, and emit Jupyter documents as blocks
Summary:
Depends on D20832. Ref T13425. Emit Jupyter notebooks as diffable blocks with block keys.

No diffing or proper inlines yet.

Test Plan: {F6888058}

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20833
2019-09-25 16:32:36 -07:00
epriestley
7ae711ed3e Add a "View as..." option to diff dropdowns for selecting between document engines
Summary:
Depends on D20831. Ref T13425. As an escape hatch to get out of future DocumentEngine rendering behavior, provide a "View As.." option.

Now I can break DocumentEngine real bad and no one can complain.

Test Plan: Used "View As" to swap document engines for image files.

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20832
2019-09-25 16:29:21 -07:00
epriestley
bb71ef6ad6 Render image diffs as abstract blocks diffs via DocumentEngine
Summary:
Depends on D20830. Ref T13425. Have the image engine elect into block rendering, then emit blocks.

This is rough (the blocks aren't actually diffed yet) but image diffs were already pretty rough so this is approximately a net improvement.

Test Plan: Viewed image diffs, saw nothing worse than before.

Maniphest Tasks: T13425

Differential Revision: https://secure.phabricator.com/D20831
2019-09-25 16:25:06 -07:00
epriestley
74d6bcbdce Allow a user to target "#anchor" by navigating to any prefix
Summary:
Ref T13410. We currently generate some less-than-ideal anchors in remarkup, but it's hard to change the algorithm without breaking stuff.

To mitigate this, allow `#xyz` to match any target on the page which begins with `xyz`. This means we can make anchors longer with no damage, and savvy users are free to shorten anchors to produce more presentation-friendly links.

Test Plan: Browsed to `#header-th`, was scrolled to `#header-three`, etc.

Maniphest Tasks: T13410

Differential Revision: https://secure.phabricator.com/D20820
2019-09-24 10:56:35 -07:00
epriestley
d4ed5d0428 Make various UX improvements to charts so they're closer to making visual sense
Summary: Ref T13279. Fix some tabular stuff, draw areas better, make the "compose()" API more consistent, unfatal the demo chart, unfatal the project burndown, make the project chart do something roughly physical.

Test Plan: Looked at charts, saw fewer obvious horrors.

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20817
2019-09-17 09:43:21 -07:00
epriestley
f529abf900 In stacked area charts, group nearby points so they don't overlap
Summary: Ref T13279. We currently draw a point on the chart for each datapoint, but this leads to many overlapping circles. Instead, aggregate the raw points into display points ("events") at the end.

Test Plan: Viewed a stacked area chart with many points, saw a more palatable number of drawn dots.

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20814
2019-09-17 09:26:54 -07:00
epriestley
3dcb4a7b50 Work around rendering engine freeze in Chrome 77 affecting workboards
Summary:
Ref T13413. In Chrome 77, workboard cards with titles that must break in the middle of words cause the browser to completely lock up.

Work around the major known instance of this by overriding the "break-word" behavior. This gives us worse rendering for tasks with very long "words" in their titles (they are truncated instead of broken) but fixes the freezing.

Once Chrome is fixed, this can be reverted.

Test Plan:
  - Created a task named "MMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMM" on a workboard.
  - Loaded the board in Chrome 77.
  - Before: entire page locks up.
  - After: smooth sailing, except the "MMMMMM..." is truncated.

Maniphest Tasks: T13413

Differential Revision: https://secure.phabricator.com/D20812
2019-09-12 19:04:41 -07:00
epriestley
d60d4e6a05 Don't present users with Herald fields/actions for uninstalled applications, unless the rule already uses them
Summary:
Fixes T7961. Currently, we present Herald users with actions like "Require legalpad signatures" and "Run build plans" even if Legalpad and Harbormaster are not installed.

Instead, allow fields and actions to be made "unavailable", which means that we won't present them as options when adding to new or existing rules.

If you edit a rule which already uses one of these fields or actions, it isn't affected.

Test Plan:
  - Created a rule with a legalpad action, uninstalled legalpad, edited the rule. Action remained untouched.
  - Created a new rule, wasn't offered the legalpad action.
  - Reinstalled the application, saw the action again.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T7961

Differential Revision: https://secure.phabricator.com/D20808
2019-09-12 14:33:28 -07:00
epriestley
9a36e6931c Inline custom policy rules inside policy capability explanation dialogs
Summary: Ref T13411. When users click a link to explain a capability (like the policy header on many objects, or the link next to specific capabilities in "Applications", "Diffusion", etc), inline the full ruleset for the custom policy into the dialog if the object has a custom policy.

Test Plan: {F6856365}

Maniphest Tasks: T13411

Differential Revision: https://secure.phabricator.com/D20805
2019-09-12 09:40:50 -07:00
epriestley
adc2002d28 Make it easier to parse "X-Forwarded-For" with one or more load balancers
Summary:
Fixes T13392. If you have 17 load balancers in sequence, Phabricator will receive requests with at least 17 "X-Forwarded-For" components in the header.

We want to select the 17th-from-last element, since prior elements are not trustworthy.

This currently isn't very easy/obvious, and you have to add a kind of sketchy piece of custom code to `preamble.php` to do any "X-Forwarded-For" parsing. Make handling this correctly easier.

Test Plan:
  - Ran unit tests.
  - Configured my local `preamble.php` to call `preamble_trust_x_forwarded_for_header(4)`, then made `/debug/` dump the header and the final value of `REMOTE_ADDR`.

```
$ curl http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR =
   FINAL REMOTE_ADDR = 127.0.0.1
</pre>
```

```
$ curl -H 'X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6
   FINAL REMOTE_ADDR = 3.3.3.3
</pre>
```

```
$ curl -H 'X-Forwarded-For: 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>

HTTP_X_FORWARDED_FOR = 5.5.5.5, 6.6.6.6
   FINAL REMOTE_ADDR = 5.5.5.5
</pre>
```

Maniphest Tasks: T13392

Differential Revision: https://secure.phabricator.com/D20785
2019-09-05 04:30:13 -07:00
epriestley
201634848e Make Phortune payment methods transaction-oriented and always support "Add Payment Method"
Summary:
Depends on D20718. Ref T13366. Ref T13367.

  - Phortune payment methods currently do not use transactions; update them.
  - Give them a proper view page with a transaction log.
  - Add an "Add Payment Method" button which always works.
  - Show which subscriptions a payment method is associated with.
  - Get rid of the "Active" status indicator since we now treat "disabled" as "removed", to align with user expectation/intent.
  - Swap out of some of the super weird div-form-button UI into the new "big, clickable" UI for choice dialogs among a small number of options on a single dimension.

Test Plan:
  - As a mechant-authority and account-authority, created payment methods from carts, subscriptions, and accounts. Edited and viewed payment methods.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13367, T13366

Differential Revision: https://secure.phabricator.com/D20719
2019-08-22 21:04:04 -07:00
epriestley
1fe6311167 Modernize user and repository "delete" workflows and improve documentation
Summary:
Fixes T8830. Fixes T13364.

  - The inability to destroy objects from the web UI is intentional. Make this clear in the messaging, which is somewhat out of date and partly reflects an earlier era when things could be destroyed.
  - `bin/remove destroy` can't rewind time. Document expectations around the "put the cat back in the bag" use case.

Test Plan: Read documentation, clicked through both workflows.

Maniphest Tasks: T13364, T8830

Differential Revision: https://secure.phabricator.com/D20694
2019-08-02 09:30:50 -07:00
epriestley
3069ef4166 Prevent object titles in the "Object Attacher" dialog from triggering Quicksand "Close Dialog on Navigation" behavior
Summary:
Fixes T13363. Currently, these are genuine links which we intercept events for.

Make them pseudolinks instead. Possible alternative approaches are:

  - Keep them as genuine links, but mark them as non-navigation links for Quicksand. (But: yuck, weird special case.)
  - Keep them as genuine links, and have the dialog handler `JX.Stratcom.pass()` to see if anything handles the event. (But: the "pass()" pattern generally feels bad.)

"Tableaus" or whatever comes out of T10469 some day will probably break everything anyway?

Test Plan:
  - Opened the "Edit Related Tasks... > Edit Subtasks" dialog.
  - Clicked task title links (not the "open in new window" icon, and not the "Select" button).
  - Before: Dialog (sometimes) closed abruptly.
  - After: Task is consistently selected as part of the attachment set.

Maniphest Tasks: T13363

Differential Revision: https://secure.phabricator.com/D20693
2019-08-01 12:25:28 -07:00
epriestley
7d41535010 When a task card is edited, emit update events for old boards and parent boards
Summary:
Ref T4900. When a card is edited, we currently emit an update notification for all the projects the task is tagged with. This isn't quite the right set:

  - We want to emit notifications for projects the task //was previously// tagged with, so it can be removed from boards it should no longer be part of.
  - We want to emit notifications for ancestors of projects the task is or was tagged with, so parent project boards can be updated.
  - However, we don't need to emit notifications for projects that don't actually have workboards.

Adjust the notification set to align better to these rules.

Test Plan:
  - Removal of Parent Project: Edited a task on board "A > B", removing the "B" project tag. Saw board A update in another window.
  - Normal Update: Edited a task title on board X, saw board X update in another window.
  - Used `bin/aphlict debug` to inspect the notification set, saw generally sensible-seeming data going over the wire.

Reviewers: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20680
2019-07-30 13:16:33 -07:00
epriestley
17caecdda3 Make workboard real-time updates mostly work
Summary:
Depends on D20654. Ref T4900. When a task is edited, emit a "workboards" event for all boards it appears on (in a future change, this should also include all boards it //previously// appeared on, and all parents of both sets of boards -- but I'm just getting things working for now).

When we receive a "workboards" event, check if the visible board should be updated.

Aphlict has a complicated intra-window leader/follower election system which could let us process this update event exactly once no matter how many windows a user has open with the same workboard. I'm not trying to do any of this since it seems fairly rare. It makes sense for events like "you have new notifications" where we don't want to generate 100 Ajax calls if the user has 100 windows open, but very few users seem likely to have 100 copies of the same workboard open.

Test Plan:
  - Ran `bin/aphlict debug`.
  - Opened workboard A in two windows, X and Y.
  - Edited and moved tasks in window X.
  - Saw "workboards" messages in the Aphlict log.
  - Saw window Y update in nearly-real-time (locally, this is fast enough that it feels instantaneous).

Then:

  - Stopped the Aphlcit server.
  - Edited a task.
  - Started the Aphlict server.
  - Saw window Y update after a few moments (i.e., update in response to a reconnect).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20656
2019-07-18 10:00:17 -07:00
epriestley
d02beaf816 Make reloading workboards with "R" respect workboard ordering
Summary:
Depends on D20653. Ref T4900. Pass ordering details to the reload endpoint so it can give the client accurate ordering/header information in the response.

The removed comment mentions this, but here's why this is a difficult mess:

  - In window A, view a board with "Group by: Owner" and no tasks owned by "Alice". Since "Alice" owns no tasks, this means the columns do not have an "Assigned to: Alice" header!
  - In window B, edit task T and assign it to Alice.
  - In window A, press "R".

Window A now not only needs to update to properly reflect the state of task T, it actually needs to draw a new "Assigned to: Alice" header in every column.

Fortunately, the "group by" code anticipates this being a big mess, is fairly careful about handling it, and the client can handle this state change and the actual code change here isn't too involved. This is just causing a lot of not-very-obvious indirect effects in the pipeline to handle these situations that need complex redraws.

Test Plan:
  - After making various normal edits/creates/moves in window A, pressed "R" in window B. Saw ordering reflected correctly after sync.
  - Went through the whole "Group by: Owner" + assign to unrepresented owner flow above. After pressing "R", saw "Assigned to: Alice" appear on the board.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20654
2019-07-17 13:17:00 -07:00
epriestley
8669c3c0d2 When updating a workboard with "R", send the client visible set with version numbers
Summary:
Depends on D20652. Ref T4900. When the user presses "R", send a list of cards currently visible on the client and their version numbers.

On the server:

  - Compare the client verisons to the server versions so we can skip updates for objects which have not changed. (For now, the client version is always "1" and the server version is always "2", so this doesn't do anything meaningful, and every card is always updated.)
  - Compare the client visible set to the server visible set and "remove" any cards which have been removed from the board.

I believe this means that "R" always puts the board into the right state (except for some issues with client orderings not being fully handled yet). It's not tremendously efficient, but we can make versioning better (using the largest object transaction ID) to improve that and loading the page in the first place doesn't take all that long so even sending down the full visible set shouldn't be a huge problem.

Test Plan:
  - In window A, removed a card from a board.
  - In window B, pressed "R" and saw the removal reflected on the client.
  - (Also added cards, edited cards, etc., and didn't catch anything exploding.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20653
2019-07-17 13:16:03 -07:00
epriestley
1ee6ecf397 Move "BoardResponseEngine" toward a more comprehensive update model
Summary:
Depends on D20639. Ref T4900. Currently, "BoardResponseEngine" has a `setObjectPHID()` method. This is called after edit operations to mean "we just edited object X, so we know it needs to be updated".

Move toward `setUpdatePHIDs(...)` in all cases, with `setUpdatePHIDs(array(the-object-we-just-edited))` as a special case of that. After this change, callers pass:

  - An optional list of PHIDs they know need to be updated on the client. Today, this is always be a card we just edited (on edit/move flows), or a sort of made-up list of PHIDs for the moment (when you press "R"). In the future, the "R" endpoint will do a better job of figuring out a more realistic update set.
  - An optional list of PHIDs currently visible on the client. This is used to update ordering details and mark cards for removal. This is currently passed by edit/move, but not by pressing "R" (it will be in the future).
  - An optional list of objects. The "R" workflow has to load these anyway, so we can save a couple queries by letting callers pass them. For now, the edit/move flows still rely on the engine to figure out what it needs to load.

This does very little to actually change client behavior, it mostly just paves the way for the next update to the "R" workflow to make it handle add/remove cases properly.

Test Plan:
  - Edited and moved cards on a workboard.
  - Pressed "R" to reload a workboard.

Neither of these operations seem any worse off than they were before. They still don't fully work:

  - When you edit a card and delete the current workboard project from it, it remains visible. This is also the behavior on `master`. This is sort of intentional since we don't necessarily want to make these cards suddenly disappear? Ideally, we would probably have some kind of "tombstone" state where the card can still be edited but can't be dragged, and the next explicit user interaction would clean up old tombstones. This interaction is very rare and I don't think it's particularly important to specialize.
  - When a card is removed from the board, "R" can't currently figure out that it should be removed from the client. This is because the client does not yet pass a "visiblePHIDs" state. It will in an upcoming change.
  - The "R" flow always sends a full set of card updates, and can not yet detect that some cards have not changed.
  - There's a TODO, but some ordering stuff isn't handled yet.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20652
2019-07-17 13:13:15 -07:00
epriestley
db69686927 Make pressing "R" on your keyboard reload the card state on workboards
Summary:
Depends on D20638. Ref T4900. This is an incremental step toward proper workboard updates.

Currently, the client can mostly update its view because we do updates when you edit or move a card, and the client and server know how to send lists of card updates, so a lot of the work is already done.

However, the code assumes we're only updating/redrawing one card at a time. Make the client accept and process multiple card updates.

In future changes, I'll add versioning (so we only update cards that have actually changed), fix the "TODO" around ordering, and move toward actual Aphlict-based real-time updates.

Test Plan:
  - Opened the same workboard in two windows.
  - Edited cards in one window, pressed "R" (capital letter, with no modifier keys) to reload the second window.
  - Saw edits and moves reflected accurately after sync, except for some special cases of header/order interaction (see "TODO").

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20639
2019-07-17 13:11:26 -07:00
epriestley
02315c4c48 Fix double-close on dialogs leading to Javascript console error
Summary:
Ref T13302. The "Close/Cancel" button is currently running two copies of the "dismiss dialog" code, since it's techncally a link with a valid HREF attribute.

An alternate formulation of this is perhaps `if (JX.Stratcom.pass()) { return; }` ("let other handlers react to this event; if something kills it, stop processing"), but `pass()` is inherently someone spooky/fragile so try to get away without it.

Test Plan: Opened the Javascript console, clicked "Edit Task" on a workboard, clicked "Close" on the dialog. Before: event was double-handled leading to a JS error in the console. After: dialog closes uneventfully.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13302

Differential Revision: https://secure.phabricator.com/D20640
2019-07-03 12:38:49 -07:00
epriestley
fc795994f3 Remove obsolete "options" from workboard "updateCard()" call
Summary: Depends on D20637. Ref T4900. This is some ancient dead code that nothing uses.

Test Plan: Grepped for `updateCard()` to verify it's private. Searched for "options" and "dirtyColumn" and got no hits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T4900

Differential Revision: https://secure.phabricator.com/D20638
2019-07-03 10:06:13 -07:00
epriestley
d6dc5d8e68 Fix the "x" link in tokenizer tokens incorrectly closing dialogs
Summary:
See <https://discourse.phabricator-community.org/t/removing-tags-or-subscribers-from-a-maniphest-modal-causes-the-modal-to-close/2874>.

My Javascript is rusty: `'' + null == 'null'`. Same for `undefined`. Use an explicit typecheck instead.

Test Plan: Clicked the "x" in a tokenizer token in a dialog, saw the token removed instead of the dialog closed.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20623
2019-06-28 12:38:51 -07:00
epriestley
9f44ee3933 Use "link.getAttribute('href')", not "link.href", to bypass dark browser magic
Summary:
Ref T13302. In at least some browsers (including Safari and Chrome), when you write this:

```
<a href="#">...</a>
```

...and then access `<that node>.href`, you get `http://local-domain-whatever.com/path/to/current/page#` back.

This is wonderful, but not what we want. Access the raw attribute value instead, which is `#` in all browsers.

Test Plan:
  - In Safari, Chrome, and Firefox:
  - Clicked "Edit Subtasks" from a task.
  - Clicked "Select" buttons to select several tasks.
  - Before: Clicking these button incorrectly closed the dialog (because of D20573).
  - After: Clicking these buttons now selects tasks without closing the dialog.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13302

Differential Revision: https://secure.phabricator.com/D20590
2019-06-19 10:44:42 -07:00
epriestley
dcf3ca8e04 When a user clicks a navigation link in a dialog, close the dialog
Summary:
Ref T13302. Currently, if you enable Quicksand (by clicking "Persistent Chat"), open a dialog with links in it (like "Create Subtask" with multiple available subtypes), and then follow a navigation link, the page content reloads behind the dialog but the dialog stays in the foreground.

Fix this by closing dialogs when users click navigation links inside them.

Test Plan:
With Quicksand enabled and disabled, clicked a subtask type in the "Create Subtask" dialog.

  - Before, Quicksand Disabled: Dialog stays on screen, then navigation occurs.
  - After, Quicksand Disabled: Dialog vanishes, then navigation occurs.
  - Before, Quicksand Enabled: Dialog stays on screen, navigation occurs behind it.
  - After, Quicksand Enabled: Dialog vanishes, then navigation occurs.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13302

Differential Revision: https://secure.phabricator.com/D20573
2019-06-18 15:16:32 -07:00
epriestley
b7aacaa4d3 Differentiate Remarkup header sizes more clearly
Summary:
Ref PHI1275. Previously, see T591. See also T7963. Headers are currently very visually similar to one another, and similar to the text size:

{F6485441}

I think the design intent was to make it hard to make bad-looking documents, but all the headers end up being very samey.

Differentiate the sizes of the headers better so they're much more obvious (e.g., when scrolling through a document) and the different levels are more distinct.

This might be a little overboard, but we can always pull it back a bit if it's too much, and I think giving users more control in Remarkup (in cases where it doesn't create some weird syntax/parsing nightmare) is generally a good thing.

Test Plan: {F6485447}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20569
2019-06-04 16:03:32 -07:00
epriestley
a80426b339 Provide chart function labels over the wire instead of making them up
Summary: Ref T13279. Makes charts incrementally more useful by allowing the server to provide labels and colors for functions.

Test Plan: {F6438872}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20501
2019-05-22 05:22:59 -07:00
epriestley
c6052b41a6 Label important data on charts
Summary:
Ref T13279. Adds client-side support for rendering function labels on charts, then labels every function as important data.

Works okay on mobile, although I'm not planning to target mobile terribly heavily for v0.

Test Plan: {F6438860}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20500
2019-05-22 05:21:26 -07:00
epriestley
81456db559 Roughly support stacked area charts
Summary:
Ref T13279. This adds support for:

  - Datasets can have types, like "stacked area".
  - Datasets can have multiple functions.
  - Charts can store dataset types and datasets with multiple functions.
  - Adds a "stacked area" dataset.
  - Makes D3 actually draw a stacked area chart.

Lots of rough edges here still, but the result looks slightly more like it's supposed to look.

D3 can do some of this logic itself, like adding up the area stacks on top of one another with `d3.stack()`. I'm doing it in PHP instead because I think it's a bit easier to debug, and it gives us more options for things like caching or "export to CSV" or "export to API" or rendering a data table under the chart or whatever.

Test Plan: {F6427780}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20498
2019-05-22 05:19:41 -07:00
epriestley
0776b5ca2c Update D3 to the current version
Summary:
Ref T13279. Old D3 seems perfectly fine, but most of the good references seem to have been written by people who update D3 more than once every 10 years (???).

This requires some minor API changes, see next diff.

Test Plan: See next diff.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20497
2019-05-22 05:16:00 -07:00
epriestley
146317f2c4 Remove the legacy chart behavior from Maniphest
Summary: Depends on D20486. Ref T13279. Now that the "Reports" UI uses a panel to draw a real chart from Facts, throw away the copy of the old code.

Test Plan: `grep`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20487
2019-05-22 04:48:24 -07:00
epriestley
aba7d1406a Really fix movable panels inside tab panels by changing the JX.Request serializer
Summary:
Depends on D20475. Ref T13272. Currently, if you `JX.Request` with `data` like `{x: null}`, we submit that as `?x=null`, i.e. as though `null` was the string `"null"`.

This is weird and almost certainly never intended/desiarable. In particular, it causes a bug where panels embedded inside tab panels are incorrectly draggable.

It's possible this breaks something which relied on the buggy behavior, but that seems unlikely.

Test Plan: Tried to drag a panel inside a tab panel, it really truly didn't work.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20476
2019-05-01 15:38:40 -07:00
epriestley
e1076528ef Copy the "line-chart" behavior to "line-chart-legacy" to keep "Maniphest > Reports" working
Summary:
Ref T13279. Charting changes alter how the "line-chart" behavior works, but the "Burnup Chart" still relies on the old behavior.

Although I'm intending to remove "Maniphest > Reports" once Facts is a minimally sufficient replacement, copy this behavior to keep it working until we're ready to pull the trigger.

Also fix a leftover typo from D20435.

Test Plan: Viewed a legacy Maniphest burnup rate report.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20449
2019-04-19 07:05:37 -07:00
epriestley
45b3c23148 Fetch chart data via async request and redraw charts when the window is resized
Summary:
Depends on D20439. Ref T13279. Some day, charts will probably need to reload themselves or do a bunch of defer/request-shaping magic when they're on a dashboard with 900 other charts.

Give the controller separate "HTML placeholder" and "actual data" modes, and make the placeholder fetch the data in a separate request.

Then, make the chart redraw if you resize the window instead of staying at whatever size it started as.

Test Plan:
  - Loaded a chart, saw it load data asynchronously.
  - Resized the window, saw the chart resize.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20440
2019-04-18 07:10:05 -07:00
epriestley
044c6fbc19 Support drawing multiple functions on the same chart
Summary: Depends on D20438. Ref T13279. Widgets produced vs widgets sold, etc.

Test Plan: {F6381609}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20439
2019-04-18 07:07:33 -07:00
epriestley
02981f0add Fix negative chart values, add storage for charts
Summary:
Ref T13279. I think I'm going to fling some stuff at the wall for a bit here and hope most of it sticks, so this series of changes may not be terribly cohesive or focused. Here:

The range of the chart is locked to "[0, 105% of max]". This is trying to make a pleasing extra margin above the maximum value, but currently just breaks charts with negative values. Later:

    - I'll probably let users customize this.
    - We should likely select 0 as the automatic minimum for charts with no negative values.
    - For charts with positive values, it would be nice to automatically pick a pleasantly round number (25, 100, 1000) as a maximum by default.

We don't have any storage for charts yet. Add some. This works like queries, where every possible configuration gets a short URL slug. Nothing writes or reads this yet.

Rename `fn()` to `css_function()`. This builds CSS functions for D3. The JS is likely to get substantial structural rewrites later on, `fn()` was just particularly offensive.

Test Plan: Viewed a fact series with negative values. Ran `bin/storage upgrade`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20438
2019-04-18 06:59:41 -07:00
epriestley
870b01f2d0 Distinguish between "bad record format" and "bad record value" when validating Trigger rules
Summary:
Depends on D20416. Ref T13269. See D20329.

If you try to save an "Assign to" rule with no assignee, we currently replace the control with an "InvalidRule" control that isn't editable. We'd prefer to give you an empty field back and let you pick a different value.

Differentiate between "bad record format" (i.e., we can't really do anything with this) and "bad record value" (i.e., everything is structurally fine, you just typed the wrong thing). In the latter case, we still build a properly typed rule for the UI, we just refuse to update storage until you fix the problem.

Test Plan:
First, hit the original issue and got a nicer UI with a more consistent control width (note full-width error):

{F6374205}

Then, applied the rest of the patch and got a normal "fix the issue" form state instead of a dead-end:

{F6374211}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20417
2019-04-17 12:40:55 -07:00
epriestley
9532bfbb32 Improve trigger editor behavior when switching to/from tokenizers
Summary:
Ref T13269. See D20329. When we switch trigger rule control types, reset the rule value.

Also, pick slightly nicer defaults for status/priority.

Test Plan:
  - Created a "Change Status To: X" rule.
  - Saved it.
  - Edited it.
  - Selected "Assign to" for the existing action's dropdown.
  - Before: tokenizer filled with nonsense.
  - After: tokenizer cleared.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20416
2019-04-17 12:24:23 -07:00
epriestley
b8551bb5f9 Reduce drag-and-drop jank on dashboards
Summary:
Depends on D20414. Ref T13272. Several minor things here:

  - Currently, you can drag panels underneath the invisible "there are no items in this column" div and the "Create Panel / Add Existing Panel" buttons. This is silly; stop it.
  - Currently, when viewing a tab panel on a dashboard, you can drag the panels inside it. This is extremely silly. Make "movable" off by default and pass it through the async flow only when we actually need it.
  - Make the whole "Add Tab..." virtual tab clickable to open the dropdown. This removes the rare exception/todo combo I added earlier. {key F}
  - Add or remove some icons or something.

Test Plan: Moved panels around on dashboards. Tried to drag panels inside tab panels. Added tab. Things were less obviously broken.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20415
2019-04-17 12:20:44 -07:00
Austin McKinley
0583f6dc50 Some formatting changes for showing auth provider config guidance
Summary:
Ref T7667. On the road to locking the auth config, also clean up some minor UI issues:

* Only show the warning about not Phacility instance auth if the user isn't a manager (see next diff).
* When rendering more than one warning in the guidance, add bullets.
* I didn't like the text in the `auth.config-lock` config setting.

Test Plan: Loaded the page, saw more reasonable-looking guidance: {F6369405}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T7667

Differential Revision: https://secure.phabricator.com/D20400
2019-04-17 11:08:16 -07:00
epriestley
f13709b13b Update search indexes for Dashboards and Panels to Ferret, plus various minor fixes
Summary:
Depends on D20410. Ref T13272. Dashboards/Panels currently use older "ngram" indexing, which is a less-powerful precursor to Ferret. Throw away the ngram index and provide a Ferret index instead. Also:

  - Remove the NUX state, which links to the wrong place now and doesn't seem terribly important.
  - Add project tags to the search result list.
  - Make the "No Tags" tag a little less conspicious.

Test Plan:
  - Indexed dashboards and panels.
  - Searched for dashboards and panels via SearchEngine using Ferret "query" field.
  - Searched for panels via "Add Existing Panel" datasource typeahead.
  - Searched for dashboards via "Add Menu Item > Dashboard" on a ProfileMenu via typeahead.
  - Viewed dashboard NUX state (no special state, but no more bad link to "/create/").
  - Viewed dashboard list, saw project tags.
  - Viewed dashboards with no project tags ("No Tags" is now displayed but less visible).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20411
2019-04-14 10:28:19 -07:00
epriestley
fb994909cf Make "Move Panel" on dashboards use the new storage and transactions
Summary: Depends on D20408. Ref T13272. The actual JS is still a little bit iffy, but this makes the server side "move" operation work correctly by updating it to use the same code as everything else.

Test Plan: Moved panels around on single-column and multi-column dashboards, saw them move to reasonable places and stay there when I reloaded the page.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20409
2019-04-14 10:25:22 -07:00
epriestley
82c46f4b93 Update "add panel" and "remove panel" Dashboard flows to the new panel storage format
Summary:
Depends on D20407. Ref T13272. This updates the "add panel" (which has two flavors: "add existing" and "create new") and "remove panel" flows to work with the new duplicate-friendly storage format.

  - We now modify panels by "panelKey", not by panel PHID, so one dashboard may have multiple copies of the same panel and we can still figure out what's going on.
  - We now work with "contextPHID", not "dashboardID", to make some flows with tab panels (or other nested panels in the future) easier.

The only major remaining flow is the Javascript "move panels around with drag-and-drop" flow.

Test Plan:
  - Added panels to a dashboard with "Create New Panel".
  - Added panels to a dashboard with "Add Existing Panel".
  - Removed panels from a dashboard.
  - Added and removed duplicate panels, got a correctly-functioning dashboard that didn't care about duplicates.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20408
2019-04-14 10:24:58 -07:00
epriestley
51f2ed498d On panel pages, show where panels are used
Summary:
Depends on D20398. Ref T13272. Fixes T6018. Previously, panels showed "used on dashboards: x, y", but this did not include cases where a panel was used by another container panel (today, a tab panel).

Do edge indexing when a dashboard or panel is saved, then pull the edges on the Panel page so we can provide a full list of uses.

Test Plan: {F6369289}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272, T6018

Differential Revision: https://secure.phabricator.com/D20399
2019-04-12 06:14:21 -07:00
epriestley
d62f4dbfc9 Index and surface usage sites for Dashboards
Summary:
Depends on D20397. Ref T13272. Similar to the recent "where are Herald rules used" stuff, show which menus Dashboards are installed in.

This is mostly straightforward, except that I pulled some of the Herald logic into a parent class so it could be shared.

Test Plan: {F6369164}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20398
2019-04-12 06:13:44 -07:00
epriestley
cbe13b3065 When editing a tab panel from a dashboard, redirect back to the dashboard
Summary:
Depends on D20396. Ref T13272. Currently, using the dropdowns to edit a tab panel from a dashboard redirects you to the tab panel page.

Instead, redirect back to the context page (usually, a dashboard -- but theoretically a containing tab panel, since you can put tab panels inside tab panels).

Also, fix some JS issues with non-integer panel keys. I've moved panel keys from "0, 1, 2, ..." to having arbitrary keys to make some operations less flimsy/error-prone, but this needs some JS tweaks.

Test Plan: Edited a tab panel from a dashboard, got sent sensibly back to the dashboard.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20397
2019-04-12 06:13:12 -07:00
epriestley
9ad9ac9be6 On Dashboard tab panels in edit mode, make the "Tab Name" and the "Dropdown Edit Caret" into different links
Summary:
Ref T13272. In edit mode, tab panels now have a dropdown menu. However, this sort of overrlaps with the actual action of clicking the tab to select it.

Separate these into different click targets so that "select tab X" and "open dropdown menu for X" are different operations.

This is more work than it appears because:

  - We have an "action icon" already, used when you put a dashboard on a portal/home to create an "Edit" link. It makes sense to attach dropdowns to this, but it has some hard-coded stuff.
  - In applications with a "Create <thing>" in the crumbs (like Maniphest), we may use a dropdown menu if there are multiple create forms available. However, this menu renders in a weird way by reading all the properties out of an actual "View" object and building something else.
  - The "list of tabs" stuff shares code with different "list of tabs" navigation used by Diffusion and Instances.

..but I think I fixed everything and didn't break anything.

Test Plan:
  - Clicked "select tab" and "open dropdown menu" as separate actions.
  - Viewed Diffusion, Maniphest with multiple create forms, Instances.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20396
2019-04-12 06:08:32 -07:00
epriestley
299b6f420d Fix two minor main menu bar CSS issues
Summary:
Ref T13263.

  - Make the user profile section of the "Profile" dropdown menu have a transparent background, not a white background. This is a pre-existing issue. This is normally hard to see, but visible on Workboards with custom background colors.
  - Fix an alignment issue with the little "V" caret in the search scope dropdown. This is a recent issue caused by some tab-caret CSS I added recently for tabbed dashboard panels.

Test Plan:
Before:

{F6367723}

After:

{F6367724}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13263

Differential Revision: https://secure.phabricator.com/D20388
2019-04-10 11:27:47 -07:00
epriestley
a35fda2019 Rebuild Dashboards on EditEngine: v1 Major Jank Edition
Summary:
Depends on D20383. Ref T13272. Fixes T12363. See PHI997. This gets the edit flows for tab panels functional again. They aren't //nice//, and a lot of the workflows are fairly janky: for example, most of them end up with you on the tab panel's page, which isn't useful if you started on a dashboard page.

However, these flows were extremely janky before anyway (see T12363) and I suspect this is a net improvement even though it's a bit of a mess. I anticipate cleaning this up bit-by-bit in future diffs.

Test Plan: {F6366372}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272, T12363

Differential Revision: https://secure.phabricator.com/D20384
2019-04-10 08:59:32 -07:00
epriestley
e4524d4707 When a dropdown menu would render in a way that hides it offscreen, try a different alignment
Summary:
Depends on D20382. Ref T13272. When something near the edge of the screen has a dropdown menu, we currently may render the menu offscreen.

Instead, keep the menu onscreen.

(This is happening because I'm adding dropdown menus to tab query panels.)

Test Plan:
Before:

{F6363339}

After:

{F6363340}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20383
2019-04-09 14:12:30 -07:00
epriestley
a1a89589b1 Make the Dashboard dropshadow a little lighter and turn panel management into a menu
Summary:
Depends on D20372. Ref T13272.

  - There's a very heavy dropshadow on panels right now that looks out of place. Reduce it a bit.
  - Panels currently have unlabeled pencil and trash icons. Turn this into a menu. I'm likely planning to add options like "Change Query..." to this menu to make managing some types of panels easier.

Test Plan: {F6332838}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20373
2019-04-09 13:58:38 -07:00
epriestley
12b9224387 Make the "Install Dashboard" flow smoother
Summary:
Depends on D20362. Ref T13272. Currently, Dashboards have an "Install Dashboard" flow which is pretty janky and only allows you to install things to the home page.

Instead, allow users to install things to any valid target (home, favorites, portals, projects). This also provides URIs like `dashboard/install/1/home/personal/` which allow you to link users to an "install a dashboard" page; this may or may not get used.

Test Plan: Installed dashboards on home, favorites, projects, and portals.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20364
2019-04-09 13:34:09 -07:00
epriestley
248d79f36d Fix "Actions" button on Phame standalone/live pages (bonus: JX.sprintf())
Summary:
See <https://discourse.phabricator-community.org/t/non-functional-actions-menu-on-live-phame-views/2593>. Several layers here:

The "Actions" button is broken because a menu behavior is failing, since we aren't rendering the menu.

When a behavior fails to initialize, catch and log the exception and continue. Previously, we stopped initializing behaviors if any failed, but behaviors are usually independent and continuing with an explicit exception seems reasonable.

Give "JX.log()" some "sprintf()" semantics to make logging the behavior failure easier. We can probably afford these extra 200 bytes now in 2019.

This fixes the button and gives us explicit errors in the log. So far, so good.

Then, when a page won't render chrome, don't try to render the main menu. This fixes the actual errors (we no longer try to initialize menu behaviors for nodes which don't exist).

Completely hide the "Actions" and "Comment" flows if the viewer isn't logged in. Although this isn't completely consistent with other applications, I think it's more appropriate for Phame. In applications like Maniphest, we show a full set of controls (but disable them) so that users who are not currently logged in have a clear path to interact with the content, under the assumption that this is a relatively common workflow. This is probably less common for Phame, where we expect most anonymous viewers not to log in or interact.

Finally, parametrize a one-off border color and add a border under the crumbs at the top of the page.

Test Plan:
  - Viewed a "Live" Phame blog post page, clicked "Actions", got a dropdown.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20378
2019-04-04 06:10:14 -07:00
epriestley
47bf382435 Allow profile menu items to be locked to the top or bottom of the menu
Summary:
Depends on D20353. Ref T13275. This is just some small quality-of-life fixes:

  - When you add items to menus, they currently go below the "Edit Menu/Manage Menu" links by default. This isn't a very good place for them. Instead, lock "edit" items to the bottom of the menu.
  - Lock profile pictures to the top of the menu. This just simplifies things a little.
  - Show more iconography hints on the "edit menu items" UI.
  - Add a "drag stuff to do things" hint if some stuff can be dragged.

Test Plan:
  - Added new items to a Portal, they didn't go to the very bottom. Instead, they went above the "Edit/Manage" links; a sensible place for them.
  - Viewed the "edit menu items" screen, saw more hints and visual richness.
  - Viewed/edited Home, Projects, Portals, Favorites

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13275

Differential Revision: https://secure.phabricator.com/D20355
2019-04-02 15:08:20 -07:00
epriestley
dba1b10720 Deactivate the remarkup autosuggest once text can't match "[[" or "((" rules
Summary:
See PHI1185, which reports a performance issue with "(" in remarkup in certain contexts.

I can't reproduce the performance issue, but I can reproduce the autosuggester incorrectly remaining active and swallowing return characters.

When the user types `(` or `[`, we wait for a prefix for the `((` (Phurl) or `[[` (Phriction) rules. We currently continue looking for that prefix until a character is entered that explicitly interrupts the search.

For example, typing `(xxx<return>` does not insert a return character, because we're stuck on matching the prefix.

Instead, as soon as the user has entered text that we know won't ever match the prefix, deactivate the autocomplete. We can slightly cheat through this by just looking for at least one character of text, since all prefixes are exactly one character long. If we eventually have some kind of `~~@(xyz)` rule we might need to add a more complicated piece of rejection logic.

Test Plan: Typed `(xxx<return>`, got a return. Used `((` and `[[` autosuggest rules normally. Used `JX.log()` to sanity check that nothing too crazy seems to be happening.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20365
2019-04-01 15:40:38 -07:00
epriestley
cec779cdab When drawing a very wide graph line diagram, smush it together a bit
Summary: Depends on D20345. Use a narrower layout for very large graphs to save some space.

Test Plan:
Before:

{F6320215}

After:

{F6320216}

This does not affect smaller graphs.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20346
2019-03-28 21:18:52 -07:00
epriestley
e586ed439a Improve overflow/ellipsis behaivor for very wide task graphs
Summary:
See downstream <https://phabricator.wikimedia.org/T171648>. The `T123 Task Name` column in graphs can currently fold down to 0 pixels wide.

Although it's visually nice to render this element without a scroll bar when we don't really need one, the current behavior is excessive and not very useful.

Instead, tweak the CSS so:

  - This cell is always at least 320px wide.
  - After 320px, we'll overflow/ellipsis the cell on small screens.

This generally gives us better behavior:

  - Small screens get a scrollbar to see a reasonable amount of content.
  - The UI doesn't turn into a total mess if one task has a whole novel of text.

Test Plan:
Old behavior, note that there's no scrollbar and the cell is so narrow it is useless:

{F6320208}

New behavior, same default view, has a scrollbar:

{F6320209}

Scrolling over gives you this:

{F6320210}

On a wider screen (this wide or better), we don't need to scroll:

{F6320211}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20345
2019-03-28 21:16:48 -07:00
epriestley
c4856c37e7 Fix content overflow in user hovercards
Summary:
Fixes T13273. This element is a bit weird, but I think I fixed it without breaking anything.

The CSS is used by project hovercards and user hovercards, but they each have a class which builds mostly-shared-but-not-really-identical CSS, instead of having a single `View` class with modes. So I'm not 100% sure I didn't break something obscure, but I couldn't find anything this breaks.

The major issue is that all the text content has "position: absolute". Instead, make the image "absolute" and the text actual positioned content. Then fix all the margins/padding/spacing/layout and add overflow. Seems to work?

Plus: hide availability for disabled users, for consistency with D20342.

Test Plan:
Before:

{F6320155}

After:

{F6320156}

I think this is pixel-exact except for the overflow behavior.

Also:

  - Viewed some other user hovercards, including a disabled user. They all looked unchanged.
  - Viewed some project hovercards. They all looked good, too.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13273

Differential Revision: https://secure.phabricator.com/D20344
2019-03-28 21:10:09 -07:00
epriestley
eecee17213 Activate "jx-toggle-class" on click to fix broken mobile behavior
Summary:
See downstream <https://phabricator.wikimedia.org/T201480>. Searching for things on mobile is a significant challenge because clicking the "Magnifying Glass" icon shows and then immediately hides the menu. I believe some aspect of iOS event handling has changed since this was originally written.

At some point, I'd like to rewrite this to work more cleanly and get rid of `jx-toggle-class`. In particular, it isn't smart enough to know that it should be modal with other menus, so you can get states like this by clicking multiple things:

{F6320110}

This would also probably just look and work better if it was an inline element that showed up under the header instead of a floating dropdown element.

However, I'm having a hard time getting the Safari debugger to actually connect to the iOS simulator, so take a small step toward this bright future and fix the immediate problem for now: toggle on click instead of mousedown/touchstart.

This means the menu opens ~100ms later, but actually works. Big improvement!

I'd like to move away from "jx-toggle-class" anyway (it usually isn't sophisticated enough to fully describe a behavior) so reducing complexity here seems good. It isn't used in //too// many places so this is unlikely to have any negative effects, I hope.

Test Plan: On iOS simulator, clicked the magnifying glass icon in the main menu to get a search input. Before: got a search input for a microsecond. After: actually got a search input.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20343
2019-03-28 17:29:06 -07:00
epriestley
3e1ffda85d Give workboard column header actions a more clickable appearance
Summary: Ref T13269. Make it visually more clear that the "Trigger" and "New Task / Edit / Bulk" dropdown menu items are buttons, not status icons or indicators of some kind.

Test Plan: {F6313872}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20332
2019-03-27 14:56:18 -07:00
epriestley
f6658bf391 When changing the trigger type in the trigger editor, properly redraw the control
Summary: Ref T13269. I refactored this late in the game to organize things better and add table cells around stuff, and accidentally broke the relationship between the "Rule Type" selector and the value selector.

Test Plan: Switched rule type selector from "Change Status" to "Play Sound", saw secondary control update properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20326
2019-03-26 11:50:31 -07:00
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