1. 14 May, 2020 7 commits
  2. 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
  3. 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
  4. 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
  5. 07 May, 2020 10 commits
  6. 06 May, 2020 1 commit
  7. 04 May, 2020 11 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
    • epriestley's avatar
      When loading a page with inlines, don't select/focus inlines which we... · 3a762480
      epriestley authored
      When loading a page with inlines, don't select/focus inlines which we immediately upgrade to "editing"
      
      Summary:
      Ref T13513. This is a bit clumsy, but the cleanest way to implement "isEditing" inlines today is to send them down as normal inlines and then simulate clicking "edit" on them.
      
      When we do, don't focus the resulting editor: focusing it makes the page scroll around and highlight things in essentially random order as the editors load in.
      
      Test Plan: Reloaded a page with some open editors, wasn't scrolled to them.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21217
      3a762480
    • epriestley's avatar
      Save drafts for inline comments currently being edited · fe501bd7
      epriestley authored
      Summary:
      Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.
      
      This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.
      
      Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21216
      fe501bd7
    • epriestley's avatar
      Don't consider empty inlines when considering whether a revision has draft comments or not · 27b7ba81
      epriestley authored
      Summary: Ref T13513. When computing whether a revision has draft comments or not, ignore empty inlines.
      
      Test Plan: Added empty inlines to a revision, no longer saw a yellow "draft" bubble in the list UI.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21215
      27b7ba81
    • epriestley's avatar
      When rendering changesets, discard empty draft inline comments · 9307f577
      epriestley authored
      Summary: Ref T13513. When you load a changeset, discard all empty inlines. This is likely a more desirable behavior than keeping empty editors around, even though the rest of the pipeline generally handles them fairly well now.
      
      Test Plan:
        - Started an inline, didn't type any text or save, reloaded page.
          - Before: page restores empty editor in the same place.
          - After: we just discard this likely-pointless empty inline.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21214
      9307f577
    • epriestley's avatar
      Refine unusual inline comment client interactions · 63bfad0f
      epriestley authored
      Summary: Ref T13513. Refine some inline behaviors, see test plan.
      
      Test Plan:
        - Edit a comment ("A"), type text ("AB"), cancel, edit.
          - Old behavior: edit and undo states (wrong, and undo does not function).
          - New behavior: edit state only.
        - Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
          - Old behavior: "AB" (wrong: you never submitted this text).
          - New behavior: "A".
        - Create a comment, type text, cancel.
          - Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
          - New behavior: no counter.
        - Cancel editing an empty comment with no text.
          - Old behavior: Something buggy -- undo, I think?
          - New behavior: it just vanishes (correct behavior).
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21212
      63bfad0f
    • epriestley's avatar
      Don't publish "empty" inline comments · f5ef341c
      epriestley authored
      Summary:
      Ref T13513. Currently, if you start an inline and then submit overall comments, we publish an empty inline. This is literally faithful to what you did, but almost certainly not the intent.
      
      Instead, simply ignore empty inlines at publishing time (and ignore "done" state changes for those comments).
      
      We could delete them outright, but if we do, they'll break if you have another window open with the empty inline (since the stored comment won't exist anymore). At least for now, leave them in place.
      
      Test Plan: Created empty inlines, submitted comments, no longer saw them publish.
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21211
      f5ef341c