Summary:
Ref T12539. See PHI190. Currently, each Diff has a `revisionID`, but Revisions do not point at the current active diff. To find the active diff for a given revision, we need to issue a separate query.
Furthermore, this query is inefficient for bulk loads: if we have a lot of revisions, we end up querying for all diff IDs for all those revisions first, then selecting the largest ones and querying again to get the actual diff objects. This strategy could likely be optimized but the query is a mess in any case.
In several cases, it's useful to have the active diff PHID without needing to do a second query -- sometimes for convenience, and sometimes for performance.
T12539 is an example of such a case: it would be nice to refine the bucketing logic (which only depends on active diff PHIDs), but it feels bad to make the page heavier to do it.
For now, this is unused. I'll start using it to fix the bucketing issue, and then we can expand it gradually to address other performance/convenience issues.
Test Plan:
- Ran migrations, inspected database, saw sensible values.
- Created a new revision, saw a sensible database value.
- Updated an existing revision, saw database update properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12539
Differential Revision: https://secure.phabricator.com/D18756
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: See PHI177. Ref T12974. PonderQuestion was overlooked during the Ferret engine conversions.
Test Plan:
Ran migrations, searched for questions, got results:
{F5241185}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12974
Differential Revision: https://secure.phabricator.com/D18736
Summary:
Ref T13000. This adds support for tracking "common" ngrams, which occur in too many documents to be useful as part of the ngram index.
If an ngram is listed in the "common" table, it won't be written when indexing documents, or queried for when searching for them.
In this change, nothing actually writes to the "common" table. I'll start writing to the table in a followup change.
Specifically, I plan to do this:
- A new GC process updates the "common" table periodically, by writing ngrams which appear in more than X% of documents to it, for some value of X, if there are at least a minimum number of documents (maybe like 4,000).
- A new GC process deletes ngrams that have been added to the common table from the existing indexes.
Hopefully, this will pare down the ngrams index to something reasonable over time without requiring any manual tuning.
Test Plan:
- Ran some queries and indexes.
- Manually inserted ngrams `xxx` and `yyy` into the ngrams table, searched and indexed, saw them ignored as viable ngrams for search/index.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18672
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:
Ref T12987. I was focused on the RefCursor table and overlooked that we need some care on this key.
It's currently possible to run `bin/storage upgrade --no-adjust`, then start Phabricator, and end up with duplicate records in this table. If you try to run `bin/storage adjust` later, it will try to add the unique key but fail. This is unusual for normal installs (they usually do not use `--no-adjust`) but we do it in the cluster and I did this exact thing on `secure`.
Normally, to avoid this, when a new table with a unique key is introduced, we also add a migration to explicitly add that key.
This is mostly harmless in this case. Fix this mistake (force the table to contain only unique rows; add the key) and try using `LOCK TABLES` to make this atomic. If this doesn't cause problems we can use this in similar situations in the future.
The "alter table may unlock things" warning comes from here:
https://dev.mysql.com/doc/refman/5.7/en/lock-tables.html
It seems like it's fine to issue `UNLOCK TABLES` even if you don't have any locks, so I think this script should always do the right thing now, regardless of ALTER TABLE unlocking or not unlocking tables.
Test Plan: Ran `bin/storage upgrade -f`, saw table end up in the right state. I'll also check this on `secure`, where the starting state is a little messier.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12987
Differential Revision: https://secure.phabricator.com/D18623
Summary:
Ref T11823. This change isn't standalone, but prepares for the more involved code change by dropping obsolete columns from the RefCursor table and adding the unique key we need to prevent the ambiguous/duplicate refs issue.
This data was moved to the RefPosition table in D18612.
Test Plan: Ran storage upgrade. See next revision for more substantial testing of this change series.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18613
Summary:
Ref T11823. This populates the new RefPosition table based on the existing RefCursor table, and deletes now-duplicate rows in the RefCursor table so the next change can add a unique key.
This change is not standalone, and there need to be separate code updates. I have a rough version of that written, but this migration needs to happen first to test it.
I'll hold this whole series of changes until after the release cut and until the code is updated.
Test Plan: Ran migration, spot-checked database tables. Saw redundant rows remove and correct-looking rows populated into the new RefPosition table.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18612
Summary:
Ref T11823. Currently, we have a "RefCursor" table which stores rows like `<branch or tag name, commit it is pointing at>` with some more data.
Because Mercurial can have a single branch pointing at several different places, this table must allow multiple rows with the same branch or tag name.
Among other things, this means there isn't a single PHID which can be used to identify a branch name in a stable way. However, we have several UIs where we want to be able to do this.
Some specific examples where we run into trouble: in Mercurial, if there are 5 heads for "default", that means there are 5 phids. And currently, if someone deletes a branch, we lose the PHID for it. Instead, we'd rather retain it so the whole world doesn't break if you accidentally delete a branch and then fix it a little later.
(I'll likely hold this until the rest of the logic is fleshed out a little more in followup changes.)
Test Plan: Ran `bin/storage upgrade`, saw the table get created without warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18602
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:
Ref T12819. This is shipping, so issue upgrade guidance to instruct installs to rebuild the index.
Also generate a new `quickstart.sql` since we haven't regenerated in a bit and there's been a large amount of table churn fairly recently.
Test Plan: Ran `bin/storage upgrade`, saw guidance notification in UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18594
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: Ref T12819. More ferret engine support.
Test Plan: Indexed and searched commits and repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18572
Summary: Ref T12819. Support for Pholio.
Test Plan: Indexed and searched mocks.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18569
Summary: Ref T12819. Adds ferret engine support for Calendar events.
Test Plan: Indexed and queried calendar events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18568
Summary: Ref T12819. Adds Ferret engine support.
Test Plan: Indexed and searched for documents.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18567
Summary: Ref T12819. Adds support for projects.
Test Plan: Indexed and searched for projects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18566
Summary: Ref T12819. Mostly straightforward, with a couple of minor query modernization things.
Test Plan: Indexed and searched for posts and blogs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18565
Summary: Ref T12819. Same deal as before, but smaller diffs after D18559.
Test Plan: Indexed and searched for packages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18564
Summary: Ref T12819. Adds Ferret support to Passphrase.
Test Plan: Indexed credentials, searched for credentials.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18556
Summary: Ref T12819. Adds Ferret engine support to initiatives.
Test Plan: Indexed and searched for initiatives.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18555
Summary:
Ref T12819. Adds support for indexing user accounts so they appear in global fulltext results.
Also, always rank users ahead of other results.
Test Plan: Indexed users. Searched for a user, got that user.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18552
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: Ref T12819. Adds storage and indexing for the Ferret engine to Differential.
Test Plan: Ran `bin/search index D123 --force`, saw indexes appear in database. No UI/user impact yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18540
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:
Ref T12819. Ferret currently does substring search, but this is not the default mode users expect: when you search for the "RICO" act, you do not expect to find documents containing "apRICOt" even though "RICO" is a substring.
To support term search, index the corpus as a list of terms with puncutation removed and whitespace normalized so the engine can match against it.
Test Plan:
Ran `storage upgrade`, ran `search index`, saw sensible database results:
```
rawCorpus: This is the task description.
Hark! Whom'st'dve eaten this "food" shall surely ~perish~?? #blessed
normalCorpus: thi the task descript hark whom dve eaten food shall sure perish bless
termCorpus: This is the task description Hark Whom'st'dve eaten this food shall surely perish blessed
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18498
Summary:
Ref T12819. This addresses two issues:
- One practical issue is that right now, if you search for "dog cat", and they appear in different fields (for example, "dog" appears ONLY in the title, while "cat" appears ONLY in a comment) we won't find the document. This is somewhat rare -- usually, if "dog" appears in the title, it's also repeated in the description -- but I think clearly a bug. To attack this, start automatically creating a virtual "ALL" field with the full document text which we'll use as the primary thing we match against.
- For fields which may occur more than once -- today, only comments -- aggregate them all into one big "all of the text" row instead of writing one row per comment. This partly addresses the first point ("dog" in one comment and "cat" in a different comment won't be found) and partly makes some of the query gymnastics easier.
Test Plan:
Ran `bin/storage upgrade`, ran `bin/search index <Txxx>`, saw sensible corpus values in the database:
```
mysql> select * from maniphest_task_ffield\G
*************************** 1. row ***************************
id: 3
documentID: 1981
fieldKey: full
rawCorpus: This is the task title
This is the task description.
normalCorpus: thi the task titl
thi the task descript
*************************** 2. row ***************************
id: 4
documentID: 1981
fieldKey: titl
rawCorpus: This is the task title
normalCorpus: thi the task titl
*************************** 3. row ***************************
id: 5
documentID: 1981
fieldKey: body
rawCorpus: This is the task description.
normalCorpus: thi the task descript
3 rows in set (0.00 sec)
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18497
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:
Ref T12819. I gave this stuff a sweet code name because all the terms related to "fulltext" and "search" already mean 5 different things. It, uh, ferrets out documents for you?
I'm building this to work a lot like the existing ngram index, which seems to work pretty well. If this sticks, it will auto-resolve the join issue (in T12443) by letting us do the entire thing locally in a JOIN and thus dodge a lot of mess.
This index gets built alongside other indexes, but only shows up in the UI if you have prototypes enabled. If you do, it appears under the existing fulltext field in Maniphest. No existing functionality is affected or disrupted.
NOTE: The query engine half of this is still EXTREMELY primitive, and this probably performs worse than the existing field for now. If this doesn't show obvious signs of being awful on `secure` I'll improve that in followup changes.
Test Plan:
Indexed my tasks, ran some simple queries, got the results I wanted, even for queries "ko", "k", "v0.1".
{F5147746}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819, T12443
Differential Revision: https://secure.phabricator.com/D18484
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: Just deletes the view code until I have time to better plan this out, or just not ship.
Test Plan: Visit Phame post on public logged out page, view count doesnt cause transaction fatal.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18475
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:
Ref T12956. After this change, individual users will no longer be able to modify builtin queries on a user-by-user basis: they will always appear at the bottom of the list, under their personal queries, and can only be managed by administrators.
To support this, clean up the old rows which could be hanging around from before: delete any personal saved queries where the saved query is a builtin query.
To ease this transition, try to pin the query we're deleting //if// the user had reordered things to put it on top.
Test Plan:
- Ran the migration, saw no changes in the UI but fewer rows.
- Went back to `master`, reordered queries to put a builtin one on top.
- Ran the migration.
- Saw that builtin one drop to the bottom (since it can't be on top anymore) but be pinned, preserving the behavior of `/maniphest/`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12956
Differential Revision: https://secure.phabricator.com/D18464
Summary:
Ref T12956. Currently, when you visit `/maniphest/` (or any other ApplicationSearch application) we execute the first query in the list by default.
In T12956, I plan to make changes so that personal queries are always first, then global/builtin queries. Without changing the "default query" rule, this will make it harder to have, for example, some custom queries in Differential but still run a global query like "Active" by default. To make this work, you'd have to save a personal copy of the "Active" query, then put it at the top.
This feels a bit cumbersome and this rule is kind of implicit and a little weird anyway. To make this work a little better as we make changes here, add an explicit pinning action, like the one we have in Project ProfileMenus.
You can now explicitly choose a query to make default.
Test Plan:
- Browsed without pinning anything, saw normal behavior.
- Pinned queries, viewed `/maniphest/`, saw a non-initial query selected by default.
- Pinned a query, deleted it, nothing exploded.
{F5098484}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12956
Differential Revision: https://secure.phabricator.com/D18422
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: This adds a very very basic view count to Phame, so bloggers can get some idea which posts are more popular than others. Anything more than this I think should be Facts or Google Analytics.
Test Plan: Write a new post, see post count. Reload page, post count goes up. Archive post, post count stays the same.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18446
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:
Ref T2543. This updates and migrates the status change transactions:
- All storage now records the modern modular transaction ("differential.revision.status"), not the obsolete non-modular transaction ("differential:status").
- All storage now records the modern constants ("accepted"), not the obsolete numeric values ("2").
Test Plan:
- Selected all the relevant rows before/after migration, data looked sane.
- Browsed around, reviewed timelines, no changes after migration.
- Changed revision states, saw appropriate new transactions in the database and timeline rendering.
- Grepped for `differential:status`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18419
Summary:
Ref T2543. Rewrites all the storage to use constants.
Note that transactions still use legacy values, I'll migrate and update them separately.
Test Plan:
- Ran migration.
- Browsed around, changed revision states, viewed dashboard, etc.
- Selected `DISTINCT()` and `GROUP_CONCAT()` of the `status` field in the database, saw sane/expected before and after values.
- Verified that old Conduit methods still return numeric constants for compatibility.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18418
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: Ref T2543. This migrates existing saved queries so they use the right modern values for the new tokenizer control, introduced in D18393.
Test Plan:
- Saved a query with "Abandoned" selected as the status in the old "<select />", prior to D18393.
- Upgraded to D18393, which broke the query (it no longer selected any status filter).
- Ran the migration to fix things.
- Saw the query now execute with "Abandoned" selected in the tokenizer, preseving the original behavior accurately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18394
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: This is no longer needed after the chatbot was removed in D17756.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18378
Summary: Just a few more.
Test Plan: Edit Picture, see new image, choose image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18370
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: Felt like tinkering with Illustrator.
Test Plan: Edit Picture, pick new image.
Reviewers: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18301
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: Moves over some of the icons we build for SAAS that can be useful for projects to. Also make builtin list dynamic.
Test Plan: Edit a project image, select a cool sword.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18297
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: More pretty images.
Test Plan: Set a robot as image for security project. So pretty.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18191
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: Updates the builtin images, leaves the old choose... icons for now. I'd like to automate this based on icon when creating a project.
Test Plan: Visit edit picture page, pick a few. Purge cache, see new default image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18162
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: Updated Files, no more AA issue.
Test Plan: look
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18144
Summary: Had some made, they seem better than my attempt.
Test Plan: review in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: avivey, Korvin
Maniphest Tasks: T12859
Differential Revision: https://secure.phabricator.com/D18143
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: Fixes T12124. Changes `ManiphestEditEngine` to populate the select using priority keywords instead of the integer value. Marks `maniphest.querystatuses` as frozen. Adds a new Conduit method for fetching potential task statuses.
Test Plan: Created tasks and changed their priorities, observed that transactions in the DB still have the same type (integers as strings). Invoked `maniphest.update` with `priority => '90'` and observed that it still works. Invoked `maniphest.edit` with `priority => 'unbreak'` and observed that it now works.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T12124
Differential Revision: https://secure.phabricator.com/D18111
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: I think these are a hair nicer in the actual UI.
Test Plan: Revisit /diffusion/
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18118
Summary: Builds out some images to use to identify repositories. Fixes T12825.
Test Plan:
Try setting custom, built in, and null images.
{F4998175}
{F4998192}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12825
Differential Revision: https://secure.phabricator.com/D18116
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:
See D18037. The migration there may cause us to write new file records as a side effect.
Ideally, we would rewrite that migration to not ever have this kind of side effect. However, that would make it much more complicated, and it's already very complicated.
Instead, retroactively expand the size of this field before `storage adjust` does it, so it has the right size by the time we hit the migration in D18037.
Test Plan:
@chad, can you `arc patch` this and see if it works?
It's possible that it will get us about five lines deeper and then we'll just hit another similar exception, and that this isn't really a viable way forward.
Reviewers: chad, amckinley
Reviewed By: amckinley
Subscribers: amckinley, chad
Differential Revision: https://secure.phabricator.com/D18107
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: Does the UI work that's part of T12234 and adds migrations for both of the old-style duplicate transactions.
Test Plan:
- Started with a clean DB.
- Checked out really old code that marks tasks as dupes using comments.
- Made a bunch of tasks and closed some as dupes. Made a bunch of additional comments.
- Checked out D10427 and did a `storage upgrade`.
- Made a bunch more new tasks and dupes.
- Snapshotted DB.
- Ran migration repeatedly until all expected edges showed up in the `phabricator_maniphest.edge`table.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T12234
Differential Revision: https://secure.phabricator.com/D18037
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: Fixes T12505. `PhabricatorProjectsMembershipIndexEngineExtension->materializeProject()` was incorrectly bailing early for milestone objects, which prevented milestone members from being calculated correctly. This was causing problems where (for example) an Owners package owned by a milestone wasn't being satisfied when a member of the milestone approved a revision.
Test Plan: Invoked migration, observed that a user's milestones correctly showed up when searched for. Also observed that accepting a revision on behalf of a milestone now satisfies Owners rules.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12505
Differential Revision: https://secure.phabricator.com/D18033
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:
Ref T12738. This makes clicking "Throw In Trash" technically do something, sort of.
In Nuance, the default mode of operation for actions is asynchronous -- so you don't have to wait for a response from Twitter or GitHub after you mash the "send default reply tweet" / "close this pull request with a nice response" button and can move directly to the next item instead.
In the future, some operations will attempt to apply synchronously (e.g., local actions like "ignore this item forever"). This fakes our way through that for now.
There's also no connection to the action actually doing anything yet, but I'll probably rig that up next.
Test Plan: {F4975227}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18010
Summary: Ref T12738. Some of the Nuance "form" workflows currently fatal after work on the GitHub stuff. Try to make everything stop fataling, at least.
Test Plan: Using "Complaints Form" no longer fatals, and now lodges a complaint instead.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12738
Differential Revision: https://secure.phabricator.com/D18007
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: Also changes access modifiers on `PhabricatorProjectTransactionEditor` and sets up `storage` for `applyExternalEffects`.
Test Plan: Created new projects, attempted to create without name, with too long of a name, and with a name that conflicts with other projects and observed expected errors.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T12673
Differential Revision: https://secure.phabricator.com/D17947
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
Summary: Ref T12634. Fixes T10049. Toggling an inline currently leaves the reticle oddly-positioned.
Test Plan:
- Selected a comment with the keyboard.
- Collapsed it.
- Saw reticle behave reasonably.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12634, T10049
Differential Revision: https://secure.phabricator.com/D17903
