HomePhabricator

Don't mangle inline comments with tables in them in Differential

Description

Don't mangle inline comments with tables in them in Differential

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

Details

Provenance
epriestleyAuthored on Sep 10 2013, 10:31 PM
Reviewer
btrahan
Differential Revision
Restricted Differential Revision
Parents
rP51eb8a301aa2: Clean up Diffusion repository list
Branches
Unknown
Tags
Unknown
Tasks
Restricted Maniphest Task

Event Timeline