e8126fa958
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:
- Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
- The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
- Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
- Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
- `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
- Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
- At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.
Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3814
Differential Revision: https://secure.phabricator.com/D6924
72 lines
1.8 KiB
JavaScript
72 lines
1.8 KiB
JavaScript
/**
|
|
* @provides javelin-behavior-differential-show-more
|
|
* @requires javelin-behavior
|
|
* javelin-dom
|
|
* javelin-workflow
|
|
* javelin-util
|
|
* javelin-stratcom
|
|
*/
|
|
|
|
JX.behavior('differential-show-more', function(config) {
|
|
|
|
function onresponse(context, response) {
|
|
var table = JX.$H(response.changeset).getNode();
|
|
var root = context.parentNode;
|
|
copyRows(root, table, context);
|
|
root.removeChild(context);
|
|
}
|
|
|
|
JX.Stratcom.listen(
|
|
'click',
|
|
'show-more',
|
|
function(e) {
|
|
var event_data = {
|
|
context : e.getNodes()['context-target'],
|
|
show : e.getNodes()['show-more']
|
|
};
|
|
|
|
JX.Stratcom.invoke('differential-reveal-context', null, event_data);
|
|
e.kill();
|
|
});
|
|
|
|
JX.Stratcom.listen(
|
|
'differential-reveal-context',
|
|
null,
|
|
function(e) {
|
|
var context = e.getData().context;
|
|
var data = JX.Stratcom.getData(e.getData().show);
|
|
|
|
var container = JX.DOM.scry(context, 'td')[0];
|
|
JX.DOM.setContent(container, 'Loading...');
|
|
JX.DOM.alterClass(context, 'differential-show-more-loading', true);
|
|
|
|
if (!data['whitespace']) {
|
|
data['whitespace'] = config.whitespace;
|
|
}
|
|
|
|
new JX.Workflow(config.uri, data)
|
|
.setHandler(JX.bind(null, onresponse, context))
|
|
.start();
|
|
});
|
|
|
|
});
|
|
|
|
function copyRows(dst, src, before) {
|
|
var rows = JX.DOM.scry(src, 'tr');
|
|
for (var ii = 0; ii < rows.length; ii++) {
|
|
|
|
// Find the table this <tr /> belongs to. If it's a sub-table, like a
|
|
// table in an inline comment, don't copy it.
|
|
if (JX.DOM.findAbove(rows[ii], 'table') !== src) {
|
|
continue;
|
|
}
|
|
|
|
if (before) {
|
|
dst.insertBefore(rows[ii], before);
|
|
} else {
|
|
dst.appendChild(rows[ii]);
|
|
}
|
|
}
|
|
return rows;
|
|
}
|