Summary:
This commit adds a keydown listener to <input> elements to activate a flag when
Ctrl (and other keys) are pressed, which causes forms to be submitted to a new
tab.
This commit also modifies the click event listener for buttons to ignore
synthetic clicks from the browser, which is important as they clobber the
"new_tab" flag otherwise.
Closes T15914
Test Plan:
Open the Advanced Search form, and do Ctrl+Return inside one of the text boxes
to ensure that the result is opened in a new tab. Also do a plain Return, plain
click on "Search", and Ctrl+Click on Search to check for regressions.
Reviewers: O1 Blessed Committers, valerio.bozzolan, aklapper
Reviewed By: O1 Blessed Committers, valerio.bozzolan, aklapper
Subscribers: aklapper, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15914
Differential Revision: https://we.phorge.it/D25773
Summary:
Update the d3 library to its last 6.x version available on https://github.com/d3/d3/releases
This also requires updating the tooltip event handling of dots in `Chart.js` to avoid an `Uncaught TypeError: d3.event is undefined` per https://observablehq.com/@d3/d3v6-migration-guide#event-management linked from https://github.com/d3/d3/releases/tag/v6.0.0
Closes T15820
Test Plan:
* Enable the Facts application, go to the Reports of a Project with task changes over time, look at charts, hover over data points, read the tooltip - e.g. on http://phorge.localhost/project/reports/1/ or http://phorge.localhost/maniphest/report/burn/
* Check HTML source of above URIs for the `<script type="text/javascript">` loading `d3.min.js` and open the JS file to verify the d3 version number bump.
* Check Console of web browser's developer tools for no errors.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15820
Differential Revision: https://we.phorge.it/D25631
Summary:
Always render dates in tooltips when hovering over data points in Burndown charts etc of the Facts application as `YYYY-MM-DD` instead of `YYYY-M-D` by converting the integer to a string and if its length is only one character, prepend a zero.
Closes T15819
Test Plan:
* Enable the Facts application, go to the Reports of a Project with task changes over time, hover over data points and read the tooltip.
* Check Console of web browser's developer tools for no errors.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15819
Differential Revision: https://we.phorge.it/D25630
Summary:
This diff adds a copy button to every repo uri in the clone repo modal. I have made the button to select the text to a merely structural span before the input - it just shows the type of the repository uri. When you click inside the input, the entire uri will be selected. Also I have uncluttered the HTML structure. A table is not needed here, nothing a flex block can't handle.
| Before | After |
|-----------|-----------|
| {F1360344} | {F1368592} |
While at it, I have extended the used javascript copy behavior. First of all: `document.execCommand('copy')` [[ https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand | could stop working every moment in every browser ]]. The [[ https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Interact_with_the_clipboard | new clipboard API ]] is the way to go, so I have implemented it as the preferred method. The old method is kept as a fallback. And I have added a very nice feature: If defined, the behavior will now issue success or error notifications. See the changed UIExamples for that.
To support the shrinking of JS code with async functions I have patched the JsShrink source.
Test Plan: Go to a repository, hit the clone button and use the new copy button. You will see a shiny notification as a reward.
Reviewers: O1 Blessed Committers, avivey, valerio.bozzolan
Reviewed By: O1 Blessed Committers, avivey, valerio.bozzolan
Subscribers: avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25536
Summary:
Ref T15645
The very helpful "Locate File" input in Diffusion was so far only visible in the homepage route of any repository (`/repository`).
With this revision you can now locate a file from every browsed directory and in any selected commit.
The finder was already "directory sensitive" meaning: if you are trying to locate a file from within a browsed directory, only the children of this path will be searched.
For the searching in a specified commit (for example: https://we.phorge.it/source/phorge/browse/master/src/;05f4d5071fdca02123bd1ff4c0935b847c7f9963), I had to do a little JS magic adding the commit to the URI on the client side.
Test Plan: Checkout, browse through your repos with Diffusion trying to find files. (I tested only with Git repos.)
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15645
Differential Revision: https://we.phorge.it/D25521
Summary:
Before this change, if you search a file using Locate File,
this was the only accepted syntax for a full-path:
path/to/something.txt
After this change, some relative/absolute Unix-like variants
are also accepted:
./path/to/something.txt
/path/to/something.txt
Similar prefixes can be frequent when you quickly copy-paste things
from shell commands like 'grep' or 'find' etc.
Ref T15508
Test Plan:
Visit a lovely repository like Phorge and use Locate File with these:
1. src/applications/maniphest/editor/ManiphestEditEngine.php
2. /src/applications/maniphest/editor/ManiphestEditEngine.php
3. ./src/applications/maniphest/editor/ManiphestEditEngine.php
After this change, also 2. and 3. return the expected file.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15508
Differential Revision: https://we.phorge.it/D25320
Summary:
This change fixes the following recent JavaScript error:
Error: Empty ID passed to JX.$()!
The regression was introduced here:
90f9da643d
Closes T15272
Test Plan: - with this change I was able to open Conpherence Persistent Chat and Drop a file
Reviewers: O1 Blessed Committers, Dylsss, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: Dylsss, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15272
Differential Revision: https://we.phorge.it/D25134
Summary:
Add a new RemarkupMetadata class and use with upload button as well as drag and drop pathways.
With this change, files are now attached automatically to the object. This this could solve
several issues, for example when a person uploads a file but only that author is able to see it.
Ref T15106
Test Plan:
- Drag and drop file, upload file with button. Check that both files are attached with "attachedFilePHIDs" values.
- drop file in a Task description (now works)
- drop file in a Task comment (now works)
- drop file in an edited Task comment (still not supported)
- This was already tested in Wikimedia Foundation: https://phabricator.wikimedia.org/D1203
Reviewers: O1 Blessed Committers, avivey, Matthew
Reviewed By: O1 Blessed Committers, avivey, Matthew
Subscribers: MCPCN, avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Tags: #remarkup
Maniphest Tasks: T15106
Differential Revision: https://we.phorge.it/D25052
Summary:
The CTRL+click now opens the links in a new tab, like any other
normal link opened with CTRL+click.
Note that the middle-click was already working.
Closes T15157
Test Plan:
- visit a Workboard
- column > menu > mouse on "Create Task"
- CTRL+click: open in new tab (→ now works)
- normal click: open the pop-up (→ still works)
- middle-click: open in new tab (→ still works)
- column > single Task > mouse on "Edit"
- CTRL+click: open in new tab (→ now works)
- normal click: open the pop-up (→ still works)
- middle-click: open in new tab (→ still works)
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15157
Differential Revision: https://we.phorge.it/D25072
Summary:
Rest assured: external links remain evil, by default.
Don't adopt them randomly by induction.
Whether you believe it or not, this specific external
link merited some deep thoughts on Phorge:
- https://we.phorge.it/T15172
So, whenever you use a mouse, a finger, or whenever we have
a confirmation dialog or not to prevent onblur disasters,
this change is probably consistent with common expectations.
Having said, external links remain evil - by default.
Closes T15172
Test Plan:
- Drag & Drop a File on a Remarkup text
- click on the link inside the popup
- it opens in a new tab (without risk of form loss)
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15172
Differential Revision: https://we.phorge.it/D25077
Summary:
This modification improves a single link shown to the user.
The goal is to make it easier to change file permissions or
file name or whatever just after the upload of the file itself.
Before this change, doing that was difficult because clicking
on the popup would send you back to the binary file only.
Closes T15165
Test Plan:
- upload a file using drag & drop (for example in a comment)
- click on the popup's link
- you see the File page, not the binary file
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15165
Differential Revision: https://we.phorge.it/D25074
Summary:
Ref T13603. Allow "VersionedDraft" to persist remarkup comment area metadata from stacked actions controls.
When files are dragged and dropped, record them as explicit uploads in comment metadata.
Test Plan: Dragged and dropped files into Remarkup stacked action text areas (e.g., in Maniphest), reloaded page, saw metadata persist across reloads.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21828
Summary: Ref T13603. This will support explicit handling of attached files.
Test Plan: Adjusted new input to have "text" input type, used it alongside additional upcoming changes, saw sensible metadata behavior.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21827
Summary:
Ref T9530. Ref T13658. The "Releeph" application was never useful outside of Facebook and any application providing release support would not resemble it much.
It has some product name literal strings, so now is as good a time as any to get rid of it.
This application never left prototype and I'm not aware of any install in the wild that uses it (or has ever used it).
I did not destroy the database itself. I'll issue upgrade guidance and destroy the database in some future release, just in case.
Test Plan: Grepped for "releeph", found no relevant/removable hits.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658, T9530
Differential Revision: https://secure.phabricator.com/D21792
Summary: This adds the Ablillity to translate the Content of the Date Picker. It is possible to display more than one Letter for the Day Captions.
Test Plan:
- Change the Language in the User Preferences, make sure it translates 'S|M|T|W|T|F|S' and Months or add the following to the translation.override config to get German Translation:
```
{
'S|M|T|W|T|F|S': 'So|Mo|Di|Mi|Do|Fr|Sa',
'January': 'Januar',
'February': 'Februar',
'March': 'März',
'May': 'Mai',
'June': 'Juni',
'July': 'Juli',
'October': 'Oktober',
'December': 'Dezember'
}
```
- Open Datepicker on a Datefield (Edit Event or Edit for Custom Field of Type date)
- The Day Headers and the Months should now be translated
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: CSharp, 0, speck, tobiaswiese
Tags: #calendar
Differential Revision: https://we.phorge.it/D25016
Summary:
The datepicker could step by the wrong number of months, due to the date rolling over to the next month when the number of days in the month is exceeded. For example, going forward from January 31 would jump to March 3, while going backward from July 31 would only go to July 1.
Push the date back to ensure that the datepicker stays in the correct month when switching.
Test Plan: Changed months starting from an assortment of dates.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: artms, Korvin
Differential Revision: https://secure.phabricator.com/D21673
Summary: Ref T13559. Substantially correct the client logic for "Save" and "Cancel" actions to handle unusual cases.
Test Plan:
Quoting behavior:
- Quoted a comment.
- Cancelled the quoted comment without modifying anything.
- Reloaded page.
- Before changes: quoted comment still exists.
- After changes: quoted comment is deleted.
- Looked at comment count in header, saw consistent behavior (before: weird behavior).
Empty suggestion behavior:
- Created a new comment on a suggestable file.
- Clicked "Suggest Edit" to enable suggestions.
- Without making any text or suggestion changes, clicked "Save".
- Before changes: comment saves, but is empty.
- After changes: comment deletes itself without undo.
General behavior:
- Created and saved an empty comment (deletes itself).
- Created and saved a nonempty comment (saves as draft).
- Created and saved an empty comment with an edit suggestion (saves).
- Created and saved an empty comment with a suggestion to entirely delete lines -- that is, no suggestion text (saves).
- Edited a comment, saved without changes (save).
- Edited a comment, save deleting all text (saves -- note that this is intentionally without undo, since this is a lot of steps to do by accident).
- Cancel editing an unchanged comment (cancels without undo).
- Cancel editing a changed comment (cancels with undo).
- Undo'd, got text back.
- Cancel new comment with no text (deletes without undo).
- Cancel new comment with text (deletes with undo).
- Undo'd, got text back.
- Saved a quoted comment with no changes (saves -- note that this is intentionally not a "delete", since just quoting someone seems fine if you click "Save" -- maybe you want to come back to it later).
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21654
Summary:
Ref T13559. Various client decisions depend on the "initial" or "committed" states of inline comments. Previously, these were informally constructed from "mostly similar" available values, or glossed over in some cases.
On the server, save the initial state when creating a comment. Save the committed state when applying a "save" operation. Send all three states to the client.
On the client, load and track all three states explicitly.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21653
Summary:
Ref T13559. Currently, the default text for inline comment side-loads in a bizarre way. Instead, when a user creates an inline comment, load the inline context and set it as part of the initial content state.
This allows the side channel (and the code that puts the text in place at the last second on the client) to be removed.
Test Plan: Created inlines, clicked "Suggest Edit". See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21652
Summary:
Ref T13559. To allow the client to make correct decisions about what buttons mean, track an explicit "Committed" content state.
This is the last version of the comment that has been saved on the server, and does not exist if the comment has never been saved.
Test Plan: Created comments, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21651
Summary:
Ref T13559. When the user clicks the "Cancel" button, we sometimes take it to mean "delete" (when the comment is empty).
Both the client and server make a decision about this, and they may not agree, which causes the client to fall out of sync.
Make the client responsible for deciding whether it wants to interpret a click on the "Cancel" button as a "revert" or a "delete".
Test Plan: Cancelled empty and nonempty comments, etc. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21649
Summary:
Ref T13559. When you click "Save" on an inline comment and it's empty, we may actually delete the comment.
Currently, the client and server both make decisions about whether the comment should be deleted. These decisions may not agree, causing the client state to fall out of sync.
Make the client authoritative about whether it wants to handle the user clicking the "Save" button as an intent to save or an intent to delete.
Test Plan: Saved empty and nonempty inlines. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21648
Summary:
Ref T13559. Instead of directly reading form state, make all callers use the "active" state instead. The state reads the form.
No functional changes, just clarifying responsiblites.
Test Plan: Created inlines, etc. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21647
Summary:
Ref T13559. Rather than reading from the document, make client inlines actively track their current "active" state.
The "active" state is what the user currently sees in the client UI.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21646
Summary:
Ref T13559. In an effort to ultimately fix the "quote + cancel" bug, begin formalizing content states on the client.
This creates a "ContentState" client object and moves the authoritative storage for the "hasSuggestion" property to it.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21645
Summary:
Ref T13559. D21261 added caching here, but the logic in rebuilding inlines wasn't quite correct, and could lead to us double-appending.
Instead, when rebuilding, unconditionally discard the old list.
Test Plan:
- Added inline comments to a file in Differential.
- Marked some done.
- Scrolled so the inline comment header was visible, saw "X / Y Comments" button in header.
- Clicked "Show 20 more lines" on the changeset with inlines (or toggle "View Unified" / "View Side-by-Side", or other interactions likely work too).
- Before: saw "X / Y" change improperly (because inlines in that file were double-counted).
- After: saw stable count.
- Grepped for "differential-inline-comment-refresh", got no hits, concluded this event has no listeners.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21642
Summary:
Ref T13644. Ref T13638.
- Double-encode the symbol that is used as a path component, similar to Diffusion.
- Fix an outdated reference to ".path", which provided context for symbol lookup.
- Prevent command-clicking headers from looking up the path as a symbol.
Test Plan:
- Command-clicked headers, no longer got a symbol.
- Command-clicked stuff with "/", saw it double-encoded and decoded properly.
- Command-clicked normal symbols, saw "path" populate correctly.
Maniphest Tasks: T13644, T13638
Differential Revision: https://secure.phabricator.com/D21641
Summary:
Ref T13629.
- Allow files to have custom alt text.
- If a file doesn't have alt text, try to generate a plausible default alt text with the information we have.
Test Plan:
- Viewed image files in DocumentEngine diffs, files, `{Fxxx}` embeds, and lightboxes.
- Saw default alt text in all cases, or custom alt text if provided.
- Set, modified, and removed file alt text. Viewed timeline and feed.
- Pulled alt text with "conduit.search".
Maniphest Tasks: T13629
Differential Revision: https://secure.phabricator.com/D21596
Summary:
Ref T13602. When rendering a user hovercard, pass the object on which the reference appears. If the user can't see the object, make it clear on the hovecard.
Restyle the "nopermission" markup in mentions to make it more obvious what the style means: instead of grey text, use red with an explicit icon.
Test Plan: {F8430398}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21554
Summary:
Ref T13602. Currently, Hovercards are functions only of the object they represent (and the viewer, etc).
Recent changes to how users who can't see an object are rendered motivate making them a function of both the object they represent //and// the context in which they are being viewed. In particular, this enables a hovecard for a user to explain "This user can't see the thing you're lookign at right now.", so visual "exiled" markers can have a path forward toward discovery.
Test Plan:
- This change isn't expected to affect any behavior.
- Viewed hovercards, moused over/out, resized windows, viewed standalone cards, viewed debug cards, saw no behavioral changes.
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21553
Summary:
See PHI1900. Recent changes to how commit graphs are drawn made the height automatic in most cases, but it fails in Differential because the element isn't initially visible so the computed height is 0.
Just give them an explicit height so they show up again.
Test Plan: Viewed graphs in Maniphest, Differential, and Diffusion; saw them all render properly.
Differential Revision: https://secure.phabricator.com/D21481
Summary:
See PHI1898. An install is reporting an execution/initialization order issue where this code is reachable before `_inlines` is initialized.
I can't immediately reproduce it, but using "getInlines()" is preferable anyway and seems likely to fix the problem.
Test Plan: Viewed revisions with inlines, added/removed/edited/replied to inlines, didn't find anything broken.
Differential Revision: https://secure.phabricator.com/D21475
Summary:
Ref T13552. Build the "commit list" elements so that the menu action items collapse under the element on mobile.
Also change the mobile breakpoint to 512px because my Safari window can't go any narrower than 508px. Future changes to responsive design will be more content-aware anyway.
Test Plan: Looked at commits in various interfaces, at desktop and mobile widths.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21430
Summary: Ref T13552. In the new combined "table/list" graph view, tidy up the graph rendering.
Test Plan: {F7633504}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21411
Summary: See PHI1753. This condition got rewritten for suggested edits and accidentally inverted.
Test Plan:
- Create a comment, type text, save draft, edit comment, cancel.
- Before: comment hides itself.
- After: comment properly cancels into pre-edit draft state.
Differential Revision: https://secure.phabricator.com/D21286
Summary: Ref T13513. When a revision has inlines with edit suggestions, pressing "j" and "k" can incorrectly select the blocks inside the diffs inside the inlines.
Test Plan: Used "j" to cycle through changes in a revision with inline comments with edit suggestions, didn't get jumped into the suggestion diffs.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21283
Summary:
Ref PHI1749. Instead of opening files to the last unchanged line on either side of the change, open files to the "simple" line number of the selected block.
For inlines, this is the inline line number.
For blocks, this is the first new-file line number, or the first old-file line number if no new-file line number exists in the block.
This may not always be what the user is hoping for (we can't know what the state of their working copy is) but should produce more obvious behavior.
Test Plan:
- In Diffusion, used "Open in Editor" with and without line selections. Saw same behavior as before.
- Used "n" and "r" to leave an inline with the keyboard, saw same behavior as before.
- Used "\" and "Open in Editor" menu item to open a file with:
- Nothing selected or changeset selected (line: 1).
- An inline selected (line: inline line).
- A block selected (line: first line in block, per above).
Differential Revision: https://secure.phabricator.com/D21282
Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.
Test Plan:
- Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
- Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21278
Summary:
Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work.
Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source.
Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21276
Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.
Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21275
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
Summary: Ref T13513. The code which added "r" and "R" to the inline menu accidentally discarded the difference between the keystrokes.
Test Plan:
- Clicked an inline, pressed "r", got new empty inline (previously: inline with quote).
- Clicked an inline, pressed "R", got a new quoted inline.
- Repeated steps with the menu items, got the expected behaviors.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21268
Summary: Ref T13513. On `secure`, I caught an issue where inlines may be accessed directly before they're constructed. Instead, access them through the relevant accessor.
Test Plan: Will deploy.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21263
Summary:
Ref T13513. The on-hover-inline reticle has switched over to have cell-based behavior. Switch the on-hover-line-number reticle to use the same behavior.
Also, clean up the dirty/redraw loop slightly: we no longer need to dirty on resize, and we don't need to redraw if the range isn't actually dirty.
Test Plan: Highlighted lines and line ranges. Hovered over inlines.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21262
Summary:
Ref T13513. See PHI1734, which raises a concern about the performance of large revisions near the 100-change threshold.
Currently, `getInlines()` is called whenever the scroll position transitions between two changesets, and it performs a relatively complicated DOM scan to lift inlines out of the document.
This shows up as taking a small but nontrivial amount of time in Firefox profiles and should be safely memoizable.
Test Plan:
- Under Firefox profiling, scrolled through a large revision.
- Before change: `getInlines()` appeared as the highest-cost thing we're explicitly doing on profiles.
- After change: `getInlines()` was no longer meaningfully represented on profiles.
- Created inlines, edited inlines, etc. Didn't identify any broken behavior.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21261
Summary:
Ref T13513. The way I'm highlighting lines won't work for Jupyter notebooks or other complex content blocks, and I don't see an obvious way to make it work that's reasonably robust.
However, we can just ignore the range behavior for complex content and treat the entire block as selected. This isn't quite as fancy as the source behavior, but pretty good.
Also, adjust unified diff behavior to work correctly with highlighting and range selection.
Test Plan:
- Used range selection in a Jupyter notebook, got reasonable behavior (range is treated as "entire block").
- Used range selection in a unified diff, got equivalent behavior to 2-up diffs.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21257
Summary:
Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state.
Also fix a couple of minor issues with select interactions and offset comments.
Test Plan: Selected inlines, saw obvious visual cues.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21256
Summary: Ref T13513. Currently, clicking inline or changeset headers eats the click events. This doesn't serve any clear purpose, and means these clicks do not clear text selections from the document, which is unusual.
Test Plan:
- Selected some text in a diff.
- Clicked a changeset header to select it.
- Before patch: text selection and context menu were retained.
- After patch: text selection was cleared and context menu was dismissed.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21255
Summary:
Ref T13513. In Safari, do this:
- view a 2-up diff with content on both sides;
- select more than one line on the right side; and
- use your mouse to click "New Inline Comment" in the context menu that pops up.
The mousedown event for the "New Inline Comment" click removes the "copy selection" behavior and creates a flash where both sides of the diff are selected.
This doesn't happen with (most) normal clicks because mouse down on a non-grabbable element clears the document selection.
To avoid this, don't reset the copy selection behavior if the user mouses down on an "<a />". This might not be robust, but seems simple and plausible as a fix.
Test Plan:
- See above.
- Before patch: flash of overbroad selection when clicking "New Inline Comment".
- After patch: no selection flash.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21254
