Skip to content

Commit 98fe8fa

Browse files
author
epriestley
committed
Use <td class="n" data-n="3"> instead of <th>3</th> for line numbers
Summary: Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `<th>3</th>` was a perfectly fine way to render a line number cell containing the number "3". But times have changed! - In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to `<td>`. - In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own. The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately. Test Plan: - Created, edited, deleted inlines in 1-up and 2-up views. - Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161, T12822 Differential Revision: https://secure.phabricator.com/D20188
1 parent 3f8eccd commit 98fe8fa

File tree

7 files changed

+92
-59
lines changed

7 files changed

+92
-59
lines changed

resources/celerity/map.php

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111
'conpherence.pkg.js' => '020aebcf',
1212
'core.pkg.css' => '261ee8cf',
1313
'core.pkg.js' => '5ace8a1e',
14-
'differential.pkg.css' => 'c3f15714',
15-
'differential.pkg.js' => 'be031567',
14+
'differential.pkg.css' => '801c5653',
15+
'differential.pkg.js' => '1f211736',
1616
'diffusion.pkg.css' => '42c75c37',
1717
'diffusion.pkg.js' => '91192d85',
1818
'maniphest.pkg.css' => '35995d6d',
@@ -61,7 +61,7 @@
6161
'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6',
6262
'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d',
6363
'rsrc/css/application/differential/add-comment.css' => '7e5900d9',
64-
'rsrc/css/application/differential/changeset-view.css' => '783a9206',
64+
'rsrc/css/application/differential/changeset-view.css' => '8a997ed9',
6565
'rsrc/css/application/differential/core.css' => 'bdb93065',
6666
'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b',
6767
'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d',
@@ -375,7 +375,7 @@
375375
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
376376
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '9b1cbd76',
377377
'rsrc/js/application/diff/DiffChangeset.js' => 'd0a85a85',
378-
'rsrc/js/application/diff/DiffChangesetList.js' => 'b91204e9',
378+
'rsrc/js/application/diff/DiffChangesetList.js' => '26fb79ba',
379379
'rsrc/js/application/diff/DiffInline.js' => 'a4a14a94',
380380
'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17',
381381
'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -541,7 +541,7 @@
541541
'conpherence-thread-manager' => 'aec8e38c',
542542
'conpherence-transaction-css' => '3a3f5e7e',
543543
'd3' => 'd67475f5',
544-
'differential-changeset-view-css' => '783a9206',
544+
'differential-changeset-view-css' => '8a997ed9',
545545
'differential-core-view-css' => 'bdb93065',
546546
'differential-revision-add-comment-css' => '7e5900d9',
547547
'differential-revision-comment-css' => '7dbc8d1d',
@@ -754,7 +754,7 @@
754754
'phabricator-darkmessage' => '26cd4b73',
755755
'phabricator-dashboard-css' => '4267d6c6',
756756
'phabricator-diff-changeset' => 'd0a85a85',
757-
'phabricator-diff-changeset-list' => 'b91204e9',
757+
'phabricator-diff-changeset-list' => '26fb79ba',
758758
'phabricator-diff-inline' => 'a4a14a94',
759759
'phabricator-drag-and-drop-file-upload' => '4370900d',
760760
'phabricator-draggable-list' => '3c6bd549',
@@ -1087,6 +1087,10 @@
10871087
'javelin-json',
10881088
'phabricator-draggable-list',
10891089
),
1090+
'26fb79ba' => array(
1091+
'javelin-install',
1092+
'phuix-button-view',
1093+
),
10901094
'27daef73' => array(
10911095
'multirow-row-manager',
10921096
'javelin-install',
@@ -1513,9 +1517,6 @@
15131517
'javelin-uri',
15141518
'javelin-request',
15151519
),
1516-
'783a9206' => array(
1517-
'phui-inline-comment-view-css',
1518-
),
15191520
'78bc5d94' => array(
15201521
'javelin-behavior',
15211522
'javelin-uri',
@@ -1586,6 +1587,9 @@
15861587
'8a16f91b' => array(
15871588
'syntax-default-css',
15881589
),
1590+
'8a997ed9' => array(
1591+
'phui-inline-comment-view-css',
1592+
),
15891593
'8ac32fd9' => array(
15901594
'javelin-behavior',
15911595
'javelin-stratcom',
@@ -1895,10 +1899,6 @@
18951899
'javelin-uri',
18961900
'phabricator-notification',
18971901
),
1898-
'b91204e9' => array(
1899-
'javelin-install',
1900-
'phuix-button-view',
1901-
),
19021902
'bd546a49' => array(
19031903
'phui-workcard-view-css',
19041904
),

src/applications/differential/render/DifferentialChangesetOneUpRenderer.php

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -92,19 +92,23 @@ protected function renderPrimitives(array $primitives, $rows) {
9292
$line = $p['line'];
9393

9494
$cells[] = phutil_tag(
95-
'th',
95+
'td',
9696
array(
9797
'id' => $left_id,
98-
'class' => $class,
99-
),
100-
$line);
98+
'class' => $class.' n',
99+
'data-n' => $line,
100+
));
101101

