Summary:
Depends on D20274. Ref T10578. This is en route to an ordering by points, it's just a simpler half-step on the way there.
Allow columns to be sorted by creation date, so the newest tasks rise to the top.
In this ordering you can never reposition cards, since editing a creation date by dragging makes no sense. This will be true of the "points" ordering too (although we could imagine doing something like prompting the user, some day).
Test Plan: Viewed boards by "natural" (allows reordering both when dragging within and between columns), "priority" (reorder only within columns), and "creation date" (reorder never). Dragged cards around between and within columns, got apparently sensible behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10578
Differential Revision: https://secure.phabricator.com/D20275
Summary:
Depends on D20273. Fixes T10722. Currently, we don't make it very clear when a card can't be edited. Long ago, some code made a weak attempt to do this (by hiding the "grip" on the card), but later UI changes hid the "grip" unconditionally so that mooted things.
Instead:
- Replace the edit pencil with a red lock.
- Provide cursor hints for grabbable / not grabbable.
- Don't let users pick up cards they can't edit.
Test Plan: On a workboard with a mixture of editable and not-editable cards, hovered over the different cards and was able to figure out which ones I could drag or not drag pretty easily. Picked up cards I could pick up, wasn't able to drag cards I can't edit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10722
Differential Revision: https://secure.phabricator.com/D20274
Summary:
Depends on D20271. Ref T10333. When a column is empty but a board is grouped (by priority, owner, etc) render the headers properly.
When a column has headers, don't apply the "empty" style even if it has no cards. This style just makes some empty space so you can drag-and-drop more easily, but headers do the same thing.
Test Plan: {F6264611}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20272
Summary:
Depends on D20267. Depends on D20268. Ref T10333. Currently, we support "Natural" and "Priority" orders, but a lot of the particulars are pretty hard-coded, including some logic in `ManiphestTask`.
Although it's not clear that we'll ever put other types of objects on workboards, it seems generally bad that you need to modify `ManiphestTask` to get a new ordering.
Pull the ordering logic out into a `ProjectColumnOrder` hierarchy instead, and let each ordering define the things it needs to work (name, icon, what headers look like, how different objects are sorted, and how to apply an edit when you drop an object under a header).
Then move the existing "Natural" and "Priority" orders into this new hierarchy.
This has a minor bug where using the "Edit" workflow to change a card's priority on a priority-ordered board doesn't fully refresh card/header order since the response isn't ordering-aware. I'll fix that in an upcoming change.
Test Plan: Grouped workboards by "Natural" and "Priority", dragged stuff around within and between columns, grepped for all touched symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20269
Summary:
Depends on D20266. Boards currently have several `whateverMap<cardPHID => stuff>` properties, but we can just move these all down into a `CardTemplate`, similar to the recently introduced `HeaderTemplate`.
The `CardTemplate` holds all the global information for a card, and then `Card` is specific for a particular copy in a column. Today, each `CardTemplate` has one `Card`, but a `CardTemplate` may have more than one card in the future (when we add subproject columns).
Test Plan: Viewed workboards in different sort orders and dragged stuff around, grepped for all affected symbols.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20267
Summary:
Depends on D20263. Ref T10333. I want to add groups like "Assignee" to workboards. This means you may have several tasks grouped under, say, "Alice".
When you drag the bottom-most task under "Alice" to the top, what does that mean?
Today, the only grouping is "Priority", and it means "change the task's secret/hidden global subpriority". However, this seems to generally be a somewhat-bad answer, and is quite complex. It also doesn't make much sense for an author grouping, since one task can't really be "more assigned" to Alice than another task.
Users likely intend this operation to mean "move it, visually, with no other effects" -- that is, user intent is to shuffle sticky notes around on a board, not edit anything substantive. The meaning is probably something like "this is similar to other nearby tasks" or "maybe this is a good place to start", which we can't really capture with any top-level attribute.
We could extend "subpriority" and give tasks a secret/hidden "sub-assignment strength" and so on, but this seems like a bad road to walk down. We'll also run into trouble later when subproject columns may appear on the board, and a user could want to put a task in different positions on different subprojects, conceivably.
In the "Natural" order view, we already have what is probably a generally better approach for this: a task display order particular to the column, that just remembers where you put the sticky notes.
Move away from "subpriority", and toward a world where we mostly keep sticky notes where you stuck them and move them around only when we have to. With no grouping, we still sort by "natural" order, as before. With priority grouping, we now sort by `<priority, natural>`. When you drag stuff around inside a priority group, we update the natural order.
This means that moving cards around on a "priority" board will also move them around on a "natural" board, at least somewhat. I think this is okay. If it's not intuitive, we could give every ordering its own separate "natural" view, so we remember where you stuck stuff on the "priority" board but that doesn't affect the "Natural" board. But I suspect we won't need to.
Test Plan:
- Viewed and dragged a natural board.
- Viewed and dragged a priority board.
- Dragged within and between groups of 0, 1, and multiple items.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20265
Summary:
Ref T13074. Today, in normal task list views in Maniphest (not workboards), you can (sometimes) reorder tasks if the view is priority-sorted.
I suspect no one ever does this, few users know it's supported, and that it was basically rendered obsolete the day we shipped workboards.
This also means that we need to maintain a global "subpriority" for tasks, which distinguishes between different tasks at the same priority level (e.g., "High") and maintains a consistent ordering on workboards.
As we move toward making workboards more flexible (e.g., group by author / owner / custom fields), I'd like to try moving away from "subpriority" and possibly removing it entirely, in favor of "natural order", which basically means "we kind of remember where you put the card and it works a bit like a sticky note".
Currently, the "natural order" and "subpriority" systems are sort of similar but also sort of in conflict, and the "subpriority" system can't really be extended while the "natural order / column position" system can.
The only real reason to have a global "subpriority" is to support the list-view drag-and-drop.
It's possible I'm wrong about this and a bunch of users love this feature, but we can re-evaluate if we get feedback in this vein.
(This just removes UI, the actual subpriority system is still intact and still used on workboards.)
Test Plan: Viewed task lists, was no longer able to drag stuff. Grepped for affected symbols. Dragged stuff in remaining grippable lists, like "Edit Forms" in EditEngine config.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20263
Summary:
These effects feel like they're possibly overkill, since other CSS rules make the selection reticle behave correctly and the implementation is relatively intuitive.
Or not, either way.
Test Plan: Selected text on either side of a 2-up diff, no more opacity effects.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20264
Summary:
Ref T10333. Ref T8135. Depends on D20247. Allow users to drag-and-drop cards on a priority-sorted workboard under headers, even if the header has no other cards.
As of D20247, headers show up but they aren't really interactive. Now, you can drag cards directly underneath a header (instead of only between other cards). For example, if a column has only one "Wishlist" task, you may drag it under the "High", "Normal", or "Low" priority headers to select a specific priority.
(Some of this code still feels a little rough, but I think it will generalize once other types of sorting are available.)
Test Plan: Dragged cards within and between priority groups, saw appropriate priority edits applied in every case I could come up with.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333, T8135
Differential Revision: https://secure.phabricator.com/D20248
Summary:
Ref T10333. When workboards are ordered (for example, by priority), add headers to the various groups. Major goals are:
- Allow users to drag-and-drop to set values that no cards currently have: for example, you can change a card priority to "normal" by dragging it under the "normal" header, even if no other cards in the column are currently "Normal".
- Make future orderings more useful, particularly "order by assignee". We don't really have room to put the username on every card and it would create a fair amount of clutter, but we can put usernames in these headers and then reference them with just the profile picture. This also allows you to assign to users who are not currently assigned anything in a given column.
- Make the drag-and-drop behavior more obvious by showing what it will do more clearly (see T8135).
- Make things a little easier to scan in general: because space on cards is limited, some information isn't conveyed very clearly (for example, priority information is currently conveyed //only// through color, which can be hard to pick out visually and is probably not functional for users who need vision accommodations).
- Maybe do "swimlanes": this is pretty much a "swimlanes" UI if we add whitespace at the bottom of each group so that the headers line up across all the columns (e.g., "Normal" is at the same y-axis position in every column as you scroll down the page). Not sold on this being useful, but it's just a UI adjustment if we do want to try it.
NOTE: This only makes these headers work for display.
They aren't yet recognized as targets by the drag list UI, so you can't drag cards into an empty group. I'll tackle that in a followup.
Test Plan: {F6257686}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20247
Summary:
Ref T10334. Partly, this just improves visual feedback for all drag operations. After D20242, we can have cases where you (for example) drag a low-priority node to a very tall column on a priority-ordered workboard. In this case, the actual dashed-border-drop-target may not be on screen.
We might make the column scroll or put some kind of hint in the UI in this case, but an easy starting point is just to make the "yes, you're targeting this column" state a bit more clear.
Test Plan: Dragged tasks between columns, saw the border higlight on the target columns. This is very tricky to take a screenshot of.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10334
Differential Revision: https://secure.phabricator.com/D20245
Summary:
Ref T10334. When a workboard is ordered by priority, dragging from column "A" to a particular place in column "B" currently means "move this task to column B, and adjust its priority so that it naturally sorts into the location under my mouse cursor".
Users frequently find this confusing / undesirable.
To begin improving this, make "drag from column A to column B" and "drag from somewhere in column A to somewhere else in column A" into different operations. The first operation, a movement between columns, no longer implies an ordering change. The second action still does.
So if you actually want to change the priority of a task, you drag it within its current column. If you just want to move it to a different column, you drag it between columns.
This creates some possible problems:
- Some users may love the current behavior and just not be very vocal about it. I doubt it, but presumably we'll hear from them if we break it.
- If you actualy want to move + reorder, it's a bit more cumbersome now. We could possibly add something like "shift + drag" for this if there's feedback.
- The new behavior is probably less surprising, but may not be much more obvious. Future changes (for example, in T10335) should help make it more clear.
- When you mouse cursor goes over column B, the card dashed-rectangle preview target thing jumps to the correct position in the column -- but that may not be under your mouse cursor. This feels pretty much fine if the whole column fits on screen. It may not be so great if the column does not fit on screen and the dashed-rectangle-thing has vanished. This is just a UI feedback issue and we could refine this later (scroll/highlight the column).
Test Plan:
- Created several tasks at different priority levels, sorted a board by priority, dragged tasks between columns. Dragging from "A" to "B" no longer causes a priority edit.
- Also, dragged within a column. This still performs priority edits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10334
Differential Revision: https://secure.phabricator.com/D20242
Summary:
Ref T13249.
- When a line has only increased in indent depth, don't red-fill highlight the left side of the diff. Since reading a diff //mostly// involves focusing on the right side, indent depth changes are generally visible enough without this extra hint. The extra hint can become distracting in cases where there is a large block of indent depth changes.
- Move the markers slightly to the left, to align them with the gutter.
- Make them slightly opaque so they're a little less prominent.
Test Plan: See screenshots.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20251
Summary:
Ref T13259. An install provided feedback that it wasn't obvious you could click the buttons in this UI.
Make it more clear that these are clickable buttons.
Test Plan:
{F6251585}
{F6251586}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20238
Summary:
Depends on D20196. See PHI985. When empty, the "moved/copied" gutter currently renders with the same background color as the rest of the line. This can be misleading because it makes code look more indented than it is, especially if you're unfamiliar with the tool:
{F6225179}
If we remove this misleading coloration, we get a white gap. This is more clear, but looks a little odd:
{F6225181}
Instead, give this gutter a subtle background fill in all casses, to make it more clear that it's a separate gutter region, not a part of the text diff:
{F6225183}
Test Plan: See screenshots. Copied text from a diff, added/removed inlines, etc.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20197
Summary:
Ref T12822. Ref PHI878. This is some leftover code from the old selection behavior that prevented visual selection of the left side of a diff if the user clicked on the right -- basically, a much simpler attack on what ultimately landed in D20191.
I think the change from `th` to `td` "broke" it so it didn't interfere with the other behavior, which is why I didn't have to remove it earlier. It's no longer necessary, in any case.
Test Plan: Grepped for behavior name, selected stuff on both sides of a diff.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20196
Summary:
Ref T13161.
- Don't show ">>" when the line indentation changed but the text also changed, this is just "the line changed".
- The indicator seems a little cleaner if we just reuse the existing "bright" colors, which already have colorblind colors anyway.
Test Plan: Got slightly better rendering for some diffs locally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20195
Summary:
Ref T12822. Ref T2495. This is the good version of D20193.
Currently, we display various nonprintable characters (ZWS, nonbreaking space, various control characters) as themselves, so they're generally invisible.
In T12822, one user reports that all their engineers frequently type ZWS characters into source somehow? I don't really believe this (??), and this should be fixed in lint.
That said, the only real reason not to show these weird characters in a special way was that it would break copy/paste: if we render ZWS as "🐑", and a user copy-pastes the line including the ZWS, they'll get a sheep.
At least, they would have, until D20191. Now that this whole thing is end-to-end Javascript magic, we can copy whatever we want.
In particular, we can render any character `X` as `<span data-copy-text="Y">X</span>`, and then copy "Y" instead of "X" when the user copies the node. Limitations:
- If users select only "X", they'll get "X" on their clipboard. This seems fine. If you're selecting our ZWS marker *only*, you probably want to copy it?
- If "X" is more than one character long, users will get the full "Y" if they select any part of "X". At least here, this only matters when "X" is several spaces and "Y" is a tab. This also seems fine.
- We have to be kind of careful because this approach involves editing an HTML blob directly. However, we already do that elsewhere and this isn't really too hard to get right.
With those tools in hand:
- Replace "\t" (raw text / what gets copied) with the number of spaces to the next tab stop for display.
- Replace ZWS and NBSP (raw text) with a special marker for display.
- Replace control characters 0x00-0x19 and 0x7F, except for "\t", "\r", and "\n", with the special unicode "control character pictures" reserved for this purpose.
Test Plan:
- Generated and viewed a file like this one:
{F6220422}
- Copied text out of it, got authentic raw original source text instead of displayed text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822, T2495
Differential Revision: https://secure.phabricator.com/D20194
Summary:
Ref T12822. Fixes a few things:
- Firefox selection of weird ranges with an inline between the start and end of the range now works correctly.
- "Show More Context" rows now render, highlight, and select properly.
- Prepares for nodes to have copy-text which is different from display-text.
- Don't do anything too fancy in 1-up/unified mode. We don't copy line numbers after the `content: attr(data-n)` change, but that's as far as we go, because trying to do more than that is kind of weird and not terribly intuitive.
Test Plan:
- Selected and copied weird ranges in Firefox.
- Kept an eye on "Show More Context" rows across select and copy operations.
- Generally poked around in Safari/Firefox/Chrome.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20192
Summary:
Ref T12822. Ref T13161. By default, when users select text from a diff and copy it to the clipboard, they get both sides of the diff and all the line numbers. This is usually not what they intended to copy.
As of D20188, we use `content: attr(...)` to render line numbers. No browser copies this text, so that fixes line numbers.
We can use "user-select" CSS to visually prevent selection of line numbers and other stuff we don't want to copy. In Firefox and Chrome, "user-select" also applies to copied text, so getting "user-select" on the right nodes is largely good enough to do what we want.
In Safari, "user-select" is only visual, so we always need to crawl the DOM to figure out what text to pull out of it anyway.
In all browsers, we likely want to crawl the DOM anyway because this will let us show one piece of text and copy a different piece of text. We probably want to do this in the future to preserve "\t" tabs, and possibly to let us render certain character codes in one way but copy their original values. For example, we could render "\x07" as "␇".
Finally, we have to figure out which side of the diff we're copying from. The rule here is:
- If you start the selection by clicking somewhere on the left or right side of the diff, that's what you're copying.
- Otherwise, use normal document copy rules.
So the overall flow here is:
- Listen for clicks.
- When the user clicks the left or right side of the diff, store what they clicked.
- When a selection starts, and something is actually selected, check if it was initiated by clicking a diff. If it was, apply a visual effect to get "user-select" where it needs to go and show the user what we think they're doing and what we're going to copy.
- (Then, try to handle a bunch of degenerate cases where you start a selection and then click inside that selection.)
- When a user clicks elsewhere or ends the selection with nothing selected, clear the selection mode.
- When a user copies text, if we have an active selection mode, pull all the selected nodes out of the DOM and filter out the ones we don't want to copy, then stitch the text back together. Although I believe this didn't work well in ~2010, it appears to work well today.
Test Plan: This mostly seems to work in Safari, Chrome, and Firefox. T12822 has some errata. I haven't tested touch events but am satisfied if the touch event story is anything better than "permanently destroys data".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20191
Summary:
Ref T13161. Ref T12822. Today, we use invisible Zero-Width Spaces to try to improve copy/paste behavior from Differential.
After D20188, we no longer need ZWS characters to avoid copying line numbers. Get rid of these secret invisible semantic ZWS characters completely.
This means that both the left-hand and right-hand side of diffs become copyable, which isn't desired. I'll fix that with a hundred thousand lines of Javascript in the next change: this is a step toward everything working better, but doesn't fix everything yet.
Test Plan:
- Grepped for `zws`, `grep -i zero | grep -i width`.
- Copied text out of Differential: got both sides of the diff (not ideal).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20189
Summary:
Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `<th>3</th>` was a perfectly fine way to render a line number cell containing the number "3".
But times have changed!
- In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to `<td>`.
- In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own.
The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately.
Test Plan:
- Created, edited, deleted inlines in 1-up and 2-up views.
- Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20188
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary:
Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like `diff -bw`, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use `diff -bw`.
However, this approach is very coarse and has a lot of problems, like detecting `"ab" -> "a b"` as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach.
We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to `diff -bw` it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration.
I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc.
Move toward detecting and rendering indent depth changes. Followup work:
- These should get colorblind colors and the design can probably use a little more tweaking.
- The OneUp mode is okay, but could be improved.
- Whitespace mode can now be removed completely.
- I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right.
Test Plan: {F6214084}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20181
Summary:
Ref T13249. See PHI810. We currently show availability dots in some interfaces (timeline, mentions) but not others (typeheads/tokenizers).
They're potentially quite useful in tokenizers, e.g. when assigning tasks to someone or requesting reviews. Show them in more places.
(The actual rendering here isn't terribly clean, and it would be great to try to unify all these various behaviors some day.)
Test Plan:
{F6212044}
{F6212045}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20173
Summary:
Ref T13249. See PHI1059. This allows "locked" in `maniphest.statuses` to specify that either "comments" are locked (current behavior, advisory, overridable by users with edit permission, e.g. for calming discussion on a contentious issue or putting a guard rail on things); or "edits" are locked (hard lock, only task owner can edit things).
Roughly, "comments" is a soft/advisory lock. "edits" is a hard/strict lock. (I think both types of locks have reasonable use cases, which is why I'm not just making locks stronger across the board.)
When "edits" are locked:
- The edit policy looks like "no one" to normal callers.
- In one special case, we sneak the real value through a back channel using PolicyCodex in the specific narrow case that you're editing the object. Otherwise, the policy selector control incorrectly switches to "No One".
- We also have to do a little more validation around applying a mixture of status + owner transactions that could leave the task uneditable.
For now, I'm allowing you to reassign a hard-locked task to someone else. If you get this wrong, we can end up in a state where no one can edit the task. If this is an issue, we could respond in various ways: prevent these edits; prevent assigning to disabled users; provide a `bin/task reassign`; uh maybe have a quorum convene?
Test Plan:
- Defined "Soft Locked" and "Hard Locked" statues.
- "Hard Locked" a task, hit errors (trying to unassign myself, trying to hard lock an unassigned task).
- Saw nice new policy guidance icon in header.
{F6210362}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20165
Summary:
See PHI1073. Improve the UX here:
- When there are a small number of connected tasks, no changes.
- When there are too many total connected tasks, but not too many directly connected tasks, show hint text with a "View Standalone Graph" button to view more of the graph.
- When there are too many directly connected tasks, show better hint text with a "View Standalone Graph" button.
- Always show a "View Standalone Graph" option in the dropdown menu.
- Add a standalone view which works the same way but has a limit of 2,000.
- This view doesn't have "View Standalone Graph" links, since they'd just link back to the same page, but is basically the same otherwise.
- Increase the main page task limit from 100 to 200.
Test Plan:
Mobile View:
{F6210326}
Way too much stuff:
{F6210327}
New persistent link to the standalone page:
{F6210328}
Kind of too much stuff:
{F6210329}
Standalone view:
{F6210330}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: 20after4
Differential Revision: https://secure.phabricator.com/D20164
Summary:
Ref T13249. See PHI774. When users follow an email login link ("Forgot password?", "Send Welcome Email", "Send a login link to your email address.", `bin/auth recover`), we send them to a password reset flow if an install uses passwords.
If an install does not use passwords, we previously dumped them unceremoniously into the {nav Settings > External Accounts} UI with no real guidance about what they were supposed to do. Since D20094 we do a slightly better job here in some cases. Continue improving this workflow.
This adds a page like "Reset Password" for "Hey, You Should Probably Link An Account, Here's Some Options".
Overall, this stuff is still pretty rough in a couple of areas that I imagine addressing in the future:
- When you finish linking, we still dump you back in Settings. At least we got you to link things. But better would be to return you here and say "great job, you're a pro".
- This UI can become a weird pile of buttons in certain configs and generally looks a little unintentional. This problem is shared among all the "linkable" providers, and the non-login link flow is also weird.
So: step forward, but more work to be done.
Test Plan: {F6211115}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20170
Summary: Ref T13249. Poll for Duo updates in the background so we can automatically update the UI when the user clicks the mobile phone app button.
Test Plan: Hit a Duo gate, clicked "Approve" in the mobile app, saw the UI update immediately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20169
Summary:
Depends on D20140. Ref T13250. Currently, the top-level exception handler doesn't dump stacks because we might not be in debug mode, and we might double-extra-super fatal if we call `PhabricatorEnv:...` to try to figure out if we're in debug mode or not.
We can get around this by setting a flag on the Sink once we're able to confirm that we're in debug mode. Then it's okay for the top-level error handler to show traces.
There's still some small possibility that showing a trace could make us double-super-fatal since we have to call a little more code, but AphrontStackTraceView is pretty conservative about what it does and 99% of the time this is a huge improvement.
Test Plan: {F6205122}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20142
Summary:
Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.
Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.
Also, make the "MFA" and "Silent" emblems more easily visible.
Test Plan:
Edited a locked task, overrode the lock, got marked for it.
{F6195005}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aeiser
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20131
Summary: Ref T13244. See PHI1052. Our error handling for Stripe errors isn't great right now. We can give users a bit more information, and a less jarring UI.
Test Plan:
Before (this is in developer mode, production doesn't get a stack trace):
{F6197394}
After:
{F6197397}
- Tried all the invalid test codes listed here: https://stripe.com/docs/testing#cards
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20132
Summary:
Depends on D20122. Fixes T8029. Adds an "Approve User" action to the "Manage" page.
Users are normally approved from the "Approval Queue", but if you click into a user's profile to check them out in more detail it kind of dead ends you right now. I've occasionally hit this myself, and think this workflow is generally reasonable enough to support upstream.
Test Plan: {F6193742}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8029
Differential Revision: https://secure.phabricator.com/D20123
Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.
Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.
Test Plan: Implemented Duo, elsewhere.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D20028
Summary:
Depends on D20016. Ref T920. This does nothing interesting on its own since the TOTP provider has no guidance/warnings, but landing it separately helps to simplify an upcoming SMS diff.
SMS will have these guidance messages:
- "Administrator: you haven't configured any mailer which can send SMS, like Twilio."
- "Administrator: SMS is weak."
- "User: you haven't configured a contact number."
Test Plan: {F6151283} {F6151284}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20017
Summary: Depends on D19992. Ref T13222. If administrators provide a custom login message, show it on the login screen.
Test Plan:
{F6137930}
- Viewed login screen with and without a custom message.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19994
Summary:
See PHI1023. Ref T7607. Occasionally, companies need their billing address (or some other custom text) to appear on invoices to satisfy process or compliance requirements.
Allow accounts to have a custom "Billing Name" and a custom "Billing Address" which appear on invoices.
Test Plan: {F6134707}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T7607
Differential Revision: https://secure.phabricator.com/D19979
Summary: See PHI1017. This is a trivial fix even though these burnups are headed toward a grisly fate.
Test Plan: Moused over some January datapoints, saw "1" instead of "0".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19967
Summary:
Ref T12509. This upgrades a `weakDigest()` callsite to SHA256-HMAC and removes three config options:
- `celerity.resource-hash`: Now hard-coded, since the use case for ever adjusting it was very weak.
- `celerity.enable-deflate`: Intended to make cache inspection easier, but we haven't needed to inspect caches in ~forever.
- `celerity.minify`: Intended to make debugging minification easier, but we haven't needed to debug this in ~forever.
In the latter two cases, the options were purely developer-focused, and it's easy to go add an `&& false` somewhere in the code if we need to disable these features to debug something, but the relevant parts of the code basically work properly and never need debugging. These options were excessively paranoid, based on the static resource enviroment at Facebook being far more perilous.
The first case theoretically had end-user utility for fixing stuck content caches. In modern Phabricator, it's not intuitive that you'd go adjust a Config option to fix this. I don't recall any users ever actually running into problems here, though.
(An earlier version of this change did more magic with `celerity.resource-hash`, but this ended up with a more substantial simplification.)
Test Plan: Grepped for removed configuration options.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19941
Summary:
Ref T13222. In D19918, I refactored how timelines get "view data". Today, this is always additional data about which images/changesets/diffs are visible on the current revision/commit/mock, so we can tell if inline comments should be linked to a `#anchor` on the same page (if the inline is rendered there somewhere) or to a `/D123?id=1&vs=2` full link on a different page (if it isn't), but in general this could be any sort of state information about the current page that affects how the timeline should render.
Previously, comment previews did not use any specialized object code and always rendered a "generic" timeline story. This was actually a bug, but none of the code we have today cares about this (since it's all inline related, and inlines render separately) so it never impacted anything.
After the `TimelineEngine` change, the preview renders with Differential-specific code. This is more correct, but we were not passing the preview the "view data" so it broke.
This preview doesn't actually need the view data and we could just make it bail out if it isn't present, but pass it through for consistency and so this works like we'd expect if we do something fancier with view data in the future.
Test Plan: Viewed comment and inline comment previews in Differential, saw old behavior restored.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19943
Summary:
Depends on D19906. Ref T13222. This isn't going to win any design awards, but make the "wait" and "answered" elements a little more clear.
Ideally, the icon parts could be animated Google Authenticator-style timers (but I think we'd need to draw them in a `<canvas />` unless there's some clever trick that I don't know) or maybe we could just have the background be like a "water level" that empties out. Not sure I'm going to actually write the JS for either of those, but the UI at least looks a little more intentional.
Test Plan:
{F6070914}
{F6070915}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19908
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Ref T13222. See PHI996. Ref T10743. For context, perhaps see T12171.
Node changed some signatures, behaviors, and error handling here in recent versions. As far as I can tell:
- The `script.runInNewContext(...)` method has never taken a `path` parameter, and passing the path has always been wrong.
- The `script.runInNewContext(...)` method started taking an `[options]` parameter at some point, and validating it, so the bad `path` parameter now throws.
- `vm.createScript(...)` is "soft deprecated" but basically fine, and keeping it looks more compatible.
This seems like the smallest and most compatible correct change.
Test Plan: Under Node 10, started Aphlict. Before: fatal error on bad `options` parameter to `runInNewContext()` (expected dictionary). After: notification server starts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T10743
Differential Revision: https://secure.phabricator.com/D19860
Summary:
Ref T13222. Fixes T12588. See PHI683. In several cases, we present the user with a choice between multiple major options: Alamnac service types, Drydock blueprint types, Repository VCS types, Herald rule types, etc.
Today, we generally do this with radio buttons and a "Submit" button. This isn't terrible, but often it means users have to click twice (once on the radio; once on submit) when a single click would be sufficient. The radio click target can also be small.
In other cases, we have a container with a link and we'd like to link the entire container: notifications, the `/drydock/` console, etc. We'd like to just link the entire container, but this causes some problems:
- It's not legal to link block eleements like `<a><div> ... </div></a>` and some browsers actually get upset about it.
- We can `<a><span> ... </span></a>` instead, then turn the `<span>` into a block element with CSS -- and this sometimes works, but also has some drawbacks:
- It's not great to do that for screenreaders, since the readable text in the link isn't necessarily very meaningful.
- We can't have any other links inside the element (e.g., details or documentation).
- We can `<form><button> ... </button></form>` instead, but this has its own set of problems:
- You can't right-click to interact with a button in the same way you can with a link.
- Also not great for screenreaders.
Instead, try adding a `linked-container` behavior which just means "when users click this element, pretend they clicked the first link inside it".
This gives us natural HTML (real, legal HTML with actual `<a>` tags) and good screenreader behavior, but allows the effective link target to be visually larger than just the link.
If no issues crop up with this, I'd plan to eventually use this technique in more places (Repositories, Herald, Almanac, Drydock, Notifications menu, etc).
Test Plan:
{F6053035}
- Left-clicked and command-left-clicked the new JS fanciness, got sensible behaviors.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222, T12588
Differential Revision: https://secure.phabricator.com/D19855
Summary:
Ref T13216. See PHI985. If you disable cookies in Firefox, accessing `window.localStorage` throws an exception. Currently, this pretty much kills all scripts on the page.
Instead, catch and ignore this, as though `window.localStorage` was not defined.
Test Plan:
- Set Firefox to "no cookies".
- Loaded any page while logged out.
- Before: JS fatal early in the stack.
- After: page loads and JS works.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19832
Summary:
Ref T13216. See PHI985. When you click a line number to start an inline comment, we intend to initiate the action only if you used the left mouse button (desktop) or a touch (tablet/device).
We currently have a `not right` condition for doing this, but it only excludes right clicks, not middle clicks (or other nth-button clicks). The `not right` condition was sligthly easier to write, but use an `is left` condition instead of a `not right` condition.
Test Plan:
- In Safari, Firefox and Chrome:
- Used left click to start an inline.
- Used middle click to do nothing (previously: started an inline).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19836
Summary:
See PHI975. Ref T13216. Ref T2543. Previously, see D19204 and PHI433.
When you're acting on a draft revision, we change the button text to "Submit Quietly" as a hint that your actions don't generate notifications yet.
However, this isn't accurate when one of your actions is "Request Review", which causes the revision to publish.
Allow actions to override the submit button text, and make the "Request Review" action change the button text to "Publish Revision".
The alternative change I considered was to remove the word "Quietly" in all cases.
I'm not //thrilled// about how complex this change is to adjust one word, but the various pieces are all fairly clean individually. I'm not sure we'll ever be able to use it for anything else, but I do suspect that the word "Quietly" was the change in D19204 with the largest effect by far (see T10000).
Test Plan:
- Created a draft revision. Saw "Submit Quietly" text.
- Added a "Request Review" action, saw it change to "Publish Revision".
- Reloaded page, saw stack saved and "Publish Revision".
- Removed action, saw "Submit Quietly".
- Repeated on a non-draft revision, button stayed put as "Submit".
- Submitted the various actions, saw them have the desired effects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216, T2543
Differential Revision: https://secure.phabricator.com/D19810
Summary: See PHI977. Ref T13216. Some text, like long package names, may overflow hovercards. Add overflow CSS behaviors to remedy this.
Test Plan:
Before:
{F6012699}
After:
{F6012700}
(You can use `/search/hovercard/` to render hovercards in a handy standalone way.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19809
Summary:
Fixes T8440. See that task for discussion.
Ref T13216. See PHI976.
Test Plan:
In Chrome, hovered a timestamp and moved the mouse up to the "overlap" area (see T8440). Before: flickered like crazy. After: no flickering.
(I couldn't reproduce the original issue in modern Firefox or Safari.)
Reviewers: amckinley, avivey
Reviewed By: avivey
Maniphest Tasks: T8440, T13216
Differential Revision: https://secure.phabricator.com/D19808
Summary:
See <https://hackerone.com/reports/434116>. Slowvote has a piece of Javascript that attempts to let you vote on `{V123}` polls inline.
It does not work: nothing ever triggers it (nothing renders a control with a `slowvote-option` sigil).
At least for now, just remove it. It has a completely separate pathway in the controller and both pathways are buggy, so this makes fixing them easier.
Test Plan: Voted in plurality and approval polls via Slowvote and the embedded widget.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19773
Summary:
Depends on D19661. Ref T13077. See PHI840.
When a user edits a page normally, add a "Save as Draft" button. Much of this change is around making that button render and behave properly: it needs to be an `<input type="submit" ...>` so browsers submit it and we can figure out which button the user clicked.
Then there are a few minor rules:
- If you're editing a page which is already a draft, we only give you "Save as Draft". This makes edits to update/revise a draft more natural.
- Highlight "Publish" if it's a likely action that you might want to take.
Internally, there are two types of edits. Both types create a new version with the new content. However:
- A "content" edit sets the version shown on the live page to the newly-created version.
- A "draft" edit does not update the version shown on the live page.
Test Plan: Edited a published document, edited the draft. Published documents. Reverted documents.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19662
Summary:
Depends on D19659. Fixes T1894. Ref T13077. See PHI840.
- Add an EditEngine, although it currently supports no fields.
- Add (basic, top-level-only) commenting (we already had the table in the database).
This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.
This also moves us incrementally toward putting all editing on top of EditEngine.
Test Plan: {F5877347}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T1894
Differential Revision: https://secure.phabricator.com/D19660
Summary: See PHI871. Ref T13197. These sections are only divided visually and don't have textual headers. Add aural headers.
Test Plan: {F5875471}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19654
Summary:
Ref T13195. If a Phriction page begins with a code block, the `clear: both;` currently makes it clear the action list.
Instead, use table-cell layout on desktops.
Test Plan: Viewed a Phriction page with an initial code block on desktop/tablet/mobile/printable layouts. Now got more sensible layouts in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: GoogleLegacy
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19649
Summary: Depends on D19621. Ref T13077. Fixes T4815. This adds previous/current/next/draft buttons and makes navigation between unpublished and published versions of a document more clear.
Test Plan: {F5841997}
Reviewers: amckinley
Maniphest Tasks: T13077, T4815
Differential Revision: https://secure.phabricator.com/D19622
Summary:
Depends on D19616. Ref T13077. Fixes T8172. In the last round of design updates, a lot of actions got stuffed into "Actions" menus.
I never really got used to these and think they're a net usability loss, and broadly agree with the feedback in T8172. I'd generally like to move back toward a state where actions are available on the page, not hidden in a menu.
For now, just put a curtain view on these pages. This could be refined later (e.g., stick this menu to the right hand side of the screen) depending on where other Phriction changes go.
(Broadly, I'm also not satisfied with where we ended up on the fixed-width pages like Diffusion > Manage, Config, and Instances. In contrast, I //do// like where we ended up with Phortune in terms of overall design. I anticipate revisiting some of this stuff eventually.)
Test Plan:
- Looked at Phriction pages on desktop/tablet/mobile/printable -- actions are now available on the page.
- Looked at other DocumentView pages (like Phame blogs) -- no changes for now.
Reviewers: amckinley
Maniphest Tasks: T13077, T8172
Differential Revision: https://secure.phabricator.com/D19617
Summary: Ref T13189. See PHI710. Ref T13088. Fixes T9951. Allow callers to `harbormaster.sendmessage` to specify that the test details are remarkup so they can use rich formatting and include links, files, etc.
Test Plan: {F5840098}
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T13088, T9951
Differential Revision: https://secure.phabricator.com/D19615
Summary:
Ref T13187. See PHI836. The "action" comment actions in Differential (Accept, Reject, etc) render a single line of descriptive text. This is currently slightly misaligned.
Give it similar sizing information to the label element to the left, so it lines up properly.
Test Plan:
Note that "Request Review" and "This revision will be..." are now aligned:
{F5828077}
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19600
Summary:
Depends on D19594. See PHI823. Ref T13164.
- Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
- Add a label for the floating header display options menu in Differential.
- Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.
Test Plan:
Viewed a revision with `?__aural__=true`:
- Saw "Remove Action: ..." label.
- Saw "Display Options" label.
- Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19595
Summary:
Ref T13164. See PHI748. Path validation has a 750ms timeout which blames to rP5038ab850c, in 2011.
Production path validation is sometimes taking more than 750ms, particularly on the initial page load where we may validate many paths simultaneously.
I have no idea why we have this timeout, and it isn't consistent with how we perform other AJAX requests. Just remove it.
Test Plan:
- Reproduced issue in production, saw all validation calls failing at 750ms. Actual underlying calls succeed, they just take more than 750ms to resolve.
- Loaded path validator locally, got green checkmark.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19575
Summary:
Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files.
We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI.
Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled.
Test Plan:
- Viewed a revision in normal and standalone views.
- No changes in normal view, and all keys still work ("N", "P", etc).
- In standalone view, "?" no longer shows nonfunctional key commands.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19571
Summary:
Depends on D19551. Ref T13164. Projects use a special kind of header setup that has a more specific CSS rule to make content black. Add an even more specific rule to make it red.
(This could probably be disentangled a bit and isn't necessarily the cleanest fix, but I poked at it for a few minutes and didn't come up with anything cleaner.)
Test Plan: Viewed projects in spaces, saw the space names colored red properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19552
Summary:
Ref T13151. See PHI685. When you haunt the panel, we only let it take up part of the screen. Let it take up slightly more of the screen so that it's more likely to fit completely on-screen without needing to scroll.
The behavior when it does scroll is fine (you get a scrollbar if your OS/browser is set up to show them) so this is a bit trivial/silly, but seems fine and doesn't have a big JS maintenance cost or anything.
Test Plan: Pressed "Z", resized my window to a weird tiny useless size, got slightly better (I guess) behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19480
Summary: Ref T13151. See PHI616. There's a bug where the current banner changeset isn't cleared correctly when we hide the banner.
Test Plan:
- View revision with several changesets.
- Scroll down slowly through first changeset until banner appears.
- Scroll up until banner disappears.
- Scroll back down.
- Before: banner fails to reappear (code still thinks it's visible and we don't want to update it).
- After: banner reappears correctly.
Reviewers: amckinley, jmeador
Reviewed By: jmeador
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19474
Summary:
Fixes T13148. Ref T13105. The new document rendering engine for images let them overflow the UI bounds.
Add `max-width: 100%;` to keep them contained.
Test Plan:
- Viewed a very wide image in Safari, Firefox and Chrome. Saw sensible rendering.
- Also viewed a normal image, saw normal behavior.
Reviewers: amckinley, avivey
Reviewed By: avivey
Maniphest Tasks: T13148, T13105
Differential Revision: https://secure.phabricator.com/D19457
Summary:
Fixes T13147. In D19437, I changed this logic to support deleting the `""` (empty string) token, but `[].pop()` returns `undefined`, not `null`, if the list is empty and I didn't think to try deleting an empty input.
Fix the logic so we don't end up in a loop if the input is empty.
Test Plan:
- In any browser, deleted all tokens in a tokenizer; then pressed delete again.
- Before: tab hangs in an infinte loop.
- After: smooth sailing.
Reviewers: amckinley, avivey
Reviewed By: avivey
Maniphest Tasks: T13147
Differential Revision: https://secure.phabricator.com/D19456
Summary:
See PHI652. When you `echo x | arc paste` today, you end up with a Paste object that has the empty string as its "language".
This is normally not valid. Pastes where the language should be autodetected should have the value `null`, not the empty string.
This behavior likely changed when `paste.create` got rewritten in terms of `paste.edit`. Adjust the implementation so it only adds the LANGUAGE transaction if there's an actual language.
Also, fix an issue where you can't use the "delete" key to delete tokens with the empty string as their value.
Test Plan:
- Created a paste with `echo x | arc paste`, got a paste in autodetect mode instead of with a bogus language value.
- Created a paste with `echo x | arc paste --lang rainbow`, got a rainbow paste.
- Deleted an empty string token with the keyboard.
- Deleted normal tokens with the keyboard.
- Edited subscribers/etc normally with the keyboard and mouse to make sure I didn't ruin anything.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19437
Summary:
See PHI624. Some of the mobile navigation and breadcrumbs in support pacts aren't as good as they could be.
In particular, we generally collapse crumbs on mobile to just the first and last crumbs. The first crumb is the application; the last is the current page.
On `/PHIxxx` pages, the first crumb isn't very useful since the Support landing page is two levels up: you usually want to go back to the pact, not all the way back to the Support landing page.
We also don't need the space since the last crumb (`PHIxxx`) is always small.
Allow Support and other similar applications to tailor the crumb behavior more narrowly if they end up in situations like this.
Test Plan:
- With an additional change to instances (see next diff), viewed a support issue page (`/PHI123`) on mobile and desktop.
- Saw a link directly back to the pact on both mobile and desktop.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19438
Summary:
See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off.
Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame.
Test Plan:
- Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL.
- Viewed a Jupyter notebook, toggled to "Source" view, saw blame.
- Viewed stuff in Files (no blame UI options).
- Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130, T13105
Differential Revision: https://secure.phabricator.com/D19414
Summary:
Ref T13130. See PHI617.
The new build log UI has tags like `<a href="...">Show More Above <span icon>^</span></a>`. If you click the little "^" icon, the event target is the `<span />` instead of the `<a />` so we expand on the wrong node.
Instead, select the `<a />` by sigil explicitly.
Test Plan: Viewed new log UI in Harbormaster, clicked "^" icon and text, got the same (correct) behavior on both.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130
Differential Revision: https://secure.phabricator.com/D19410
Summary: Ref T13126. After SourceView changes, embedded pastes with the `{Pxxx}` syntax are line-wrapping line numbers in Safari, at least. Put a stop to this.
Test Plan: Viewed a `{Pxxx}` with more than 10 lines. Before: weird line wrapping; after: nice consistent display.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13126
Differential Revision: https://secure.phabricator.com/D19393
Summary:
Ref T13127. Users with red/green colorblindness may have difficulty using this element in its current incarnation.
We could give it different behavior if the "Accessibility" option is set for red/green colorblind users, but try a one-size-fits-all approach since the red/green aren't wholly clear anwyay.
Test Plan: {F5530050}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13127
Differential Revision: https://secure.phabricator.com/D19385
Summary:
Ref T13124. Ref T13131. Fixes T8953. See PHI512.
When you receieve a notification about an object and then someone hides that object from you (or deletes it), you get a phantom notification which is very difficult to clear.
For now, test that notifications are visible when you open the menu and clear any that are not.
This could be a little more elegant than it is, but the current behavior is very clearly broken. This unbreaks it, at least.
Test Plan:
- As Alice, configured task stuff to notify me (instead of sending email).
- As Bailey, added Alice as a subscriber to a task, then commented on it.
- As Alice, loaded home and saw a notification count. Didn't click it yet.
- As Bailey, set the task to private.
- As Alice, clicked the notification bell menu icon.
- Before change: no unread notifications, bell menu is semi-stuck in a phantom state which you can't clear.
- After change: bad notifications automatically cleared.
{F5530005}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13131, T13124, T8953
Differential Revision: https://secure.phabricator.com/D19384
Summary:
Depends on D19377. Ref T13125. Ref T13124. Ref T13105. Coverage reporting in Diffusion didn't initially survive the transition to Document Engine; restore it.
This adds some tentative/theoretical support for multiple columns of coverage, but no way to actually produce them in the UI. For now, the labels, codes, and colors are hard coded.
Test Plan:
Added coverage with `diffusion.updatecoverage`, saw coverage in the UI:
{F5525542}
Hovered over coverage, got labels and highlighting.
Double-checked labels for "N" (Not Executable) and "U" (Uncovered). See PHI577.
Faked some multi-column coverage, but you can't currently get this yourself today:
{F5525544}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13125, T13124, T13105
Differential Revision: https://secure.phabricator.com/D19378
Summary: Ref T13105. After moving Diffusion to DocumnentEngine, this no longer has callers. It will become part of the document behavior.
Test Plan: Grepped for calls to the `diffusion-browse-file` behavior, found none.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19377
Summary:
Depends on D19349. Ref T13105. This was the behavior in Diffusion before with a little hard-coded snippet.
Remove that snippet ("diffusion-jump-to") and add a more general-purpose snippet to SourceView.
This is a tiny bit hacky still (and probably doesn't work quite right with Quicksand) but gets things working again and works in all of Files, Paste, and Diffusion.
Test Plan: Followed links to particular lines in Paste, Files and Diffusion; got scrolled to the right place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19350
Summary:
Depends on D19348. Ref T13105. When copying text from Paste or Diffusion, we'd like to copy only source, not line numbers.
We currently accomplish this with zero-width spaces plus a trigger that fires on "copy" in Paste and Diffusion. This is quite gross.
In the new-style Harbormaster logs, we use an approach that seems slightly better: CSS psuedoelements.
This isn't a complete solution (see also PHI504 / T5032) but puts us in a slightly better place.
Use it in Paste/Files/Diffusion too.
This gives us good behavior in all browsers in Files and Paste.
This gives us good behavior in Chrome and Firefox in Diffusion. Safari will copy (but not visually select) blame information in Diffusion. I think we can live with that for now.
Test Plan: Selected and copy/pasted stuff in Diffusion, Files, and Paste. Got good behavior everywhere except Safari + Diffusion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19349
Summary:
Depends on D19347. Ref T13105. See PHI565. The "highlight lines" behavior is interacting poorly with the new blame element in Diffusion.
Make the behavior a little simpler and hopefully more robust.
Test Plan:
- Clicked commit/revision links in Diffusion, saw the links get followed instead of the lines highlighted.
- Highlighted lines in Diffusion, saw just the line/code highlight instead of the whole thing.
- Highlighted lines in Paste and new-style Harbormaster build logs, saw consistent behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19348
Summary: Depends on D19346. Ref PHI568. I love Javascript.
Test Plan:
- Viewed a revision.
- Dragged file tree view really wide.
- Scrolled document to the right.
- Toggled file tree off and on by pressing "f" twice.
- Before patch: file tree grew wider and wider after it was toggled.
- After patch: file tree stayed the same size after it was toggled.
- Dragged to various widths and reloaded to make sure the "sticky across reloads" behavior still works.
- Scrolled right, dragged the tree a bit, then reloaded and didn't see it flip out.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19347
Summary:
Ref T13105. Previously, the "source code" view in Paste rendered on a brown/orange-ish background. I've been using this element in more contexts (Files, Diffusion) and removed the colored background to make text (particularly syntax-highlighted text) easier to read and reduce visual noise with the new blame colors.
In Diffusion the view is in a box with a white background so removing the background left us with white, but in Paste it's just directly on the page so the background was bleeding through. Instead, set it to white explicitly.
Test Plan: Viewed source files in Files, Diffusion and Paste; saw text on a white background.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19346
Summary:
See PHI568. If you make the file tree UI very wide so that the page generates a horizontal scrollbar and then scroll the page, the page content can paint underneath the menu.
The menu already has a z-index to make it render above the content, but doesn't actually have a background. Give it a background.
The "transparent" rule was added in D16346 but I don't see any reason why we actually need it there, so I think this probably won't break anything.
Test Plan: {F5518822}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120
Differential Revision: https://secure.phabricator.com/D19344
Summary: Ref T13118. The first fix there fixed Safari, but made Chrome weird. Try this?
Test Plan: Viewed a code block with `name=...` in Safari, Firefox and Chrome and saw consistent display without weird wrappping.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13118
Differential Revision: https://secure.phabricator.com/D19319
Summary:
Depends on D19313. Ref T13105. Fixes T13015. We lost the coloration for ages in the switch to Document Engine.
Restore it, and use a wider range of colors to make the information more clear.
Test Plan: Viewed some blame, saw a nice explosion of bright colors. This is a cornerstone of good design.
Maniphest Tasks: T13105, T13015
Differential Revision: https://secure.phabricator.com/D19314
Summary:
Depends on D19312. Ref T13105. For readability, render only one link for each contiguous block of changes.
Also make the actual rendering logic a little more defensible.
Test Plan: Viewed some files with blame, saw one render per chunk instead of one per line.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19313
Summary: Depends on D19311. Ref T13105. Currently, blame only renders on the initial request. Instead, redraw blame after swapping views.
Test Plan: Swapped from "Source -> Hexdump -> Source" and "Hexdump -> Source". Saw blame on source in all cases.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19312
Summary: Ref T13105. The line linker behavior currently has trouble identifying the line number when blame is active. Improve this, albeit not the most cleanly.
Test Plan: Selected lines with blame on.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19310
Summary: Ref T13105. This needs refinement but blame sort of works again, now.
Test Plan: Viewed files in Diffusion and Files; saw blame in Diffusion when viewing in source mode.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19309
Summary: Ref T13105. Ref T13047. This makes symbol indexes work with DocumentEngine in Files, and restores support in Diffusion.
Test Plan: Command-clicked stuff, got taken to the symbol index with reasonable metadata in Diffusion, Differential and Files.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105, T13047
Differential Revision: https://secure.phabricator.com/D19307
Summary:
Ref T13105. Fixes some issues with line linking and highlighting under DocumentEngine:
- Adding `$1-3` to the URI didn't work correctly with query parameters.
- Reading `$1-3` from the URI didn't work correctly because Diffusion parses them slightly abnormally.
Test Plan: Clicked/dragged lines to select them. Observed URI. Reloaded page, got the right selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19305
Summary: Fixes T13118. Ref T13120. This construction is a little odd; I'm not entirely sure why Safari is doing what it's doing, but this appears to fix it.
Test Plan: Viewed blocks like those in T13118 in Safari. Before the patch, weird last-letter wrapping. After the patch, sensible behavior.
Maniphest Tasks: T13118, T13120
Differential Revision: https://secure.phabricator.com/D19303
Summary: Ref T13110. See PHI230. Show revision sizes on a roughly logarithmic scale from 1-7 stars. See D16322 for theorycrafting on this element.
Test Plan: Looked at some revisions, saw plausible-looking size markers.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19294
Summary: Depends on D19273. Ref T13105. Adds "Change Text Encoding..." and "Highlight As..." options when rendering documents, and makes an effort to automatically detect and handle text encoding.
Test Plan:
- Uploaded a Shift-JIS file, saw it auto-detect as Shift-JIS.
- Converted files between encodings.
- Highlighted various things as "Rainbow", etc.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19274
Summary:
Ref T13114. See PHI522. Although it looks like results are already ordered correctly, the override rendering isn't accommodating disabled results gracefully.
Give closed results a distinctive look (grey + strikethru) so it's clear when you're autocompleting `@mention...` into a disabled user.
Test Plan: {F5497621}
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19272
Summary:
Depends on D19258. Ref T13105.
- When the default renderer is an Ajax renderer, don't replace the URI. For example, when viewing a Jupyter notebook, the URI should remain `/F123`, not instantly change to `/view/123/jupyter/`.
- Fix an issue where non-ajax renderers could fail to display the dropdown menu properly.
Test Plan:
- Viewed a Jupyter notebook, stayed on the same URI.
- Changed rendering, got different URIs.
- Viewed a JSON file and toggled renderers via dropdown.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19259
Summary:
Ref T13105. This adds various small client-side improvements to document rendering.
- In the menu, show which renderer is in use.
- Make linking to lines work.
- Make URIs persist information about which rendering engine is in use.
- Improve the UI feedback for transitions between document types.
- Load slower documents asynchronously by default.
- Discard irrelevant requests if you spam the view menu.
Test Plan: Loaded files, linked to lines, swapped between modes, copy/pasted URLs.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19256
Summary: Ref T13105. Allow normal text files to be rendered as documents, and add a "source code" rendering engine.
Test Plan: Viewed some source code.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19254
Summary:
Depends on D19252. Ref T13105. This very roughly renders Jupyter notebooks.
It's probably better than showing the raw JSON, but not by much.
Test Plan:
- Viewed various notebooks with various cell types, including markdown, code, stdout, stderr, images, HTML, and Javascript.
- HTML and Javascript are not live-fired since they're wildly dangerous.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19253
Summary:
Depends on D19251. Ref T13105. This adds rendering engine support for PDFs.
It doesn't actually render them, it just renders a link which you can click to view them in a new window. This is much easier than actually rendering them inline and at least 95% as good most of the time (and probably more-than-100%-as-good some of the time).
This makes PDF a viewable MIME type by default and adds a narrow CSP exception for it. See also T13112.
Test Plan:
- Viewed PDFs in Files, got a link to view them in a new tab.
- Clicked the link in Safari, Chrome, and Firefox; got inline PDFs.
- Verified primary CSP is still `object-src 'none'` with `curl ...`.
- Interacted with the vanilla lightbox element to check that it still works.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19252
Summary:
Ref T13105. Although Markdown is trickier to deal with, we can handle Remarkup easily.
This may need some support for encoding options.
Test Plan: Viewed `.remarkup` files, got remarkup document presentation by default. Viewed other text files, got an option to render as remarkup.
Reviewers: avivey
Reviewed By: avivey
Subscribers: mydeveloperday, avivey
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19251
Summary:
Depends on D19245. Fixes T11145. Ref T13108. See PHI488. Disable workflow buttons when they're clicked to prevent accidental client-side double submission.
This might have some weird side effects but we should normally never need to re-use a workflow dialog form so it's not immediately obvious that this can break anything.
Test Plan:
- Added `sleep(1)` to the Mute controller and the Maniphest task controller.
- Added `phlog(...)` to the Mute controller.
- Opened the mute dialog, mashed the button a thousand times.
- Before: Saw a bunch of logs.
- After: Button immediately disables, saw only one log.
Maniphest Tasks: T13108, T11145
Differential Revision: https://secure.phabricator.com/D19246
Summary:
See PHI488. Ref T13108. Currently, there is a narrow window between when the response returns and when the browser actually follows the redirect where the form is live and you can click the button again.
This is relativey easy if Phabricator is running //too fast// since the button may be disabled only momentarily. This seems to be easier in Firefox/Chrome than Safari.
Test Plan:
- In Firefox and Chrome, spam-clicked a comment submit button.
- Before: could sometimes get a double-submit.
- After: couldn't get a double-submit.
- This could probably be reproduced more reliabily by adding a `sleep(1)` to whatever we're redirecting //to//.
- Submitted an empty comment, got a dialog plus a still-enabled form (so this doesn't break the non-redirect case).
Maniphest Tasks: T13108
Differential Revision: https://secure.phabricator.com/D19245
Summary:
Depends on D19238. Ref T13105. Give document engines some reasonable automatic support for degrading gracefully when someone tries to hexdump a 100MB file or similar.
Also, make "Video" sort above "Audio" for files which could be rendered either way.
Test Plan: Viewed audio, video, image, and other files. Adjusted limits and saw full, partial, and fallback/error rendering.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19239
Summary: Depends on D19237. Ref T13105. This adds a (very basic) "Hexdump" engine (mostly just to have a second option to switch to) and a selector for choosing view modes.
Test Plan: Viewed some files, switched between audio/video/image/hexdump.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19238
Summary:
Ref T13105. This change begins modularizing document rendering. I'm starting in Files since it's the use case with the smallest amount of complexity.
Currently, we hard-coding the inline rendering for images, audio, and video. Instead, use the modular engine pattern to make rendering flexible and extensible.
There aren't any options for switching modes yet and none of the renderers do anything fancy. This API is also probably very unstable.
Test Plan: Viewwed images, audio, video, and other files. Saw reasonable renderings, with "nothing can render this" for any other file type.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19237
Summary:
See <https://discourse.phabricator-community.org/t/desktop-only-notifications-mode-is-broken/1234>. Ref T13102. The "Desktop Only" mode for notifications currently shows both desktop and web notifications.
In fact, `JX.Notification` currently has no ability to render notifications as desktop-only. Make this work.
Note that many of the variables and parameters here, including `showAnyNotification`, `web_ready`, and `desktop_ready`, are named in an incorrect or misleading way. However, the new behavior appears to be correct.
Test Plan:
- Emitted test notifications in "No Notifications", "Web Only", "Web and Desktop", and "Desktop" modes.
- Saw appropriate notifications appear in the UI.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19233
Summary: Ref T13106. When profiling service queries, there's no convenient way to easily get a sense of why a query was issued. Add a mode to collect traces for each query to make this more clear. This is rough, but works well enough to be useful.
Test Plan: Clicked "Analyze Query Plans", got stack traces for each service call.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19221
Summary:
See PHI451. Ref T13102. DarkConsole uses an ancient inline "onclick" handler to expand the stack traces for errors.
The new Content-Security-Policy prevents this from functioning.
Replace this with a more modern behavior-driven action instead.
Test Plan:
- Clicked some errors in DarkConsole, saw stack traces appear.
- Grepped for `onclick` and `jsprintf()` to see if I could find any more of these, but came up empty.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19218
Summary:
Fixes T12994. We need `MYSQLI_ASYNC` to implement client-side query timeouts, and we need MySQLi + MySQL Native Driver to get `MYSQLI_ASYNC`.
Recommend users install MySQLi and MySQL Native Driver if they don't have them. These are generally the defaults and best practice anyway, but Ubuntu makes it easy to use the older stuff.
All the cases we're currently aware of stem from `apt-get install php5-mysql` (which explicitly selects the non-native driver) so issue particular guidance about `php5-mysqlnd`.
Test Plan:
- Faked both issues locally, reviewed the text.
- Will deploy to `secure`, which currently has the non-native driver.
Maniphest Tasks: T12994
Differential Revision: https://secure.phabricator.com/D19216
Summary:
Ref T13102. An install has a custom rule for bridging JIRA references via Doorkeeper and would like to be able to render them as `JIRA-123` instead of `JIRA JIRA-123 Full JIRA title`.
I think it's reasonable to imagine future support upstream for `JIRA-123`, `{JIRA-123}`, and so on, although we do not support these today. We can take a small step toward eventual support by letting the rendering pipeline understand different view modes.
This adds an optional `name` (the default text rendered before we do the OAuth sync) and an optional `view`, which can be `short` or `full`.
Test Plan:
I tested this primarily with Asana, since it's less of a pain to set up than JIRA. The logic should be similar, hopefully.
I changed `DoorkeeperAsanaRemarkupRule` to specify `name` and `view`, e.g `'view' => (mt_rand(0, 1) ? 'short' : 'full')`. Then I made a bunch of Asana references in a comment and saw them randomly go short or long.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19215
Summary: Ref T13103. Make favicons customizable, and perform dynamic compositing to add marker to indicate things like "unread messages".
Test Plan: Viewed favicons in Safari, Firefox and Chrome. With unread messages, saw pink dot composited into icon.
Maniphest Tasks: T13103
Differential Revision: https://secure.phabricator.com/D19209
Summary:
See <https://discourse.phabricator-community.org/t/lightbox-not-working/1201/3>. The lightbox code is fragile and currently relies on simulating a click on the actual "<a />" tag surrounding other images in the document.
This breaks the prev/next links which ignore the event because it there's no "<img />".
Instead, don't simulate clicks and just call the code we want directly.
Test Plan: Added several images to a page, used lightbox prev/next buttons to cycle between them.
Differential Revision: https://secure.phabricator.com/D19197
Summary:
Depends on D19192. Ref T4190. Ref T13101. Instead of directly including the proxy endpoint with `<img src="..." />`, emit a placeholder and use AJAX to make the request. If the proxy fetch fails, replace the placeholder with an error message.
This isn't the most polished implementation imaginable, but it's much less mysterious about errors.
Test Plan: Used `{image ...}` for valid and invalid images, got images and useful error messages respectively.
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19193
Summary: Depends on D19190. Fixes T12590. Ref T13099. Replaces the barely-usable, gigantic, poorly ordered "<select />" control with a tokenizer. Attempts to fix various minor issues.
Test Plan:
- Edited paths: include/exclude paths, from different repositories, different actual paths.
- Used "Add New Path" to add rows, got repository selector prepopulated with last value.
- Used "remove".
- Used validation typeahead, got reasonable behaviors?
The error behavior if you delete the repository for a path is a little sketchy still, but roughly okay.
Maniphest Tasks: T13099, T12590
Differential Revision: https://secure.phabricator.com/D19191
Summary: Ref T12590. This is ancient code which was used to prefill `/trunk/tfb/www/` or similar at Facebook. I don't think it ever had a UI and no install has asked for this feature since 2011.
Test Plan: Grepped for affected symbols, edited paths in Owners.
Maniphest Tasks: T12590
Differential Revision: https://secure.phabricator.com/D19189
Summary:
Depends on D19183. Ref T11015. Currently, adding a trailing slash works great and omitting it mysteriously doesn't work.
Store a normalized version with an unconditional trailing slash for the lookup logic to operate on, and a separate display version which tracks what the user actually typed.
Test Plan:
- Entered "/src/main.c", "/src/main.c/", saw them de-duplicate.
- Entered "/src/main.c", saw it stay that way in the UI but appear as "/src/main.c/" internally.
- Added a rule for "/src/applications/owners" (no slash), created a revision touching paths in that directory, saw Owners fire for it.
- Changed the display value of a path only ("/src/main.c" to "/src/main.c/"), saw the update reflected in the UI without any beahvioral change.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19184
Summary:
Ref T13099. See that task for discussion. Chrome is unhappy with an MFA form submitting to an endpoint which redirects you to an OAuth URI.
Instead, do the redirect entirely on the client.
Chrome's rationale here isn't obvious, so we may be able to revert this at some point.
Test Plan: Went through the OAuth flow locally, was redirected on the client. Will verify in production.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19177
Summary:
Depends on D19166. Ref T13088. When the user scrolls away from a followed log, break the focus lock.
Let users stop following a live log.
Show when lines are added more clearly.
Don't refresh quite as quickly give users a better shot at clicking the stop button.
These behaviors can probably be refined but are at least more plausible and less actively user-hostile than the first version of this behavior was.
Test Plan: Used `write-log --rate` to write a large log slowly. Clicked "Follow Log", followed for a bit. Scrolled away, still got live updates but no more scroll lock. Clicked stop, no more updates.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19167
Summary:
Depends on D19165. Ref T13088. Currently, in other applications, we use Zero Width Spaces and Javascript "copy" listeners to prevent line numbers from being copied. This isn't terribly elegant.
Modern browsers support a second approach: using psuedo-elements with `content`. Try this in Harbormaster since it's conceptually cleaner, at least. One immediate drawback is that Command-F can't find this text either.
Test Plan: In Safari, Chrome and Firefox, highlighted ranges of lines and copy/pasted text. Got just text (no line numbers) in all cases.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19166
Summary: Depends on D19164. Ref T13088. Now that the JS behaviors are generic, use them on the Harbormaster standalone page.
Test Plan: Clicked lines and dragged across line ranges. Reloaded pages. Saw expected highlighting behavior in the client and on the server across reloads.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19165
Summary: Depends on D19163. Ref T13088. Increase the generality of this code so it can be shared with Harbormaster.
Test Plan: Clicked individual lines, clicked-and-dragged, etc., in Paste. Got sensible URI and highlight behaviors.
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19164
Summary:
Depends on D19162. Ref T13088. When a user links to `$1234`, we need to render a default view of the log with a piece at the head, a piece at the end, and a piece in the middle.
We also need to figure out the offset for line 1234, or multiple offsets for "1234-2345".
Since the logic views/reads mostly anticipated this it isn't too much of a mess, although there are a couple of bugs this exposes with view specifications that use combinations of parameters which were previously impossible.
Test Plan: Viewed a large log with no line marker. Viewed `$1`. Viewed `$end`. Viewed `$35-40`, etc. Expanded context around logs.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19163
Summary: Depends on D19156. Ref T13094. This replaces the remaining forms in the file embed view and lightbox with normal download links.
Test Plan: Clicked "Download" and lightbox -> download for embedded files.
Maniphest Tasks: T13094
Differential Revision: https://secure.phabricator.com/D19157
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary: Depends on D19152. Ref T13088. This adds live log tailing. It is probably not the final version of this feature because it prevents escape once you begin tailing a log.
Test Plan: Used `bin/harbormaster write-log --rate ...` to write a log slowly. Viewed it in the web UI. Clicked "Follow Log". Followed the log until the write finished, a lifetime later.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19153
Summary:
See PHI399. Ref T4340. This header provides an additional layer of protection against various attacks, including XSS attacks which embed inline `<script ...>` or `onhover="..."` content into the document.
**style-src**: The "unsafe-inline" directive affects both `style="..."` and `<style>`. We use a lot of `style="..."`, some very legitimately, so we can't realistically get away from this any time soon. We only use one `<style>` (for monospaced font preferences) but can't disable `<style>` without disabling `style="..."`.
**img-src**: We use "data:" URIs to inline small images into CSS, and there's a significant performance benefit from doing this. There doesn't seem to be a way to allow "data" URIs in CSS without allowing them in the document itself.
**script-src** and **frame-src**: For a small number of flows (Recaptcha, Stripe) we embed external javascript, some of which embeds child elements (or additional resources) into the document. We now whitelist these narrowly on the respective pages.
This won't work with Quicksand, so I've blacklisted it for now.
**connect-src**: We need to include `'self'` for AJAX to work, and any websocket URIs.
**Clickjacking**: We now have three layers of protection:
- X-Frame-Options: works in older browsers.
- `frame-ancestors 'none'`: does the same thing.
- Explicit framebust in JX.Stratcom after initialization: works in ancient IE.
We could probably drop the explicit framebust but it wasn't difficult to retain.
**script tags**: We previously used an inline `<script>` tag to start Javelin. I've moved this to `<data data-javelin-init ...>` tags, which seems to work properly.
**`__DEV__`**: We previously used an inline `<script>` tag to set the `__DEV__` mode flag. I tried using the "initialization" tags for this, but they fire too late. I moved it to `<html data-developer-mode="1">`, which seems OK everywhere.
**CSP Scope**: Only the CSP header on the original request appears to matter -- you can't refine the scope by emitting headers on CSS/JS. To reduce confusion, I disabled the headers on those response types. More headers could be disabled, although we're likely already deep in the land of diminishing returns.
**Initialization**: The initialization sequence has changed slightly. Previously, we waited for the <script> in bottom of the document to evaluate. Now, we go fishing for tags when domcontentready fires.
Test Plan:
- Browsed around in Firefox, Safari and Chrome looking for console warnings. Interacted with various Javascript behaviors. Enabled Quicksand.
- Disabled all the framebusting, launched a clickjacking attack, verified that each layer of protection is individually effective.
- Verified that the XHProf iframe in Darkconsole and the PHPAST frame layout work properly.
- Enabled notifications, verified no complaints about connecting to Aphlict.
- Hit `__DEV__` mode warnings based on the new data attribute.
- Tried to do sketchy stuff with `data:` URIs and SVGs. This works but doesn't seem to be able to do anything dangerous.
- Went through the Stripe and Recaptcha workflows.
- Dumped and examined the CSP headers with `curl`, etc.
- Added a raw <script> tag to a page (as though I'd found an XSS attack), verified it was no longer executed.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19143
Summary:
Depends on D19141. Ref T13088. Some of the fundamental log behaviors like "loading the correct rows" are now a bit better behaved.
The UI is a little less garbage, too.
Test Plan: Viewed some logs and loaded more context by clicking the buttons.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19142
Summary: Depends on D19139. Ref T13088. This doesn't actually work, but is close enough that a skilled attacker might be able to briefly deceive a small child.
Test Plan:
- Viewed some very small logs under very controlled conditions, saw content.
- Larger logs vaguely do something resembling working correctly.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19141
Summary:
Ref T13090. The default width changed recently to become much wider, but the behavior on this control isn't great. Instead:
- Pick a default width somewhere between the two.
- Make the width sticky across show/hide (pressing "f" twice remembers your width instead of resetting it).
- Make the width sticky across reloads (dragging the bar, then reloading the page keeps the bar in the same place).
Test Plan:
- Without settings, loaded page: got medium-width bar.
- Dragged bar wide/narrow, toggled on/off with "f", got persistent width.
- Dragged bar wide/narrow, reloaded page, got persistent width.
- Dragged bar wide/narrow, toggled it off, reloaded page, toggled it on, got persistent width.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19129
Summary:
Ref T13077. The autosuggester is a little too eager right now, and will eat carriage returns after typing `[` if you never activate the tokenizer.
To fix this, try just canceling sooner. If that doesn't work, we might need to cancel more eagerly by testing to see if the tokenizer is actually open.
Test Plan: Typed `[x]<return>`, got my return instead of getting trapped by the autosuggester.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19110
Summary: Depends on D19107. Ref T13077. The underlying datasource may need some adjustment but this appears to work properly locally.
Test Plan: Typed `[[ por` locally, was suggested "Porcupine Facts". Typed `[[ / ]]`, saw it render as a reference to the wiki root instead of the install root.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19108
Summary:
Depends on D19106. Fixes T5941. Ref T13077. Allows you to find Phriction documents as suggestions from global quick search.
Also supports `w` to jump to Phriction and `w query` to query Phriction.
The actual query logic for the datasource may need some tweaking after it collides with reality, but seems to produce fairly reasonable results in local testing against synthetic data.
Test Plan: Searched for "Porcupine Facts", "Travel Companions", and other useful local pages. Searched for `w`. Searched for `w zebra facts`.
Maniphest Tasks: T13077, T5941
Differential Revision: https://secure.phabricator.com/D19107
Summary: Fixes T13080. The banner wasn't properly included in the sleep/wake logic.
Test Plan:
Mentioned `Dxxx` on a task. Enabled persistent chat to activate Quicksand. Reloaded page. Clicked `Dxxx`. Scrolled down until a changeset header appeared. Pressed back button.
- Before patch: ended up on task, with header still around.
- After patch: ended up on task, with header properly vanquished.
Pressed "forward", ended up back on the revision with the header again.
Maniphest Tasks: T13080
Differential Revision: https://secure.phabricator.com/D19086
Summary: See PHI356. Adds inline comment and done counts to the filetree. Also makes the filetree wider by default.
Test Plan: Fiddled with filetrees in different browsers on different revisions. Added inlines, marked them done/undone.
Differential Revision: https://secure.phabricator.com/D19041
Summary: See PHI356. Makes it easier to pick out change types in the filetree view in Differential.
Test Plan: Created a diff with adds, copies, moves, deletions, and binary files. Viewed in Differential, had an easier time picking stuff out.
Differential Revision: https://secure.phabricator.com/D19040
Summary: Ref T13053. See PHI126. Add an explicit "Mute" action to kill mail and notifications for a particular object.
Test Plan: Muted and umuted an object while interacting with it. Saw mail route appropriately.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19033
Summary: See PHI333. Some of the cleanup at the tail end of the bulk edit changes made "Assign To" stop working properly, since we don't strip the `array(...)` off the `array(PHID)` value we receive.
Test Plan:
- Used bulk editor to assign and unassign tasks (single value datasource).
- Used bulk editor to change projects (multi-value datasource).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18975
Summary:
See PHI325. When a transaction group in Differential (or Pholio) only has an inline comment, it renders with a "V" caret but no actual dropdown menu.
This caret renders in a "disabled" color, but the color is "kinda grey". The "active" color is "kinda grey with a dab of blue". Here's what they look like today:
{F5401581}
Just remove it.
Test Plan: Viewed one of these, no longer saw the inactive caret.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18963
Summary:
Depends on D18939. Ref T13047. Symbol lookup can be activated from a diff (in Differential or Diffusion) or from the static view of a file at a particular commit.
In the latter case, we need to figure out the path a little differently. The character and line number approaches still work as written.
Test Plan:
- Command-clicked symbols in the Diffusion browse view with blame on and off; saw path, line and char populate properly.
- Command-clicked symbols in Differential diff view to check I didn't break anything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18940
Summary: Depends on D18937. Ref T13047. When available, provide character positions so external indexers can return more accurate results.
Test Plan: Clicked symbols in Safari, Firefox and Chrome, got sensible-looking character positions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18939
Summary:
Ref T13047. In some reasonable cases, knowing the path and line number where a symbol appears is useful in ranking or filtering the set of matching symbols.
Giving symbol sources more information can't hurt, and it's generally free for us to include this context since we just need to grab it out of the document and pass it along.
We can't always get this data (for example, if a user types `s idx` into global search, we have no clue) but this is similar to other types of context which are only available sometimes (like which repository a symbol appears in).
Test Plan: Command-clicked some symbols in 1-up (unified) and 2-up (side-by-side) diff views with symbol indexes configured. Got accurate path and line information in the URI I was redirected to.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18936
Summary:
See PHI173. Currently, Herald has an "Assign to" action for tasks, and you can specify custom fields with datasource values (like users or projects) that have a limit (like 1 "Owner", or 12 "Jury Members").
Herald doesn't support these limits right now, so you can write `[ Assign to ][ X, Y, Z ]`. This just means "Assign to X", but make it more clear by actually enforcing the limit in the UI.
Test Plan:
- Created a "projects" custom field with limit 1.
- Tried to create actions that 'assign to' or 'set custom field to' more than one thing, got helpfully rebuffed by the UI.
- Created an "add subscribers" action with more than one value.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18887
Summary: Ref T13025. We're getting kind of a lot of actions, so put them in nice groups so they're easier to work with.
Test Plan: {F5386038}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025
Differential Revision: https://secure.phabricator.com/D18880
Summary:
Depends on D18866. Ref T13025. Fixes T12415. This makes the old "Add Comment" action work, and adds support for a new "Set description to" action (possibly, I could imagine "append description" being useful some day, maybe).
The implementation is just a `<textarea />`, not a whole fancy remarkup box with `[Bold] [Italic] ...` buttons, preview, typeaheads, etc. It would be nice to enrich this eventually but doing the rendering in pure JS is currently very involved.
This requires a little bit of gymnastics to get the transaction populated properly, and adds some extra validation since we need some code there anyway.
Test Plan:
- Changed the description of a task via bulk editor.
- Added a comment to a task via bulk editor.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T12415
Differential Revision: https://secure.phabricator.com/D18867
Summary:
Depends on D18862. See PHI173. Ref T13025. Fixes T10005. This redefines bulk edits in terms of EditEngine fields, rather than hard-coding the whole thing.
Only text fields -- and, specifically, only the "Title" field -- are supported after this change. Followup changes will add more bulk edit parameter types and broader field support.
However, the title field now works without any Maniphest-specific code, outside of the small amount of binding code in the `ManiphestBulkEditor` subclass.
Test Plan: Used the bulk edit workflow to change the titles of tasks.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T10005
Differential Revision: https://secure.phabricator.com/D18863
Summary:
Depends on D18805. Ref T13025. Fixes T10268.
Instead of using a list of IDs for the bulk editor, power it with SearchEngine queries. This gives us the full power of SearchEngine and lets us use a query key instead of a list of 20,000 IDs to avoid issues with URL lengths.
Also, split it into a base `BulkEngine` and per-application subclasses. This moves us toward T10005 and universal support for bulk operations.
Also:
- Renames most of "batch" to "bulk": we're curently inconsitent about this, I like "bulk" better since I think it's more clear if you don't regularly interact with `.bat` files, and newer stuff mostly uses "bulk".
- When objects in the result set can't be edited because you don't have permission, show the status more clearly.
This probably breaks some stuff a bit since I refactored so heavily, but it seems mostly OK from poking around. I'll clean up anything I missed in followups to deal with remaining items on T13025.
Test Plan:
{F5302300}
- Bulk edited from Maniphest.
- Bulk edited from a workboard (no more giant `?ids=....` in the URL).
- Hit most of the error conditions, I think?
- Clicked the "Cancel" button.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T10268
Differential Revision: https://secure.phabricator.com/D18806
Summary:
Ref T13025. See PHI50. Fixes T11286. Ref T10005. Begin modernizing the bulk editor.
For T10005 ("move the bulk editor to modern infrastructure"), rewrite the rendering of the editable set so that it is application-agnostic and can work with any kind of object.
For T11286 ("let users de-select items in the working set"), make the working set editable.
Test Plan:
{F5302158}
- Deselected some objects, applied an edit, saw the edit apply to only selected objects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T11286, T10005
Differential Revision: https://secure.phabricator.com/D18805
Summary:
See PHI242. All use cases for this that I know of are pretty hacky, but they don't seem perilous, and it's easier than webhooks.
See P1895, T10183, and T9853 for me previously refusing to implement this since all those use cases were also pretty bad.
Test Plan:
- Wrote a rule to add comments, saw it add comments.
- Reviewed summary, re-edited rule, reviewed transcript to check that all the strings worked OK.
- Wrote a new rule for a non-commentable object (a blog) to make sure I wasn't offered the "Add a comment" action.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18823
Summary:
Ref T10233. See PHI231. When users ignore the `arc land` prompt about bad revision states, make it explicitly clear in the transaction log that they broke the rules.
You can currently figure this out by noticing that there's no "This revision is accepted and ready to land." message, but it's unrealistic to expect non-expert users to look for the //absence// of a message to indicate something, and this state change is often relevant.
Test Plan: {F5302351}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10233
Differential Revision: https://secure.phabricator.com/D18808
Summary:
Ref T13018. See that task and the Discourse thread for discussion.
This doesn't work as-is and we need to `og:description` everything to make it work. I don't want to sink any more time into this so just back all the changes out for now.
(The `<html>` change is unnecessary anyway.)
Test Plan: Strict revert.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13018
Differential Revision: https://secure.phabricator.com/D18782
Summary:
See PHI180. Currently, if you begin creating or editing an inline and then swap display modes (for example, with "View Unified"), your edit is lost.
Persisting the editor state is complicated and this is very rare, so just prevent the action and warn the user instead.
Also make the warning persist for a little longer since a few of the messages, including this one, take a couple seconds to read now.
Test Plan:
- Edited a comment, tried to swap display modes, got a warning.
- Swapped display modes normally with no comment being edited.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18774
Summary:
See PHI185. When looking at a revision, you can Command-Click (Mac) symbols to jump to their definitions (provided the symbol index has been built).
Currently, the code works on any node inside the changeset list, so it activates when clicking links inside inline comments and opening them in a new window.
To avoid this, don't activate if we're inside an inline comment. This technically prevents you from doing a symbol lookup on a symbol inside a codeblock inside an inline, but that seems fine/reasonable.
Test Plan: Wrote `Dxxx` in an inline, command-clicked it. Before: got a symbol lookup. After: just a new tab with the revision.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18753
Summary: Give profile images a little more space, fix "/" spacing, add a tooltip.
Test Plan: {F5251205}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18749
Summary: Depends on D18746. See PHI174. Adds small author portraits next to each blame line (this is similar to GitHub).
Test Plan:
My local test data isn't that great since I don't have commits from a lot of accounts, but looks functional:
{F5251056}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18747
Summary:
Ref PHI174. This reverts most of these changes:
- 37843127e9 / D18481
- 94cad30ac3 / D18474
- 12ae08b6b1 / D18473
- 0a01334172 / D18462
- ac91ab1ef9 / D18452
These changes made the Diffusion blame view very similar to GitHub's blame view. See D18452 for a before/after of the bulk of these changes; the other revisions are bugfixes.
I think this was generally a step backward, and not motivated by solving a specific problem. I've found the new UI less usable than the old one, and at least one install (see PHI174) also has.
In particular, the revision/commit titles are very bulky and not terribly useful; the date column also isn't terribly useful; the "age" color actually IS pretty useful and was heavily de-emphasized.
I've kept one bugfix here (missing `'a'` tag type) and kept the upgraded icon for "Skip Past This Commit".
I'm going to follow this up with some additional changes:
- Show a small author profile icon, similar to GitHub, to address PHI174 more directly.
- Try a zebra-stripe on blocks of rows to make it more clear where changes affected by a particular commit begin and end.
- Try a hue shift, not just a brightness/saturation shift, to make the "age" color more distinct.
- Try computing colors as even steps, not based purely on age. Currently, if a file has one long-distant commit and several recent commits, all the recent ones show up as very bright green. I think this would probably be more useful if they were distributed more evenly across the available color bands.
Test Plan:
Viewed blame views in Diffusion, saw a more compact UI similar to the old UI.
{F5251019}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18746
Summary: Hide navbar, and make curtain behave like on a phone, when printing.
Test Plan: {F5197340}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18583
Summary:
Fixes T12979. In D18457, we added a "No Notifications" setting to let users disable the blue and yellow pop-up notifications that alert you when an object has been updated, since some users found them distracting.
However, the change made "do nothing" the default, so all other `JX.Notification` callsites -- which never pass a preference -- were effectively turned off no matter what your setting was set to. This includes the "Read-Only" mode warning (grey), the "High Security" mode warning (purple), the "timezone" warning, and a few others.
Tweak things a little bit so the setting applies to ONLY blue and yellow ("object you're following was updated" / "this object was updated") notifications, not other types of popup notifications.
Test Plan:
- With notifications on in settings, got blue notifications and "Read-only".
- With notifications off in settings, got "Read-only" but no blue notifications.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12979
Differential Revision: https://secure.phabricator.com/D18600
Summary: Adds some side margin here.
Test Plan: error out a form field in a white box
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18571
Summary: This simplifies EditEngine pages in general by removing the dual header, and extending to allow setting of a custom PHUIHeaderView if needed (like settings).
Test Plan:
Review all settings pages, review task, project pages. This should all be fine, but is a big change maybe some layouts I'm not considering. Tested these all mobile, destkop as well.
{F5166181}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18527
Summary: Updates settings panel UI for new white box, cleans up other various UI nitpicks.
Test Plan: Click through each setting that had a local setting page. Edit Engine pages will follow up on another diff.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18526
Summary: Simplifies the Repository Management pages to the new fixed column layout. I've also moved "Status" into the Basics page, which feels better, and moved "Documentation" as a nav item to a button in the header. This removed "action list" and "curtain view" from the management panels and uses the new bits from Config/Phacility. Undecided if the icons should stay or go for the nav. Left them in for Diffusion. I want to update the EditEngine pages to display in this UI and not leave the portal, but I haven't dug into that this page. I'm a bit worried it will not easily be possible.
Test Plan:
Generate a svn, git, hg repository, test each of the new pages and each of the new buttons. Activate, deactivate, etc.
{F5164674}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18523
Summary:
This is a full UI pass at a cleaner "Config" application. The main idea is to simplify the UI, center it, and have a different feel than other UI, a sort of "manage" UI theme for objects with loads of settings. Also adds a new minimalistic "WHITE_CONFIG" box type which may get re-used in Diffusion settings. This is a 90% pass, I'll have a few follow up diffs. Specifically:
- Build breadcrumbs as a flexible UI to go into headers.
- One click ObjectItemView option, for hover states.
- Sidenav doesn't always select (AphrontFilter issue)
- Mobile touchups, though it's pretty reasonable.
Test Plan:
Click through every page here, edit options, see new navigation UI. Test a few various setup issue layouts including fatals.
{F5163228}
{F5163229}
{F5163230}
{F5163231}
{F5163232}
{F5163233}
{F5163234}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18519
Summary: Custom icons here aren't being set. Also use more standard `tt` UI.
Test Plan: Set an icon, see set Icon.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18522
Summary: I can't seem to blame any rational for this, these should vertical align.
Test Plan: Project headers with watching buttons.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18520
Summary: This is an older CSS rule that's no longer needed on mobile.
Test Plan: view history view with harbormaster results
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18507
Summary: Implements a new mobile view thats more fullscreen, not boxed, so more space. Fixes issues with mobile tables when scrolling overflowed content.
Test Plan: Test home, branch, tags, code, file browse, graph, compare, history, readme, open revisions, owners.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18505
Summary: Only for grey buttons, but can expand. Sets a selected class.
Test Plan: Review new changes in UIExamples.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18501
Summary: Simplifies the UI here by removing various borders, instead using just background colors and better alignment.
Test Plan: Test instances, settings, home, projects, workboards.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18488
Summary: Fixes T8944. Adds a small dot if notification is new along with color. Goes away when clicked. Increased font and padding for readability.
Test Plan: Send notifications from test account, review them in menu, application search, and in real-time display.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T8944
Differential Revision: https://secure.phabricator.com/D18485
Summary: My fake data was 100%, and not all tables have full revision history. This leads to a broken table. Instead check if we have //any// revisions at all, then always show the column, with or without a link inside.
Test Plan: going on a limb this is the correct fix and test on secure... again ...
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18474
Summary: There is still some layout issues with revisions, so I've tested it better and moved it to it's own column
Test Plan: Fake in some revision data, test various sizes and shapes.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18473
Summary: Ref D17991, this rule got more specific with shade tag re-write, so needs updating for headers.
Test Plan: Review a differential header, first icon is now 15px instead of 12px
Reviewers: epriestley, avivey
Reviewed By: avivey
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18472
Summary: This is wierd and I can't think of a use for it? Causing issues on hover states.
Test Plan: Action list, menus, dropdowns, etc. Labels shouldn't have hover states.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18463
Summary: Using icons and dropdown buttons without text looks a little wonky, this resets the CSS a bit.
Test Plan: Review button with icon and text, just icon, just test, and dropdowns.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18461
Summary: These items should be floated, not display: block.
Test Plan: Test blame view with commits AND revisions, check they display inline.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18462
Summary: Fixes T12792. Expands the Notifications to "web, desktop, both, or none" for real-time notifications in settings.
Test Plan: Test with "test notifications" button, and while logged into two accounts with each of the 4 settings.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T12792
Differential Revision: https://secure.phabricator.com/D18457
Summary: Ref T12824, adds more information to the blame view, exposes date, commit summary, lighter colors.
Test Plan:
Review many diffs with and without blame on.
{F5111758}
{F5111759}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18452
Summary: Moves browseFile to single column, implements Owners as a list under the file (and now directory as well), improved information listed in Owners, and moves actions into the Diffusion action bar instead of the header.
Test Plan:
Test browsing directories, files, text, images, binaries, enabling blame. Mobile and desktop.
{F5111045}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18448
Summary: These can wrap on iPhone 5 screens, no need for so much padding.
Test Plan: iPhone 5 simulator, tablet and mobile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18437
Summary: Simplifies the page, adds base support for PHUITwoColumn fixed from Instances (which I'll delete css there).
Test Plan:
click on every settings page, UI seems in tact, check mobile, desktop, mobile menus.
{F5102572}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18436
Summary: Moves the method up to DiffusionController, so it can be more universally used. Also now center aligns tabs on mobile. Still todo, get search nicely toggled on mobile
Test Plan: Test mobile, desktop. Test search from home, from browse, and browsing a specific path.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18432
Summary: Moving this down the the "bar" to allow pattern search on home. Rebuilds the mobile layout a little.
Test Plan:
Test actions on mobile, desktop, tablet.
{F5100460}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18431
Summary: Roughs this in a little, kinda basic. Allows for grouping results by page. A bit better on mobile. Would like more content return from conduit though.
Test Plan:
Test `CMS`, `cms`, and `OMGLOLWTFBBQ`, desktop and mobile
{F5099081}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18429
Summary: This is only on browse pages, but I think could be global (home) also. Moves it from a button, field, to just a field.
Test Plan:
Review search on desktop, mobile.
{F5098886}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18428
Summary: Uses `fire`, `underline`. Sets text that overflows to ellipsis.
Test Plan:
Test searching for CMS in Phabricator. Check other typeaheads, tokenizers.
{F5098496}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18424
Summary: Getting to the straight browse view went away, this adds a link back. I'll look at more long term solution for getting to grep search.
Test Plan: Click on header, get take to browse view.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18421
Summary: The things you find testing production.
Test Plan: Run sprite, celerity
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18409
Summary:
Fixes T12960. When the user enters a date like "1917", we currently loop ~20 million times.
Instead:
- Be a little more careful about parsing.
- Javascript's default behavior of interpreting "2001-02-31" as "2001-03-03" ("February 31" -> "March 3") already seems reasonable, so just let it do that.
Test Plan:
Verified these behaviors:
- `2017-08-08` - Valid, recent.
- `17-08-08` - Valid, recent.
- `1917-08-08` - Valid, a century ago, no loop.
- `2017-02-31` - "February 31", interpreted as "March 3" by Javascsript, seems reasonable.
- `Quack` - Default, current time.
- `0/0/0`, `0/99/0` - Default, current time.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T12960
Differential Revision: https://secure.phabricator.com/D18383
Summary: Makes dialogs a little wider, form dialogs a lot wider (space controls). Also cleans up Passphrase dialogs. Fixes T12833. I think forms probably need to move to tables for better layout flexibility like veritical alignment.
Test Plan: Passphrase create, edit, etc. Other dialogs.
Reviewers: epriestley
Subscribers: Korvin
Maniphest Tasks: T12833
Differential Revision: https://secure.phabricator.com/D18382
Summary: Centers tabs when used above the page header when on mobile.
Test Plan: Test mobile and desktop layouts of Instances.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18368
Summary: If we don't have any panels, just an action list, we want to hide the entire box on mobile since it's just an empty line.
Test Plan: Review Owners, Differential curtains on mobile, desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18350
Summary: We don't ever set fluid, since it already is fluid, also no CSS. Add an actual fixed version.
Test Plan: For use in Instances.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18336
Summary: Rather than have tabs live in two column view, sometimes like `admin` we'll want a global set of tabs that work well with all layouts and crumbs.
Test Plan:
I tested this in an upcoming diff for instances.
{F5080228}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18332
Summary: Allows setting on an image here if wanted.
Test Plan: Set a rocket to launch a new instance on rSAAS
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18334
Summary:
Ref T10252. The previous fix rPa8a9fddb0738 only works for macOS.
Under Windows the @ symbol is composed of AltGr+q. For Chrome and Edge the "AltGr" keypressEvent is like pressing the Control key and the Alt key at the same time.
This fix changes the condition in such a way, that this case (pressing Control and Alt at the same time) is not blocked.
Test Plan:
Testing for the issue:
- Launch Windows 10, Select German Keyboard, Use latest Chrome (60)
- Observe typing `@` does not trigger typeahead
- Apply patch, retest, see typeahead.
Regression tested:
- Windows 10, Chrome, Firefox, Edge
- Mac OS, Chrome, Firefox, Safari
- Keyboard layouts, English, French, German, Spanish
All tests passed
Reviewers: benwick, epriestley
Reviewed By: epriestley
Subscribers: epriestley
Maniphest Tasks: T10252
Differential Revision: https://secure.phabricator.com/D18269
Summary: Additonal option to use newly made images in these views.
Test Plan:
Built an example in UIExamples.
{F5071682}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18299
Summary: We've never used this, and no current plans to.
Test Plan: grep for use cases.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18298
Summary: Adds dropdown carets to buttons more universally that are actually dropdowns.
Test Plan: Differential, Application Search, Diffusion. Mobile and Desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18292
Summary: Fixes T12930. I can't verify this fix, but the colors here work in light/dark mode correctly.
Test Plan: Wait for @cspeckmim to verify
Reviewers: epriestley, cspeckmim
Reviewed By: cspeckmim
Subscribers: cspeckmim, Korvin
Maniphest Tasks: T12930
Differential Revision: https://secure.phabricator.com/D18286
Summary: Fixes T12931. Adds a branch selector that's always visible if the repo has commits.
Test Plan:
Test a plain hg, svn, git repository. Test setting a bad default branch. Test a good default branch. Test on desktop, mobile layouts.
{F5058061}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12931
Differential Revision: https://secure.phabricator.com/D18267
Summary: Ref T12733. In this case, the server returns no new `row`, but we would incorrectly try to bind to one.
Test Plan:
- Created a new comment.
- Edited it.
- Deleted all the text.
- Saved changes.
- Before: Header continues to show phantom "1 Unsubmitted Comment", browser error log reflects one error.
- After: Header reflects comment being deleted, error log is quiet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18262
Summary: Moves default color back to black, increases line-height to clean new object borders
Test Plan: Review lots of remarkup in sb, this is same height as "document" CSS.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18249
Summary: Cleans up a bunch of Differential odd/special colors. Adds some basic "highlight" colors instead of pure yellow.
Test Plan: Test each color change in normal and dark modes.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18239
Summary: Mostly this is an exercise to clean up our CSS and Celerity processor by making sure all important color decisions are generatable. It's somewhat resonable to use if you don't review code. Posting it up here mostly so I don't lose the work.
Test Plan: Visit lots and lots of pages with dark mode on and off.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18227
Summary: Just some cleanup. Make sure action-bar has consistent space if locate is there or not, hide tabs if repository has no content. Use clone or checkout language depending on SCM. Fixes T12915.
Test Plan:
Test git, hg, svn blank states.
{F5042707}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12915
Differential Revision: https://secure.phabricator.com/D18208
Summary: First pass at providing a skeleton framework for laying out basic items in a left/right view. Will likely add some mobile-responsive options.
Test Plan: UIExamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18200
Summary: This moves actions into the Diffusion main header, removes the locate file box, and widens description and cloning details. Projects are not currently in this layout, but will follow up in another diff. Trying to keep these changes small and iterative.
Test Plan:
Locate some files, test actions dropdown, repository with and without description. Also tablet, mobile layouts.
{F5040026}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18193
Summary: Cleans up colors, removes commit hash and links the text instead. Also unsure how valuable "lint" column is here, but left it. I'd maybe like to understand that workflow since it just seems like clutter overall. Also Fixes T12905
Test Plan:
Review Phabricator, hg, and a few other test repositories locally. Holler if anything here seems bad, but this feels easier to read and use to me.
{F5038425}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12905
Differential Revision: https://secure.phabricator.com/D18189
Summary: The main change here is moving (compare, search, history) into buttons in the header bar on all browse views. This allows Directory Browsing to be full width, since there is no other curtain information. File, Image, LFS, Binary all stay in TwoColumn layouts with the same buttons in the header.
Test Plan: Test viewing a directory, file, image, binary file, readme, and fake a gitlfs.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17766
Summary: Builds out a responsive tab bar system for PHUITwoColumnView pages
Test Plan:
Tested at mobile, tablet, and desktop breakpoints
{F5012429}
{F5012430}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18148
Summary: Fixes T12850, marks it hidden with zero width so the element still exists in the DOM
Test Plan: T12850 code, locally
Reviewers: epriestley, amckinley, avivey
Reviewed By: avivey
Subscribers: Korvin
Maniphest Tasks: T12850
Differential Revision: https://secure.phabricator.com/D18130
Summary:
Fixes T8909. Ref T12733.
UI attempts to follow the mock, but is a bit rough since PHUIXButtonView without text in this menu gets weird spacing, we don't have circular buttons yet, and PHUIXActionView without an icon also gets odd spacing.
Test Plan: {F5003125}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733, T8909
Differential Revision: https://secure.phabricator.com/D18128
Summary:
Ref T12733. When a revision has unsubmitted checkmarks:
- Color the banner yellow.
- Show them in the "X unsubmitted" count.
- Make the "X unsubmitted" button cycle between all drafts (written but unpublished comments) and "draft done" (checked but unsubmitted "Done" checkbox comments).
Test Plan:
- Checked a "Done" box, saw "1 unsubmitted" and yellow banner.
- Clicked "5 unsubmitted" repeatedly, saw it cycle through all unsubmitted comments and checkboxes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18127
Summary:
Ref T12733. This paves the way for a separate "hide" operation which completely hides things.
(I didn't extend this to the server side because that would require schema changes and the new "hide" state is client-only.)
Test Plan: Collapsed and expanded inlines, viewed tooltips.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18126
Summary: Adds a new DiffusionBranchListView which replaces the BranchTable when browsing all branches in Diffusion. Has all the same capabilities, but is easier to read, adds a Compare button, and plays nicely on mobile. It does take up more space, but I think that's generally OK here since we expect our branches to not be heaping piles of intern revert branches.
Test Plan:
Follow a few repositories with branches, like Phabricator and KDE's Krita. View layouts on mobile, tablet, desktop. Try out new compare button.
{F4996207}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: avivey, Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18113
Summary: Adds spacing to the buttons, line-height for aligning text vertically.
Test Plan: Leave comments on a diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18110
Summary: Should be button-grey
Test Plan: Use object selector on a diff for a task
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18100
Summary: These are now unused.
Test Plan: grep, remove uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18090
Summary: This probably got nuked at some point, but the class is there so let's use it.
Test Plan: Browse a directory with 800 files. See pager change color.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18089
Summary: Updates to use the standard 13px size we use everywhere else, cleans up a few mobile/display bugs, adds a hover state for `tr`.
Test Plan:
Review Diffusion, Daemons, Almanac, People Logs, anything else?
{F4991070}
{F4991071}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18088
Summary: Padding is ridiculously large on mobile devices, shrink it.
Test Plan: Review in simulator.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18087
Summary: Little nits and spacing changes to viewing diffusion commit history on phones.
Test Plan:
Review in Chrome, iOS Simulator.
{F4990749}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18085
Summary: This is a little too agressive, it's meant to only color direct icons, not icons inside tags if the object item is disabled.
Test Plan: Check closed tasks and see icon colors inside tags, check a disabled project, see disabled color.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18084
Summary: Minor, adds some weightier checkbox styles for use in Remarkup.
Test Plan:
Test a task, Phriction, various remarkup list styles.
{F4990161}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: cspeckmim, Korvin
Differential Revision: https://secure.phabricator.com/D18080
Summary: Try to dis-ambiguate various button types and colors. Moves `simple` to `phui-button-simple` and moves colors to `button-color`.
Test Plan: Grep for buttons still inline, UIExamples, PHUIX, Herald, and Email Preferences.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18077
Summary: Ref M1476. Same as D18070 but that did most of the magic.
Test Plan: {F4987961}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18071
Summary: Formally support borderless tags in PHUITagView.
Test Plan: Used in Diffusion History List
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18074
Summary: Some unneeded CSS here.
Test Plan: Click on ToC button, see more normal colors.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18073
Summary: Saturate the color a little more, add yellow
Test Plan: uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18068
Summary:
- Add a simple green button... maybe don't need
- Fix tokenizer search icon
- Splite simple and button-bar into own files
Test Plan: uiexamples, various pages with buttons, diffusion
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18063
Summary: Ref T12779. Try a little harder to get the autoscroll heuristic right, but also just stick to anchors if the URL has an anchor and the scroll position is near that anchor.
Test Plan:
- Loaded an anchored diff at a bunch of window sizes, seemed pretty sticky.
- Added `usleep(100000 * mt_rand(1, 15))` to `ChangesetViewController` to make changesets load slowly and in random order, reloaded a bunch of times while scrolling around, things appeared reasonable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12779
Differential Revision: https://secure.phabricator.com/D18060
Summary: Ref rPf2fcafb40dde94ddf4ee22716fea74fca0334a64#38208, I think this is a more usable layout. Gets rid of clippy, audit. Adds back Differential link as tag, Build Status as button.
Test Plan: Faked data on this for Differential, Builds, should all work though. Test on real and fake repositories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18061
Summary: Let's buttons just be an icon, no pressure to also have text.
Test Plan: UIExamples, Search, Home, Policy Controls... Probably 99% of them.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18056
Summary: Ref T12780. I'd like 18,000 GitHub stars now please thank you
Test Plan: this feature is awful
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12780
Differential Revision: https://secure.phabricator.com/D18053
Summary: Ref T12780. Button styles are bleeding over here on the icon, restrict to .button classes
Test Plan: uiexamples.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12780
Differential Revision: https://secure.phabricator.com/D18055
Summary:
Ref T12733. Ref M1476. This adds `PHUIXButtonView`, for client-side button rendering.
It also adds a PHUIX example which renders the server and client versions of each component side-by-side so it's easier to see if they're messed up.
Test Plan: {F4984128}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18051
Summary: Ref M1476. I'm going to see if I can set up side-by-side "PHUI" vs "PHUIX" to make maintaining them a touch easier. Before doing that, nuke some really old UI examples that don't seem very useful.
Test Plan: Viewed UIExamples, saw fewer bad ones.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18049
Summary: Ref T12733. This adds classes for unsubmitted/unsaved changes, and lays some groundwork for additional buttons.
Test Plan:
- Added, edited, deleted comments.
- Saw bar background color update appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18045
Summary: Ref T12733. This ultimately conflicts with scrolling and took about two days to get reported as a bug/regression. See T12733 for a bunch of additional discussion. See T1026 for original discussion.
Test Plan:
- Left single-line and multi-line comments on desktop.
- Tapped to leave single-line comments on mobile.
- Dragged lines on mobile, got a scroll instead of a range comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18044
Summary: Ref T12733. Completely removes the objectives UI.
Test Plan:
- Grepped for `objective`, etc.
- Browsed revisions, no JS errors / broken stuff.
- (If I missed anything, it's likely to turn up in followup changes.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18043
Summary: This moves Diffusion History to use an easier to parse list view for commits and their (diff, audit, build) status. I left TableView around, which is used on a repositories home, and we can maybe add a "graph view" history back as another controller. Not sure what the real use is for that kind of feature though. I don't have Harbormaster set up locally so I could use another install to give this a run. I also expect to maybe not live with this UI as final, I like the UX, but the icons for indicating status don't really feel great to me, just OK.
Test Plan:
pull various repositories, check various history displays.
{F4980356}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18039
Summary: These are unfortunatly manually built so I missed them in testing circle view changes.
Test Plan: Test lightbox, conpherence, uiexamples
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18036
Summary: I think this is reasonable for my current use case, but stacking icons overally is pretty clunky.
Test Plan:
UIExamples
{F4978899}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18032
Summary: Adds some indentation and color. Ref T9868.
Test Plan: A long page with multiple indentation levels.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9868
Differential Revision: https://secure.phabricator.com/D18025
Summary: Makes this a bit more flexible and allow UI to take over `col-2` completely. Also cleaned up application search a little with tags
Test Plan: Review various pages, grep for callsites.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18021
Summary: Some of these are unused, defaults to a lighter color naturally.
Test Plan: uiexamples, grep, phriction
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18020
Summary:
Fixes T6906.
I found the code in `behavior-search-typeahead.js` that was throwing away the closedness-detction work done in `Prejab.js::transformDatasourceResults`. Modified it to re-add the correct class name to the `phabricator-main-search-typeahead-result` elements.
Then I found some CSS in `typeahead-browse.css` and completely flailed around until realizing that particular CSS only gets loaded when hitting the typeahead endpoint directly. Copied the relevant bit of CSS over to `main-menu-view.css` (but maybe it should be removed from `typeahead-browse.css`?).
This is my first JS/CSS change, so please don't assume I did anything right.
Test Plan: {F4975800}
Reviewers: #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: epriestley
Maniphest Tasks: T6906
Differential Revision: https://secure.phabricator.com/D18017
Summary: Gives the ability to hide a big long block of text in an ObjectListItem without cluttering the UI.
Test Plan:
Added a test case to UIExamples. Click on icon, see content. Click again, content go away.
{F4974153}
{F4974311}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18006
Summary: Going to play a bit with this layout (diffusion sans audit) and see how it feels on profile. Uses a user image, moves the commit hash (easily selectible) and separates commits by date.
Test Plan:
Review profiles with and without commits.
{F4973987}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18005
Summary: Grep for phui-tag-shade and verify we're no longer calling shade-color directly.
Test Plan: Search, workboard, story points, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17993
Summary: Adds a new tag type, starts to try to clean up the mess that are PHUITags
Test Plan:
Review UIExamples.
{F4972323}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17991
Summary: Ref T12733. When an inline is selected, make it stand out so you can see where you are in the document more clearly.
Test Plan: {F4968509}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17982
Summary: Ref T12733. Show a "reply" icon for replies, and make them stack directly under their parent.
Test Plan: {F4968500}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17981
Summary: Ref T12733. Shows a comment snippet when hovering inlines in the objective list.
Test Plan: {F4968490}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17980
Summary: Ref T12733. Currently, creating a new inline and then canceling it leaves a marker in the objective list. Instead, remove the marker.
Test Plan:
- Created an empty inline, cancelled. Created a non-empty inline, cancelled. No objective marker in either case.
- Created a new normal inline, objective marker.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17979
Summary:
Ref T12733. When creating a new comment, the objective could appear to far up in the scrollbar because we were anchoring it to an invisible row.
Instead, anchor to the "edit" row while editing.
Test Plan: Created a new comment at the very top of a file, saw "File, Star" icons instead of "Star, File".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17978
Summary:
Ref T12733.
- While editing a comment, show a pink star ({icon star, color=pink}) with a tooltip.
- Slight UI tweaks, including draft comments getting an indigo pencil ({icon pencil, color=indigo}).
Test Plan: {F4968470}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17977
Summary: Ref T12733. Currently, long filenames get cut off at 160px. Instead, don't cut them off.
Test Plan:
Before:
{F4968401}
After:
{F4968402}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17975
Summary:
Ref T12733. In the longer run I'd like to just push this out from the edge, but that currently gets us into trouble since we start bumping into content. On my system, the trackpad scrollbar also expands in size when moused over, so the minimum number of pixels we need to push it out is approximatley 15px. This hits body content and the persistent chat.
For now, just disable this element on trackpad systems.
Test Plan:
Disconnected all USB peripherals, quit and relaunched Safari, saw no objective list.
Reconnected mouse, relaunched Safari, saw objective list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17974
Summary: See D17955.
Test Plan: Loaded a revision, no longer saw annotations with prototypes off. Still saw annotations with prototypes on.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17983
Summary:
Ref T12733. Currently, we detect the changeset which is in the middle of the screen as the current changeset.
This doesn't always get us the most intuitive changeset, particularly after a navigation from the scroll objective list: when you jump to changeset "X", you'd tend to expect "X" to be shown in the header, but the //next// changeset may be shown if "X" is too short.
Instead, select the changeset near the top of the screen (spanning an invisible line slightly below the banner).
Test Plan: Scrolled and jumped through a document with long and short changesets, saw a more intuitive changeset selected by the banner.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17976
Summary:
Minor UI tweaks:
- Use the dynamic icon for each file (e.g., image, text), not a hard-coded icon.
- Render the path (less important) in grey and the filename (more important) in black.
Test Plan: {F4966176}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17957
Summary:
Add important objectives (like waygates and quest markers) to the minimap.
This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.
Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)
{F4966037}
Reviewers: chad, amckinley
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17955
Summary: Brings more UI tweaks to disabled objects, like projects/people. Also fixes a missing icon in projects.
Test Plan: Application search with people and projects that have disabled results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12732
Differential Revision: https://secure.phabricator.com/D17962
Summary: This allows adding of pinboard items to a timeline. I'm hoping we can get this in for Maniphest (Pholio, Cover Image) and Macro (because, Macro), but unsure how to scalably do this. Anyways, here's the front end.
Test Plan:
Make some fake timeline items in UIExamples, test mobile, tablet, and desktop breakpoints.
{F4965798}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17950
Summary:
Fixes T1591. This was removed long ago because it was a mess to implement and caused a bunch of weird issues, and also my tolerance for dealing with weird JS issues was much, much lower.
I have now survived the fires of JX.Scrollbar and would love to address 200 small nitpicks about obscure browser behaviors on Linux, so open the floodgates again.
A secondary goal here is to create room to add a global view state menu on the right, with 300 options like "hide all inlines", "hide done inlines", "hide collapsed inlines", "hide ghosts", "show ghosts", "enable filetree", "disable filetree", etc, etc. Not sure how much of this I'll actually do. I have one more experiment I want to try first.
Test Plan: {F4963294}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T1591
Differential Revision: https://secure.phabricator.com/D17945
Summary:
Ref T12616. This puts "h" back to collapse or expand the current file.
This removes some very complicated/messy code around following links in the table of contents and getting files auto-expanded. I suspect no one will miss this, but we can restore it if ayone notices.
Test Plan: Pressed "h" to collapse/expand a file. Also used the menu items.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17940
Summary:
Fixes T10563. If you have a thread like this:
```
> A
> B
> C
```
...and you reply to "B", we should put the new inline below "B".
We currently do when you reload the page, but the initial edit goes at the bottom always (below "C").
Test Plan: {F4963015}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10563
Differential Revision: https://secure.phabricator.com/D17938
Summary: Skips rendering of partial elements if no actions are present.
Test Plan: Tested on profile menu item page, maniphest curtain, phriction dropdown, and instance backups page (no actions at all).
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17931
Summary:
Fixes T11662. In the very obscure situation described in that task, quiet a JS console warning.
The actual edit operation appears to work correctly after changes elsewhere.
There aren't really any legitimate lines for us to highlight in this case so I'm just giving up rather than trying to do something approximate.
Test Plan:
- Wrote `long.txt`.
- Created revision.
- Added an inline near the bottom.
- Removed most of `long.txt`.
- Updated revsion.
- Replied to the ghost inline.
- Edited the reply to the ghost inline, worked.
- Hovered the reply to the ghost inline: no line highlight, but no errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11662
Differential Revision: https://secure.phabricator.com/D17930
Summary: Fixes T1026. Ref T12616. Allows drag-to-select on devices to add inlines on a range of lines, using dark magic that I copy/pasted from StackOverflow.
Test Plan: Left a comment on a range of lines on iPhone simulator.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T1026
Differential Revision: https://secure.phabricator.com/D17928
Summary: Ref T12616. This makes line range selection use the new code, and removes the remainder of the old "hover a line number" / "select a line range" code.
Test Plan: Hovered line numbers; selected line ranges.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17927
Summary:
Ref T11401. Fixes T5232. Ref T12616.
Partly, this moves more code over to the new stuff.
This also allows "r" to work if you have code selected (not just comments). If you "reply" to code, you start a new comment.
You can "R" a comment to quote it. This just starts a new comment normally if you "R" a block of code. This is sort of a power-user version of "quote" since it seems like it probably doesn't really make sense to put it in the UI ever (maybe).
With the new click-to-select, you can click + "R" to reply-with-quote.
Test Plan: Used "r" and "R" to reply to comments and code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T11401, T5232
Differential Revision: https://secure.phabricator.com/D17920
Summary:
Fixes T8047. Ref T12616. Fixes T9270. This moves the "hover" part of the hover/drag behavior to the new code, leaving the "drag" part for a followup change.
The new hover UI behaves properly with Quicksand (T8047) and the filetree (T9270).
Test Plan: Hovered over inlines, saw lines select properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T9270, T8047
Differential Revision: https://secure.phabricator.com/D17919
Summary: Ref T12616. The ability to do {nav Edit > Cancel > Undo} to get your text back on inlines got dropped during the conversion. Restore it.
Test Plan:
Created, replied, and edited inlines, typed text, then cancelled. Was able to undo.
Also undid normal deletion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17916
Summary:
Ref T12616. Fixes T11648. Currently, we snug up replies with a negative margin (from T10563) but this throws off the anchor highlighting.
Instead:
- Remove padding from these dolumns.
- Use margins on the stuff inside them instead.
- Less margins for replies.
- Less margins for collapsed comments.
- Show some text for collapsed comments.
Test Plan:
{F4960890}
{F4960891}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T11648
Differential Revision: https://secure.phabricator.com/D17913
Summary:
Fixes T11784. A lot of things are interacting here, but this probably gets slightly better results slightly more often?
Basically:
- When we load content, we try to keep the viewport "stable" on the page, so the page doesn't jump around like crazy.
- If you're near the top or bottom of the page, we try to stick to the top (e.g., reading the summary) or bottom (e.g., writing a comment).
- But, if you followed an anchor to a comment that's close to the bottom of the page, we might stick to the bottom intead of staying with the anchor.
Kind of do a better job by not sticking to the bottom if you have an anchor. This will get things wrong if you follow an anchor, scroll down, start writing a comment, etc. But this whole thing is a pile of guesses anyway.
Test Plan:
- Followed an anchor, saw non-sticky stabilization.
- Loaded the page normally, scrolled to the bottom real fast, saw sticky stabilization.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11784
Differential Revision: https://secure.phabricator.com/D17911
Summary:
Ref T12616. Fixes T12715. I suspect these are very rarely used. (I think you tried to get rid of them before but I pushed back since we couldn't really offer great alternatives at the time?)
Now that the code is in a better place:
- Click an inline's header (just the colored part) to select it with the keyboard selection cursor.
- Click again to deselect it.
- You can use "n" and "p" to jump to comments, so "click + n" is the same as the old "V" action.
- This also makes it easier to swap between keyboard and mouse workflows, since you can jump into things with the keyboard at any inline.
Also, make "Reply" render more consistently.
Test Plan:
- Did all that stuff, things seemed to work OK.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12715, T12616
Differential Revision: https://secure.phabricator.com/D17908
Summary:
Fixes T8130. Allows selected comments to be shown/hidden (with "q") or marked done/not-done (with "w").
(These key selections are because "qwer" are right next to each other on QWERTY keyboards, and now mean "hide, done, edit, reply".)
Also, allow "N" and "P" to do next/previous inline, including hidden inlines. This makes "q" to hide/show a little more powerful and a little easier to undo.
Test Plan: Used "q", "w", "N" and "P" to navigate and interact with comments.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8130
Differential Revision: https://secure.phabricator.com/D17906
Summary:
Ref T12634. Fixes T8131. Currently, most edit operations (edit, reply, collapse, mark done) lose the keyboard cursor state.
Instead, bind the state more tighlty to the inline object itself (instead of the rows which happen to be in the document), and then do a bit of recalculation to try to keep it selected across edits.
Test Plan:
- Used "n" to select an inline.
- Clicked "Done" checkbox.
- Pressed "n".
- Went to the next inline (previously: lost position in document).
- Behavior is also better for: edit, reply, collapse/expand.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12634, T8131
Differential Revision: https://secure.phabricator.com/D17905
Summary:
Ref T12634. Fixes T12633. These events allow the keyboard reticle to resize properly.
(I expect to possibly hide/disable the reticle in the future during edits, but at least make the behavior sensible for now.)
Test Plan:
- Used "n" to select a block.
- Clicked a line number in that block to start a new inline comment.
- Saw reticle resize properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12634, T12633
Differential Revision: https://secure.phabricator.com/D17904
