1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-29 18:22:41 +01:00

Fix every HTML issue I could find

Summary:
I attempted to test every interface. I probably missed some stuff, but I at least have some level of confidence that the `phutil_tag` branch is fairly stable.

Fixed these issues:

[1] Fixed a Herald issue with object links in transcripts. Some actions return
links; this was previously hard-coded.
[2] DarkConsole refactoring created an issue where the "`" event handler registered too many times.
[3] Fixed a bug where `strlen($value)` was being checked, but fields may now return array(). Possibly we should implement phutil_is_empty_html() or similar.
[4] Fixed a undefined variable issue for image edit transactions.
[5] Fixed an issue with rendering participant transactions. This required phutil_safe_html() because `pht()` can't handle `array()` for `%s`.
[6] Fixed an issue where feed was entirely overescaped by reverting an overly ambitious render_tag -> tag.
[7] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[8] Fixed an issue where • was shown escaped.
[9] Fixed an issue where "no data" was overescaped.
[10] Fixed an issue with strict tables and inserting `''` instead of `0` into an integer column.
[11] Fixed an issue with strict tables and inserting `''`.
[12] Fixed an issue with missing space after ":" for mini panels.

Encountered (but did not fix) these issues:

[X1] "e" works incorrectly on comments you are not allowed to edit. Did not fix.
[X2] Coverage currently interacts incorrectly with "--everything" for Phutil tests.

Test Plan:
  - Viewed Differential.
  - Created a diff via copy/paste.
  - Viewed standalone diff.
  - Jumped to diff via changeset table.
  - Created a revision.
  - Updated revision.
  - Added a comment.
  - Edited revision dependencies.
  - Edited revision tasks.
  - Viewed MetaMTA transcripts.
  - Viewed Herald transcripts [1].
  - Downloaded raw diff.
  - Flagged / unflagged revision.
  - Added/edited/deleted inline comment.
  - Collapsed/expanded file.
  - Did show raw left.
  - Did show raw right.
  - Checked previews for available actions.
  - Clicked remarkup buttons
  - Used filetree view.
  - Used keyboard: F, j, k, J, K, n, p, t, h, "?" [2] [X1].
  - Created a meme.
  - Uploaded a file via drag and drop.
  - Viewed a revision with no reviewers.
  - Viewed a revision with >100 files.
  - Viewed various other revisions [3].
  - Viewed an image diff.
  - Added image diff inline comments.
  - Viewed Maniphest.
  - Ran various queries.
  - Created task.
  - Created similar task.
  - Added comments to tasks.
  - Ran custom query.
  - Saved custom query.
  - Edited custom queries.
  - Drag-reordered tasks.
  - Batch edited tasks.
  - Exported tasks to excel.
  - Looked at reports (issue in T2311 notwithstanding).
  - Viewed Diffusion.
  - Browsed Git, SVN, HG repositories.
  - Looked at history, browse, change, commit views.
  - Viewed audit.
  - Performed various audit searches.
  - Viewed Paste.
  - Performed paste searches.
  - Created, edited, forked paste.
  - Viewed Phriction.
  - Edited a page.
  - Viewed edit history.
  - Used search typeahead to search for user / application.
  - Used search to search for text.
  - Viewed Phame.
  - Viewed Blog, Post.
  - Viewed live post.
  - Published/unpublished post.
  - Previewed post.
  - Viewed Pholio.
  - Edited/commented mock.
  - Viewed ponder.
  - Viewed question.
  - Added answer/comment.
  - Viewed Diviner.
  - Viewed Conpherence [4] [5].
  - Made Conpherence updates.
  - Viewed calendar.
  - Created status.
  - Viewed status.
  - Viewed Feed [6].
  - Viewed Projects.
  - Viewed project detail.
  - Edited project.
  - Viewed Owners.
  - Viewed package detail.
  - Edited package [7].
  - Viewed flags.
  - Edited flag.
  - Deleted flag.
  - Viewed Herald.
  - Viewed rules.
  - Created rule.
  - Edited rule.
  - Viewed edit log.
  - Viewed transcripts.
  - Inspected a transcript.
  - Viewed People.
  - Viewed list.
  - Administrated user.
  - Checked username/delete stuff.
  - Looked at create/import LDAP/activity logs.
  - Looked at a user profile.
  - Looked at user about page.
  - Looked at Repositories.
  - Edited repository.
  - Edited arcanist project.
  - Looked at daemons.
  - Looked at all daemons [8].
  - Viewed combined log.
  - Looked at configuration.
  - Edited configuration.
  - Looked at setup issues [9].
  - Looked at current settings.
  - Looked at application list.
  - Installed / uninstalled applications [10].
  - Looked at mailing lists.
  - Created a mailing list.
  - Edited a mailing list.
  - Looked at sent mail.
  - Looked at received mail.
  - Looked at send/receive tests.
  - Looked at settings.
  - Clicked through all the panels.
  - Looked at slowvote.
  - Created a slowvote [11].
  - Voted in a slowvote.
  - Looked at Macro.
  - Created a macro.
  - Edited a macro.
  - Commented on a macro.
  - Looked at Countdown.
  - Created a Countdown.
  - Looked at it.
  - Looked at Drydock.
  - Poked around a bit.
  - Looked at Fact.
  - Poked around a bit.
  - Looked at files.
  - Looked at a file.
  - Uploaded a file.
  - Looked at Conduit.
  - Made a Conduit call.
  - Looked at UIExamples.
  - Looked at PHPAST.
  - Looked at PHIDs.
  - Looked at notification menu.
  - Looked at notification detail.
  - Logged out.
  - Logged in.
  - Looked at homepage [12].
  - Ran `arc unit --everything --no-coverage` [X2].

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4807
This commit is contained in:
epriestley 2013-02-04 17:06:34 -08:00
parent ebc2937dc8
commit 94f6b6ca4e
14 changed files with 64 additions and 53 deletions