102102
$render = $p['render'];
103103
if ($aural !== null) {
104104
$render = array($aural, $render);
105105
}
106106

107-
$cells[] = phutil_tag('th', array('class' => $class));
107+
$cells[] = phutil_tag(
108+
'td',
109+
array(
110+
'class' => $class.' n',
111+
));
108112
$cells[] = $no_copy;
109113
$cells[] = phutil_tag('td', array('class' => $class), $render);
110114
$cells[] = $no_coverage;
@@ -115,7 +119,11 @@ protected function renderPrimitives(array $primitives, $rows) {
115119
} else {
116120
$class = 'right new';
117121
}
118-
$cells[] = phutil_tag('th', array('class' => $class));
122+
$cells[] = phutil_tag(
123+
'td',
124+
array(
125+
'class' => $class.' n',
126+
));
119127
$aural = $aural_plus;
120128
} else {
121129
$class = 'right';
@@ -127,7 +135,13 @@ protected function renderPrimitives(array $primitives, $rows) {
127135

128136
$oline = $p['oline'];
129137

130-
$cells[] = phutil_tag('th', array('id' => $left_id), $oline);
138+
$cells[] = phutil_tag(
139+
'td',
140+
array(
141+
'id' => $left_id,
142+
'class' => 'n',
143+
'data-n' => $oline,
144+
));
131145
$aural = null;
132146
}
133147

@@ -144,12 +158,12 @@ protected function renderPrimitives(array $primitives, $rows) {
144158
$line = $p['line'];
145159

146160
$cells[] = phutil_tag(
147-
'th',
161+
'td',
148162
array(
149163
'id' => $right_id,
150-
'class' => $class,
151-
),
152-
$line);
164+
'class' => $class.' n',
165+
'data-n' => $line,
166+
));
153167

