1. 15 May, 2020 2 commits
    • epriestley's avatar
      Reduce the frequency of DOM scans to rebuild inlines when scrolling revisions · c666cb9f
      epriestley authored
      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
      c666cb9f
    • epriestley's avatar
      Remove code which overrides "diffusion.ssh-username" when instanced · b1351d0f
      epriestley authored
      Summary:
      Ref T13529. Now that instances can be renamed, an instance may have multiple valid SSH usernames and the preferred SSH username may not be the intenal instance name.
      
      `PhacilitySiteSource` should already always set `diffusion.ssh-username` correctly, to the current preferred SSH username (which may be "new-name" after a rename from "old-name"), so we should never be able to reach this code without an accurate `diffusion.ssh-username` value available.
      
      The code to resolve names into instances also already works for both "ssh old-name@..." and "ssh new-name@...".
      
      So I believe this code has no beneficial effects and only causes harm: it may force us to return "old-name" when falling through would correctly return "new-name".
      
      Test Plan:
        - Previously: renamed an instance, then SSH'd to it using both the old and new names. Both work.
        - Previously: verified that `diffusion.ssh-username` is set correctly after a rename.
        - Verified that Diffusion "Clone" UI now shows "new-name" after an instance rename.
        - The real question here is: does this break something I'm not thinking of? And the change probably has to go to production to answer that.
      
      Maniphest Tasks: T13529
      
      Differential Revision: https://secure.phabricator.com/D21259
      b1351d0f
  2. 14 May, 2020 11 commits
    • epriestley's avatar
      Improve offset/range inline behavior for rich diffs and unified diffs · 3ee6b539
      epriestley authored
      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
      3ee6b539
    • epriestley's avatar
      Give selected inline comments are more obvious selected state · fbd57ad8
      epriestley authored
      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
      fbd57ad8
    • epriestley's avatar
      When users click headers to select diff UI elements, don't eat the events · b021da71
      epriestley authored
      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
      b021da71
    • epriestley's avatar
      Fix a flash of document selection when "oncopy" and "inline on range" behaviors interact · 42f1a95a
      epriestley authored
      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
      42f1a95a
    • epriestley's avatar
      When cancelling an inline comment edit, exit the edit state after the response arrives · f45519c0
      epriestley authored
      Summary: Ref T13513. This fixes a bug where clicking a line number, then clicking "Cancel" causes the paths panel to briefly update with an extra inline comment counted on the file.
      
      Test Plan:
        - Clicked a line number.
        - Typed some text.
        - Clicked "Cancel".
        - Before patch: paths panel flashes "1".
        - After patch: paths panel stays stable.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21253
      f45519c0
    • epriestley's avatar
      Distinguish more carefully between "null" inline offsets and "0" inline offsets · cfb5de6f
      epriestley authored
      Summary:
      Ref T13513. Currently, when creating an inline by selecting a line range, slightly careless handling leads to an inline with "0" offsets (by passing "undefined" to the server). This causes the block to highlight every line except the last one as fully bright, which is incorrect.
      
      An inline with "0" offsets and an inline with no offsets are different. Be more careful about passing offsets around and rendering them.
      
      Test Plan:
        - Used the line numbers to add an inline to lines 4-8 of a change.
        - Hovered the inline.
        - Saw all four lines marked as "dull"-highlighted (previously: three bright lines, one dull line).
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21252
      cfb5de6f
    • epriestley's avatar
      Store inline comment offset information and show it when highlighting comments · 2f539879
      epriestley authored
      Summary:
      Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.
      
      When hovering the comment, highlight the original range.
      
      Test Plan: {F7480926, size=full}
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21250
      2f539879
    • epriestley's avatar
      Improve select-to-comment behavior in Firefox and on unified diffs · ebef22cc
      epriestley authored
      Summary:
      Ref T13513.
      
        - Firefox represents multiple selected rows as a discontinuous range. Accommodate this.
        - Unified diffs don't have a "copy" marker. Do something sort-of-reasonable for them.
      
      Test Plan:
        - Selected multiple lines of content in Firefox, got an option to add a comment.
        - Selected content in unified mode, got an option to add a comment.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21249
      ebef22cc
    • epriestley's avatar
      Allow users to create inline comments by directly selecting text directly · 42378ea3
      epriestley authored
      Summary: Ref T13513. Support direct text selection for inlines. This is currently just an alternate way to get to the same place as using line numbers, but can preserve offset/range information in the future.
      
      Test Plan:
        - Selected some text, hit "c", clicked "New Inline Comment", got sensible comments on both sides of a diff in Safari, Chrome, and (with limitations) Firefox.
        - Caveats: no unified support, doesn't work across lines in Firefox.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21248
      42378ea3
    • epriestley's avatar
      Add "View Raw Remarkup" to inline comments · c063e0e5
      epriestley authored
      Summary: Ref T13513. Ref T11401. Support viewing raw remarkup for inlines.
      
      Test Plan: Viewed raw remarkup on inlines.
      
      Maniphest Tasks: T13513, T11401
      
      Differential Revision: https://secure.phabricator.com/D21246
      c063e0e5
    • epriestley's avatar
      Move inline comment actions into a dropdown menu · 419b7cee
      epriestley authored
      Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.
      
      Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).
      
      Maniphest Tasks: T13513, T11401
      
      Differential Revision: https://secure.phabricator.com/D21244
      419b7cee
  3. 13 May, 2020 2 commits
    • epriestley's avatar
      Improve line breaking behavior in Firefox and Chrome under complex conditions · 1da54837
      epriestley authored
      Summary: See <https://github.com/phacility/phabricator/pull/854>. In some situations, `line-break: anywhere` produces better behavior than `word-break: break-all`. It never appears to produce worse behavior.
      
      Test Plan:
        - Break behavior changes if a line contains "<span />" elements caused by syntax highlighting. This CSS adjustment only appears to apply to text with internal "<span />" elements.
        - This specifically impacts certain internal breakpoints adjacent to punctuation, so the test case is highly specific. Generic test cases with latin word characters do not evidence any behavioral changes.
        - This change appears to have no impact on Safari, which uses the better behavior in all cases.
        - Before Patch: In Firefox and Chrome, this specific change breaks awkwardly. There is more room for text to fit on the broken line:
      
      Firefox
      
      {F7480567}
      
      Chrome
      
      {F7480568}
      
        - After Patch: Firefox and Chrome break the line better. Here's Firefox:
      
      {F7480569}
      
        - Additional context:
      
      Safari Behavior (Unchanged)
      
      {F7480570}
      
      Chrome with no highlighting (desirable behavior). Firefox does the same thing.
      
      {F7480571}
      
      Also tested other cases, which seem never-worse in any browser.
      
      {F7480574}
      
      Differential Revision: https://secure.phabricator.com/D21247
      1da54837
    • epriestley's avatar
      Fix an issue where passphrase-protected private keys were stored without discarding passphrases · 3dea9208
      epriestley authored
      Summary:
      Ref T13454. See <https://discourse.phabricator-community.org/t/newly-created-ssh-private-keys-with-passphrase-not-working-anymore/3883>.
      
      After changes to distinguish between invalid and passphrase-protected keys, SSH private key management code incorrectly uses "-y ..." ("print public key") when it means "-p ..." ("modify input file, removing passphrase"). This results in the command having no effect, and Passphrase stores the raw input credential, not the stripped version.
      
      We can't recover the keys because we don't store the passphrase, so no migration here is really possible. (We could add more code to detect this case, but it's presumably rare.)
      
      Also, correct the behavior of the "Show Public Key" action: this is available for users who can see the credential and does not require edit permission.
      
      Test Plan:
        - Created a new credential with a passphrase, then showed the public key.
      
      Maniphest Tasks: T13006, T13454
      
      Differential Revision: https://secure.phabricator.com/D21245
      3dea9208
  4. 12 May, 2020 4 commits
    • epriestley's avatar
      Render proper "Show Context" links in DocumentEngine diffs, not just bullets · df139f04
      epriestley authored
      Summary:
      Ref T13513. Currently, viewing a Jupyter document, hidden context just gets a plain "* * *" facade with no way to expand it.
      
      Support click-to-expand, like source changes.
      
      Test Plan:
        - Clicked to expand various Jupyter diffs.
        - Clicked to expand normal source changes.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21243
      df139f04
    • epriestley's avatar
      When an inline was left on a rendered DocumentEngine document, don't include an email context patch · e8109e4a
      epriestley authored
      Summary:
      Ref T13513. If you leave an inline on line 20 of a Jupyter document, we currently render context around *raw* line 20, which is inevitably some unrelated piece of JSON.
      
      Instead, drop this context. (Ideal behavior would be to render context around Jupyter block 20, but that's a whole lot of work.)
      
      Test Plan:
        - On Jupyter changes and normal source changes, made and submitted inline comments, then viewed text and HTML mail.
        - Saw no context on Jupyter comments (instead of bad context), and unchanged behavior (useful context) on normal source changes.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21242
      e8109e4a
    • epriestley's avatar
      Make "View as Document Type..." only show valid options · acc1fa16
      epriestley authored
      Summary:
      Ref T13513. Currently, "View as Document Type..." lists every available engine.
      
      This is hard to get completely right because we can't always rebuild the document ref accurately in the endpoint, but try harder to fake something reasonable.
      
      Test Plan: Used "View as Document Type..." on Jupyter notebooks, was given "Jupyter" and "Source" as options.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21241
      acc1fa16
    • epriestley's avatar
      When creating an inline, save the current document engine · 0cca40db
      epriestley authored
      Summary:
      Ref T13513. As part of inline metadata, save the document engine the change is being rendered with.
      
      This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else.
      
      The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks.
      
      Test Plan:
        - Created inlines and replies on normal soure code, saw no document engine annotated in the database.
        - Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database.
        - Swapped document engines between Jupyter and Source, etc.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21240
      0cca40db
  5. 08 May, 2020 5 commits
    • epriestley's avatar
      Fix an issue where storage inlines are fed to InlineAdjustmentEngine · 6dc20d1e
      epriestley authored
      Summary:
      Ref T13513. If an intradiff has at least one unchanged file ("hasSameEffectAs()") or more than 100 files ("Large Change"), we hit this block and don't upcast storage inlines to runtime inlines. I missed this in testing.
      
      Add the conversion step.
      
      Test Plan: Viewed an intradiff with at least one unchanged file and at least one inline comment, saw correct rendering instead of fatal.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21239
      6dc20d1e
    • epriestley's avatar
      Make "Delete" from inline comment previews function correctly while editing comments · e7ebd5d9
      epriestley authored
      Summary: Ref T13513. Currently, if you're editing a comment, "delete" doesn't put the comment into the correct state. This action is normally only reachable from comment previews, since an editing inline has no "delete" button.
      
      Test Plan:
        - Started editing an inline, clicked "Delete", got a deletion.
        - Created an inline, typed text,
        - Deleted a normal comment via preview.
        - Deleted a normal comment via the on-inline action.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21238
      e7ebd5d9
    • epriestley's avatar
      Make "View" from inline comment previews correctly jump to "isEditing" inlines · b804e8cf
      epriestley authored
      Summary:
      Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.
      
      Update this behavior so it works on inlines in either "Viewing" or "Editing" states.
      
      Test Plan:
        - Clicked "View" on a normal inline, got jumped/selected.
        - Clicked "View" on an editing inline, got jumped/selected.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21237
      b804e8cf
    • epriestley's avatar
      Persist "Show Changeset" and improve path text selection · 24ba66f1
      epriestley authored
      Summary:
      Ref T13513. Currently:
      
        - If you click the "Show Changeset" button, your state change doesn't actually get saved on the server.
        - It's hard to select a changeset path name for copy/paste because the "highlight the header" code tends to eat the event.
      
      Instead: persist the former event; make the actual path text not be part of the highlight hitbox.
      
      Test Plan:
        - Clicked "Show Changeset", reloaded, saw changeset visibility persisted.
        - Selected changeset path text without issues.
        - Clicked non-text header area to select/deselect changesets.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21236
      24ba66f1
    • epriestley's avatar
      Lift inline comment draft behaviors to "InlineController" · fa2d30ee
      epriestley authored
      Summary:
      Ref T13513. Currently, if you:
      
        - click a line to create an inline;
        - type some text;
        - wait a moment; and
        - close the page.
      
      ...you don't get an "Unsubmitted Draft" marker in the revision list.
      
      Lift all the draft behavior to "InlineController" and make saving a draft dirty the overall container draft state.
      
      Test Plan:
        - Took the steps described above, got a draft state marker.
        - Created, edited, submitted, etc., inlines in Diffusion and Differential.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21235
      fa2d30ee
  6. 07 May, 2020 10 commits
  7. 06 May, 2020 1 commit
  8. 04 May, 2020 5 commits
    • epriestley's avatar
      Fix an issue where non-ID changeset state keys were used as changeset IDs · a590db28
      epriestley authored
      Summary:
      Ref T13519. This is a little fuzzy, but I think the workflow here is:
      
        - View an intradiff, generating an ephemeral comparison changeset with no changeset ID. This produces a state key of "*".
        - Apply "hidden" state changes to the changeset.
        - View some other intradiff and/or diff view.
        - The code attempts to use "*" as a changset ID?
      
      I'm not entirely sure this is accurate; this was observed in production and I couldn't get a clean reproduction case locally.
      
      Optimistically, try making changeset IDs explicit rather than relying on state keys to be "usually changeset-ID-like".
      
      Test Plan: Used "hidden" locally across multiple intradiffs, but I wasn't cleanly able to reproduce the initial issue.
      
      Maniphest Tasks: T13519
      
      Differential Revision: https://secure.phabricator.com/D21223
      a590db28
    • epriestley's avatar
      Fix a JS issue when the anchor element on a page has no container · 6b691029
      epriestley authored
      Summary: See D21213. If there's no matching element, `findAbove()` throws. Handle these cases correctly.
      
      Test Plan: Visited `#toc` on a revision, no longer saw a JS error.
      
      Differential Revision: https://secure.phabricator.com/D21222
      6b691029
    • epriestley's avatar
      Fix an intradiff error when the newer changeset does not exist · 6430d6d6
      epriestley authored
      Summary: Ref T13523. If a file hasn't been touched in the newer changeset, we can currently hit an error in the interdiff.
      
      Test Plan:
        - Touched "moo.txt" in Diff 1.
        - Reverted the changes to "moo.txt" in Diff 2.
        - Diffed 2 vs 1.
        - Before patch: fatal (call to getFilename() on null).
        - After patch: clean interdiff.
      
      Maniphest Tasks: T13523
      
      Differential Revision: https://secure.phabricator.com/D21220
      6430d6d6
    • epriestley's avatar
      When cancelling an unsaved editing inline after a reload, don't cancel into an empty state · 07e160bd
      epriestley authored
      Summary:
      Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.
      
      Instead, provide the text when the editor opens. This fixes a cancel interaction.
      
      Test Plan:
        - Create an inline, type text, don't save.
        - Reload page.
        - Cancel.
        - Before: cancelled into empty state.
        - After: cancelled into deleted+undo state.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21219
      07e160bd
    • epriestley's avatar
      When a user submits "isEditing" inlines and chooses to publish them, publish... · 7fa47408
      epriestley authored
      When a user submits "isEditing" inlines and chooses to publish them, publish their current draft state as-shown
      
      Summary: Ref T13513. When users choose to publish inlines, we want to publish the visible text, not the last "checkpointed" state.
      
      Test Plan:
        - Created an inline ("AAA").
        - Edited it into "BBB", did not save.
        - Submitted.
        - Confirmed that I want to publish the unsaved inline.
        - Saw "BBB" publish.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21218
      7fa47408