1. 22 Jun, 2020 7 commits
  2. 22 May, 2020 2 commits
  3. 21 May, 2020 2 commits
    • epriestley's avatar
      Prevent keyboard selection of change blocks inside edit suggestions · 87fb35ab
      epriestley authored
      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
      87fb35ab
    • epriestley's avatar
      Make "Open in Editor" use the simple line number of the current selected block · 66566f87
      epriestley authored
      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
      66566f87
  4. 20 May, 2020 5 commits
    • epriestley's avatar
      Clean up Diffusion behaviors for inline edit suggestions · d2d7e7f5
      epriestley authored
      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
      d2d7e7f5
    • epriestley's avatar
      Render inline comment suggestions as real diffs · 10f24135
      epriestley authored
      Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing.
      
      Test Plan: {F7495053}
      
      Maniphest Tasks: T13513
      
      Differential Revision: https://secure.phabricator.com/D21277
      10f24135
    • epriestley's avatar
      Roughly support inline comment suggestions · 84656215
      epriestley authored
      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
      84656215
    • epriestley's avatar
      Make server components of inline comment content handling state-oriented · 00430fdb
      epriestley authored
      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
      00430fdb
    • epriestley's avatar
      Make inline content "state-oriented", not "string-oriented" · 87bc3052
      epriestley authored
      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
      87bc3052
  5. 19 May, 2020 2 commits
    • epriestley's avatar
      (stable) Fix "r" and "R" both replying with quote on inline comments · 8dec499b
      epriestley authored
      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
      8dec499b
    • epriestley's avatar
      Fix "r" and "R" both replying with quote on inline comments · 770a5c84
      epriestley authored
      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
      770a5c84
  6. 15 May, 2020 4 commits
    • epriestley's avatar
      (stable) Fix an out-of-order access issue with inlines · b24bac5a
      epriestley authored
      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
      b24bac5a
    • epriestley's avatar
      Fix an out-of-order access issue with inlines · 93b08f0e
      epriestley authored
      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
      93b08f0e
    • epriestley's avatar
      Use a more consistent inline highlighting style with fewer redraws · e959f934
      epriestley authored
      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
      e959f934
    • 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
  7. 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
  8. 13 May, 2020 1 commit
    • 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
  9. 12 May, 2020 2 commits
    • 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
  10. 08 May, 2020 3 commits
    • 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
  11. 07 May, 2020 1 commit