Page MenuHomePhabricator
Create Task
Maniphest T6107

"Reply to inline comment" shortcut broken in Diffusion
Closed, ResolvedPublic
Actions

Description

The "r" keyboard shortcut (reply to inline comment) throws an exception, even when a comment in selected. Just tried this on rPffa017630f2d, here's the stacktrace:

TypeError: Cannot read property 'getElementsByTagName' of null
#10 https://secure.phabricator.com/res/phabricator/cbdbd552/core.pkg.js:333
#9 https://secure.phabricator.com/res/phabricator/cbdbd552/core.pkg.js:333:11 Function.JX.install.statics.scry()
#8 https://secure.phabricator.com/res/phabricator/73337d1d/differential.pkg.js:188:31 inline_op()
#7 https://secure.phabricator.com/res/phabricator/73337d1d/differential.pkg.js:190:84 
#6 https://secure.phabricator.com/res/phabricator/cbdbd552/core.pkg.js:586:71 Object.JX.install.members._onkeyhit()
#5 https://secure.phabricator.com/res/phabricator/cbdbd552/core.pkg.js:578:45 Object.JX.install.members._onkeypress()
#4 https://secure.phabricator.com/res/phabricator/cbdbd552/core.pkg.js:147:62 Function.JX.install.statics.pass()
#3 https://secure.phabricator.com/res/phabricator/cbdbd552/core.pkg.js:142:299 Function.JX.install.statics._dispatchProxy()
#2 https://secure.phabricator.com/res/phabricator/cbdbd552/core.pkg.js:125:170 Function.JX.install.statics.dispatch()
#1 https://secure.phabricator.com/res/phabricator/b88ab49e/rsrc/externals/javelin/core/init.js:15:53 HTMLHtmlElement.JX.__rawEventQueue()&host=secure.phabricator.com

Revisions and Commits

rP Phabricator
D10843rP22551d106d35 diffusion / differential - better handle case where no comment selected

Related Objects

Event Timeline

staticshock created this task.Sep 16 2014, 5:46 PM
staticshock raised the priority of this task from to Normal.
staticshock updated the task description. (Show Details)
staticshock added a project: Diffusion.
staticshock added a subscriber: staticshock.
epriestley added a project: Support Impact.Oct 3 2014, 3:40 AM
epriestley added a subscriber: epriestley.

Support Impact This is broken.

nkovacs added a subscriber: nkovacs.Oct 13 2014, 9:27 AM

The same error happens in differential, when you have a renamed file. You get the message "This file was changed only by adding or removing whitespace. Show File Contents", and clicking Show File Contents throws this error (in core.pkg.js line 333).

btrahan claimed this task.Nov 7 2014, 10:34 PM
btrahan added a comment.Nov 11 2014, 10:18 PM

I can't reproduce this fully and then have questions....

  • pressing 'r' throws a fatal
    • should this automagically select the first comment or do nothing? easy-ish fix in either case
  • select a comment (via 'n') and then pressing 'r' works

...and then there's another bug mentioned in the comments which is real. That one sadly is going to be hard to fix; its deep within differential parsing code... See https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/parser/DifferentialChangesetParser.php;b462a80cca31ebf55944b049a8adca4c2af997ea$597-604

btrahan mentioned this in T4599: Incorrect "only adding or removing whitespace" shield being applied to origin of moved and modified files..Nov 11 2014, 11:19 PM
btrahan added a revision: D10843: diffusion / differential - better handle case where no comment selected.Nov 12 2014, 8:20 PM
btrahan closed this task as Resolved.Nov 12 2014, 8:26 PM
btrahan added a commit: rP22551d106d35: diffusion / differential - better handle case where no comment selected.

Closed by commit rP22551d106d35.

Phabricator · Built by Phacility