154168
$render = $p['render'];
155169
if ($aural !== null) {

src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -306,10 +306,26 @@ public function renderTextChange(
306306
// clipboard. See the 'phabricator-oncopy' behavior.
307307
$zero_space = "\xE2\x80\x8B";
308308

309+
$old_number = phutil_tag(
310+
'td',
311+
array(
312+
'id' => $o_id,
313+
'class' => $o_classes.' n',
314+
'data-n' => $o_num,
315+
));
316+
317+
$new_number = phutil_tag(
318+
'td',
319+
array(
320+
'id' => $n_id,
321+
'class' => $n_classes.' n',
322+
'data-n' => $n_num,
323+
));
324+
309325
$html[] = phutil_tag('tr', array(), array(
310-
phutil_tag('th', array('id' => $o_id, 'class' => $o_classes), $o_num),
326+
$old_number,
311327
phutil_tag('td', array('class' => $o_classes), $o_text),
312-
phutil_tag('th', array('id' => $n_id, 'class' => $n_classes), $n_num),
328+
$new_number,
313329
$n_copy,
314330
phutil_tag(
315331
'td',

src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,8 @@ public function render() {
3131
}
3232

3333
$cells = array(
34-
phutil_tag('th', array(), $left_hidden),
35-
phutil_tag('th', array(), $right_hidden),
34+
phutil_tag('td', array('class' => 'n'), $left_hidden),
35+
phutil_tag('td', array('class' => 'n'), $right_hidden),
3636
phutil_tag('td', $attrs, $inline),
3737
);
3838

src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,9 @@ public function render() {
7171
);
7272

7373
$cells = array(
74-
phutil_tag('th', array(), $left_hidden),
74+
phutil_tag('td', array('class' => 'n'), $left_hidden),
7575
phutil_tag('td', $left_attrs, $left_side),
76-
phutil_tag('th', array(), $right_hidden),
76+
phutil_tag('td', array('class' => 'n'), $right_hidden),
7777
phutil_tag('td', $right_attrs, $right_side),
7878
);
7979

webroot/rsrc/css/application/differential/changeset-view.css

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -72,23 +72,6 @@
7272
width: 0;
7373
}
7474

75-
.differential-diff th {
76-
text-align: right;
77-
padding: 1px 6px 1px 0;
78-
vertical-align: top;
79-
background: {$lightbluebackground};
80-
color: {$bluetext};
81-
cursor: pointer;
82-
border-right: 1px solid {$thinblueborder};
83-
overflow: hidden;
84-
85-
-moz-user-select: -moz-none;
86-
-khtml-user-select: none;
87-
-webkit-user-select: none;
88-
-ms-user-select: none;
89-
user-select: none;
90-
}
91-
9275
.prose-diff {
9376
padding: 12px 0;
9477
white-space: pre-wrap;
@@ -182,6 +165,34 @@
182165
background: #dddddd;
183166
}
184167

168+
.differential-diff .inline > td {
169+
padding: 0;
170+
}
171+
172+
/* Specify line number behaviors after other behaviors because line numbers
173+
should always have a boring grey background. */
174+
175+
.differential-diff td.n {
176+
text-align: right;
177+
padding: 1px 6px 1px 0;
178+
vertical-align: top;
179+
background: {$lightbluebackground};
180+
color: {$bluetext};
181+
cursor: pointer;
182+
border-right: 1px solid {$thinblueborder};
183+
overflow: hidden;
184+
185+
-moz-user-select: -moz-none;
186+
-khtml-user-select: none;
187+
-webkit-user-select: none;
188+
-ms-user-select: none;
189+
user-select: none;
190+
}
191+
192+
.differential-diff td.n::before {
193+
content: attr(data-n);
194+
}
195+
185196
.differential-diff td.cov {
186197
padding: 0;
187198
}
@@ -316,10 +327,6 @@ td.cov-I {
316327
pointer-events: none;
317328
}
318329

319-
.differential-diff .inline > td {
320-
padding: 0;
321-
}
322-
323330
.differential-loading {
324331
border-top: 1px solid {$gentle.highlight.border};
325332
border-bottom: 1px solid {$gentle.highlight.border};

webroot/rsrc/js/application/diff/DiffChangesetList.js

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,13 @@ JX.install('DiffChangesetList', {
7070
var onrangedown = JX.bind(this, this._ifawake, this._onrangedown);
7171
JX.Stratcom.listen(
7272
'mousedown',
73-
['differential-changeset', 'tag:th'],
73+
['differential-changeset', 'tag:td'],
7474
onrangedown);
7575

7676
var onrangemove = JX.bind(this, this._ifawake, this._onrangemove);
7777
JX.Stratcom.listen(
7878
['mouseover', 'mouseout'],
79-
['differential-changeset', 'tag:th'],
79+
['differential-changeset', 'tag:td'],
8080
onrangemove);
8181

8282
var onrangeup = JX.bind(this, this._ifawake, this._onrangeup);
@@ -360,7 +360,7 @@ JX.install('DiffChangesetList', {
360360
while (row) {
361361
var header = row.firstChild;
362362
while (header) {
363-
if (JX.DOM.isType(header, 'th')) {
363+
if (this.getLineNumberFromHeader(header)) {
364364
if (header.className.indexOf('old') !== -1) {
365365
old_list.push(header);
366366
} else if (header.className.indexOf('new') !== -1) {
@@ -1247,11 +1247,7 @@ JX.install('DiffChangesetList', {
12471247
},
12481248

12491249
getLineNumberFromHeader: function(th) {
1250-
try {
1251-
return parseInt(th.id.match(/^C\d+[ON]L(\d+)$/)[1], 10);
1252-
} catch (x) {
1253-
return null;
1254-
}
1250+
return parseInt(th.getAttribute('data-n'));
12551251
},
12561252

12571253
getDisplaySideFromHeader: function(th) {

0 commit comments

Comments
 (0)