View file

@ -18,7 +18,7 @@ final class PhabricatorConfigEditor
switch ($xaction->getTransactionType()) { switch ($xaction->getTransactionType()) {
case PhabricatorConfigTransaction::TYPE_EDIT: case PhabricatorConfigTransaction::TYPE_EDIT:
return array( return array(
'deleted' => (bool)$object->getIsDeleted(), 'deleted' => (int)$object->getIsDeleted(),
'value' => $object->getValue(), 'value' => $object->getValue(),
); );
} }
@ -54,7 +54,7 @@ final class PhabricatorConfigEditor
$v['value']); $v['value']);
} }
$object->setIsDeleted($v['deleted']); $object->setIsDeleted((int)$v['deleted']);
$object->setValue($v['value']); $object->setValue($v['value']);
break; break;
} }

View file

@ -155,7 +155,9 @@ abstract class ConpherenceController extends PhabricatorController {
$item->addClass('conpherence-selected'); $item->addClass('conpherence-selected');
$item->addClass('hide-unread-count'); $item->addClass('hide-unread-count');
} }
$nav->addCustomBlock($item->render());
// TODO: [HTML] Clean this up when we clean up HTML stuff in Conpherence.
$nav->addCustomBlock(phutil_safe_html($item->render()));
} }
if (empty($conpherences) || $read) { if (empty($conpherences) || $read) {
$nav->addCustomBlock($this->getNoConpherencesBlock()); $nav->addCustomBlock($this->getNoConpherencesBlock());

View file

@ -35,6 +35,7 @@ final class ConpherenceTransactionView extends AphrontView {
->setEpoch($transaction->getDateCreated()) ->setEpoch($transaction->getDateCreated())
->setContentSource($transaction->getContentSource()); ->setContentSource($transaction->getContentSource());
$content = null;
$content_class = null; $content_class = null;
switch ($transaction->getTransactionType()) { switch ($transaction->getTransactionType()) {
case ConpherenceTransactionType::TYPE_TITLE: case ConpherenceTransactionType::TYPE_TITLE:

View file

@ -47,17 +47,17 @@ final class PhabricatorDaemonLogListView extends AphrontView {
case PhabricatorDaemonLog::STATUS_RUNNING: case PhabricatorDaemonLog::STATUS_RUNNING:
$style = 'color: #00cc00'; $style = 'color: #00cc00';
$title = 'Running'; $title = 'Running';
$symbol = '•'; $symbol = "\xE2\x80\xA2";
break; break;
case PhabricatorDaemonLog::STATUS_DEAD: case PhabricatorDaemonLog::STATUS_DEAD:
$style = 'color: #cc0000'; $style = 'color: #cc0000';
$title = 'Died'; $title = 'Died';
$symbol = '•'; $symbol = "\xE2\x80\xA2";
break; break;
case PhabricatorDaemonLog::STATUS_EXITED: case PhabricatorDaemonLog::STATUS_EXITED:
$style = 'color: #000000'; $style = 'color: #000000';
$title = 'Exited'; $title = 'Exited';
$symbol = '•'; $symbol = "\xE2\x80\xA2";
break; break;
case PhabricatorDaemonLog::STATUS_UNKNOWN: case PhabricatorDaemonLog::STATUS_UNKNOWN:
default: // fallthrough default: // fallthrough

View file

@ -116,10 +116,13 @@ final class DifferentialChangesetFileTreeSideNavBuilder {
} }
$tree->destroy(); $tree->destroy();
$filetree = $filetree = phutil_tag(
'<div class="phabricator-filetree">'. 'div',
implode("\n", $filetree). array(
'</div>'; 'class' => 'phabricator-filetree',
),
$filetree);
$nav->addLabel(pht('Changed Files')); $nav->addLabel(pht('Changed Files'));
$nav->addCustomBlock($filetree); $nav->addCustomBlock($filetree);
$nav->setActive(true); $nav->setActive(true);

View file

@ -80,7 +80,7 @@ final class DifferentialRevisionDetailView extends AphrontView {
foreach ($this->auxiliaryFields as $field) { foreach ($this->auxiliaryFields as $field) {
$value = $field->renderValueForRevisionView(); $value = $field->renderValueForRevisionView();
if (strlen($value)) { if ($value !== null) {
$label = rtrim($field->renderLabelForRevisionView(), ':'); $label = rtrim($field->renderLabelForRevisionView(), ':');
$properties->addProperty($label, $value); $properties->addProperty($label, $value);
} }

View file

@ -345,7 +345,7 @@ final class PhabricatorDirectoryMainController
array( array(
), ),
array( array(
phutil_tag('strong', array(), $title.':'), phutil_tag('strong', array(), $title.': '),
$body $body
))); )));
$this->minipanels[] = $panel; $this->minipanels[] = $panel;

View file

@ -111,7 +111,7 @@ final class PhabricatorFeedStoryView extends PhabricatorFeedView {
require_celerity_resource('phabricator-feed-css'); require_celerity_resource('phabricator-feed-css');
return phutil_tag( return phutil_render_tag(
'div', 'div',
array( array(
'class' => $this->oneLine 'class' => $this->oneLine
@ -119,7 +119,7 @@ final class PhabricatorFeedStoryView extends PhabricatorFeedView {
: 'phabricator-feed-story', : 'phabricator-feed-story',
'style' => $image_style, 'style' => $image_style,
), ),
array($head, $body, $foot)); $this->renderSingleView(array($head, $body, $foot)));
} }
} }

View file

@ -476,21 +476,21 @@ final class HeraldTranscriptController extends HeraldController {
$rows = array(); $rows = array();
foreach ($data as $name => $value) { foreach ($data as $name => $value) {
if (!is_scalar($value) && !is_null($value)) { if (!($value instanceof PhutilSafeHTML)) {
$value = implode("\n", $value); if (!is_scalar($value) && !is_null($value)) {
} $value = implode("\n", $value);
}
if (strlen($value) > 256) { if (strlen($value) > 256) {
$value = phutil_tag( $value = phutil_tag(
'textarea', 'textarea',
array( array(
'class' => 'herald-field-value-transcript', 'class' => 'herald-field-value-transcript',
), ),
$value); $value);
} else if ($name === 'Object Link') { } else {
// The link cannot be escaped $value = phutil_escape_html($value);
} else { }
$value = phutil_escape_html($value);
} }
$rows[] = array( $rows[] = array(

View file

@ -32,7 +32,10 @@ final class PhabricatorOwnersEditController
$package->setName($request->getStr('name')); $package->setName($request->getStr('name'));
$package->setDescription($request->getStr('description')); $package->setDescription($request->getStr('description'));
$old_auditing_enabled = $package->getAuditingEnabled(); $old_auditing_enabled = $package->getAuditingEnabled();
$package->setAuditingEnabled($request->getStr('auditing') === 'enabled'); $package->setAuditingEnabled(
($request->getStr('auditing') === 'enabled')
? 1
: 0);
$primary = $request->getArr('primary'); $primary = $request->getArr('primary');
$primary = reset($primary); $primary = reset($primary);

View file

@ -23,7 +23,7 @@ final class PhabricatorSlowvoteCreateController
if ($request->isFormPost()) { if ($request->isFormPost()) {
$poll->setQuestion($request->getStr('question')); $poll->setQuestion($request->getStr('question'));
$poll->setResponseVisibility($request->getInt('response_visibility')); $poll->setResponseVisibility($request->getInt('response_visibility'));
$poll->setShuffle($request->getBool('shuffle', false)); $poll->setShuffle((int)$request->getBool('shuffle', false));
$poll->setMethod($request->getInt('method')); $poll->setMethod($request->getInt('method'));
if (!strlen($poll->getQuestion())) { if (!strlen($poll->getQuestion())) {

View file

@ -143,7 +143,7 @@ abstract class PhabricatorApplicationTransaction
foreach ($phids as $phid) { foreach ($phids as $phid) {
$links[] = $this->renderHandleLink($phid); $links[] = $this->renderHandleLink($phid);
} }
return implode(', ', $links); return phutil_safe_html(implode(', ', $links));
} }
public function getIcon() { public function getIcon() {

View file

@ -53,8 +53,7 @@ final class PhabricatorObjectItemListView extends AphrontView {
$string = nonempty($this->noDataString, pht('No data.')); $string = nonempty($this->noDataString, pht('No data.'));
$items = id(new AphrontErrorView()) $items = id(new AphrontErrorView())
->setSeverity(AphrontErrorView::SEVERITY_NODATA) ->setSeverity(AphrontErrorView::SEVERITY_NODATA)
->appendChild(phutil_escape_html($string)) ->appendChild(phutil_escape_html($string));
->render();
} }
$pager = null; $pager = null;

View file

@ -40,6 +40,8 @@ JX.behavior('dark-console', function(config, statics) {
statics.visible = config.visible; statics.visible = config.visible;
statics.selected = config.selected; statics.selected = config.selected;
install_shortcut();
return statics.root; return statics.root;
} }
@ -202,29 +204,30 @@ JX.behavior('dark-console', function(config, statics) {
JX.DOM.setContent(statics.el.panel, div); JX.DOM.setContent(statics.el.panel, div);
} }
// Install keyboard shortcut. function install_shortcut() {
var desc = 'Toggle visibility of DarkConsole.'; var desc = 'Toggle visibility of DarkConsole.';
new JX.KeyboardShortcut('`', desc) new JX.KeyboardShortcut('`', desc)
.setHandler(function(manager) { .setHandler(function(manager) {
statics.visible = !statics.visible; statics.visible = !statics.visible;
if (statics.visible) { if (statics.visible) {
JX.DOM.show(root); JX.DOM.show(root);
if (statics.req.current) { if (statics.req.current) {
draw_request(statics.req.current); draw_request(statics.req.current);
}
} else {
JX.DOM.hide(root);
} }
} else {
JX.DOM.hide(root);
}
// Save user preference. // Save user preference.
new JX.Request('/~/', JX.bag) new JX.Request('/~/', JX.bag)
.setData({visible: statics.visible ? 1 : 0}) .setData({visible: statics.visible ? 1 : 0})
.send(); .send();
// Force resize listeners to take effect. // Force resize listeners to take effect.
JX.Stratcom.invoke('resize'); JX.Stratcom.invoke('resize');
}) })
.register(); .register();
}
}); });