Page MenuHomePhabricator
Create Task
Maniphest T5030

Improve behavior when jumping to inline comments
Closed, WontfixPublic
Actions

Description

When jumping to inline comments, behaviors could be improved after T430:

  • If the comment is on the current diff, but the inline hasn't loaded yet, it would be nice to prioritize loading of that content block.
  • If the comment is on a different diff, it would be nice to link to the other diff with a hint in the URI about which content block to load first.
  • The 5 second timeout where we stop looking for an anchor is sort of arbitrary. Showing some kind of UI might be useful, or changing the rule to consider scrolling, content loading, have a longer timeout, etc.
  • The comment link should also start at the line(s) highlighted, not just the comment.

Revisions and Commits

rP Phabricator
D12497rP1858d2aa6679 Make Differential timeline aware of ghostly inlines

Related Objects
Search...

Event Timeline

epriestley created this task.May 12 2014, 5:52 PM
epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Differential.
epriestley added subscribers: epriestley, csilvers.
epriestley added a comment.May 12 2014, 5:55 PM

When viewing a URI like /D123, we'll generate links to, e.g,, /D123#inline-456 to avoid reloading the page. These will eventually expire when a diff is updated. Instead, the client and server can coordinate to figure out where the inline actually is and load that instead.

epriestley added a comment.May 12 2014, 5:56 PM

To deal with that last one more easily: we could generate permanent links instead (/D123?id=789#inline-456) since we have to capture them anyway to implement the "on the current diff..." case above.

sophiebits added a comment.May 12 2014, 6:29 PM

When viewing a URI like /D123, we'll generate links to, e.g,, /D123#inline-456 to avoid reloading the page. These will eventually expire when a diff is updated. Instead, the client and server can coordinate to figure out where the inline actually is and load that instead.

We could use window.pushState to change the URL without a reload, though of course that may be confusing.

igorgatis added a subscriber: igorgatis.May 18 2014, 12:15 PM
epriestley edited this Maniphest Task.May 25 2014, 2:16 PM
epriestley added a comment.Jun 8 2014, 7:45 PM

https://github.com/phacility/phabricator/issues/447 discusses a similar set of issues for Diffusion.

chad merged a task: T6324: Next should jump to line immediately above comment, rather than comment itself.Jan 19 2015, 9:31 PM
chad added subscribers: ezyang, joshuaspence.
chad updated the task description. (Show Details)Jan 19 2015, 11:24 PM
epriestley moved this task from Backlog to v2 (UI/Mobile) on the Differential board.Mar 4 2015, 5:13 PM
cspeckmim added a subscriber: cspeckmim.Apr 16 2015, 2:38 PM
chad mentioned this in T7846: Align the navigation buttons for inline comment toolbar.Apr 17 2015, 9:48 PM
chad merged a task: T7846: Align the navigation buttons for inline comment toolbar.
chad added a subscriber: chad.
epriestley added a revision: D12497: Make Differential timeline aware of ghostly inlines.EditedApr 21 2015, 8:33 PM

D12497 doesn't directly fix any of this, but it makes links to other diffs much less common.

epriestley added a commit: rP1858d2aa6679: Make Differential timeline aware of ghostly inlines.Apr 21 2015, 10:32 PM
jpoehls added a subscriber: jpoehls.Apr 22 2015, 5:52 PM
jhurwitz mentioned this in T8592: "View on previous revision" button on ghosted comment can jump to wrong place.Jun 17 2015, 9:55 PM
jhurwitz merged a task: T8592: "View on previous revision" button on ghosted comment can jump to wrong place.Sep 10 2015, 6:13 PM
jhurwitz added a project: Restricted Project.
jhurwitz added subscribers: • sbezek, angie, jhurwitz.
epriestley moved this task from v2 (UI/Mobile) to Inline Infra on the Differential board.Dec 29 2016, 6:11 PM
Herald added a subscriber: eadler. · View Herald TranscriptDec 29 2016, 6:11 PM
epriestley added a project: Inline Comments.Apr 21 2017, 12:20 PM
epriestley moved this task from Backlog to Glitches on the Inline Comments board.Apr 21 2017, 12:25 PM
epriestley mentioned this in T12616: Modernize display code for inline comments.May 4 2017, 2:25 PM
epriestley closed this task as Wontfix.May 16 2017, 5:34 PM
epriestley claimed this task.

I'm not sure this task has any specific, actionable issues remaining. Partly, it has just been open for twenty five decades and become a bit of a dumping ground.

I think at least some of this (in T8592) was probably T11784, which is really a different beast.

T7846 wasn't really related and we're at least trying a version without those actions now.

T6324 and T7846 discuss jumping above comments, but at HEAD we do jump a little bit above comments (3-4 lines on my screen) which seems pretty reasonable to me. It would be potentially bad to jump to the top of the highlighted range in the general case because the rage may be arbitrarily tall. We could still pursue some version of this, but I'd like to see more clearly-communicated, modern interest in it (ideally, after the cloud of issues surrounding T12616 start to settle).

epriestley added a comment.May 16 2017, 5:35 PM

(If something you care about got merged here and isn't covered at HEAD / by T12616, feel free to file a new report or feature request adhering to modern guidelines.)

Phabricator · Built by Phacility