This changes updates the unresolved toggle so that it only attempts to
save when explicitly tapped (as opposed to the value of resolved
changing on its own).
This was causing issues with comment synchronization between
gr-thread-view and diff comments, and also re-rendering the order of
comments in gr-thread-list within the dom-repeat.
This change also lets the resolved checkbox get selected via the enter
key.
Change-Id: I6065c745a0eabcf19df38f3cd6fd2c0d7e7434f3
For some dialogs, the confirm event should fire on enter. This change
adds this optional property.
Checking the disabled property before firing the confirm event should
prevent unintended side effects throughout the app.
Change-Id: I5603065b37bf87b1c4b36ff54376fb4066869afe
We encountered an issue where PolyGerrit was not respecting browser
font preferences. Browser font preferences include both a default font
size and a minimum font size.
In the event that a font declared in px is smaller than the minimum font
size, the size is increased to equal the minimum font size. However,
if the font is declared in px and greater than the minimum, the
preferred font size is not taken into account.
Browsers' default font size is 16px [1], So instead of declaring the
base font in px (previously 13px), this change makes it a percentage
of the default font size. If the user has changed their default, this
is taken into account.
From this point, all other fonts will be declared in rems, which makes
it easier to base the new size off of the base font size of the <html>
tag, rather than ems which bases it on the container[2]. This allows us
to declare font sizes in a way more similar to pixels, yet relative.
A follow-up change will change all font size declaration to use rem from
em for consistency.
[1] https://stackoverflow.com/questions/29511983/is-the-default-font-size-of-every-browser-16px-why
[2] https://webdesign.tutsplus.com/tutorials/comprehensive-guide-when-to-use-em-vs-rem--cms-23984
Bug: Issue 8157
Change-Id: I04ec2c861b2fb288ec7556dfb655d7feea9c9157
Formerly, the save button would be disabled when attempting to edit a
comment to have the same content as what's already been saved. This can
be a confusing UX where it isn't clear why a comment cannot be saved.
Allow saving comments even when the text has not been changed.
Bug: Issue 8128
Change-Id: I615c38e0cc109ea87f58c9b8a9b7de28ec459b88
Previously, the resolved checkbox called _saveDraft(). Using save()
instead disables the comment while waiting for the save request to
return.
Bug: Issue 7933
Change-Id: I44ce88094b271aa46a726a75aa4eb283b29f1a35
Draft comments with message text should have a confirm dialog when
discarded to prevent accidental data loss.
This is a regression introduced by I0dae814.
Bug: Issue 7900
Change-Id: Ibd9829ccf36a551445282e5cb0d37c1a6df46ab9
A recent change modified some comment editing UX. As a consequence, the
save button disabled state was erroneously based entirely on whether or
not the comment message had changed.
With this change:
- Save disabled state on a comment takes into account resolved state
- The resolved checkbox is always visible on an expanded draft comment
- When the resolved checkbox is modified on a non-`editing` draft
comment, it is saved immediately
Bug: Issue 6023
Change-Id: I40b0b280129e5a76c13e38c97525f91a8e56482d
A few changes:
- Utilize mixins within gr-button internally, so less styles have to be
overridden by elements implementing their own button styles.
- All buttons have a hover background color, which is 12% black
overlayed on top of its current background color.
Bug: Issue 7894
Change-Id: I4f2879aa0232912267bbf1290a1c800d024099a6
Recently, the comment editing UX was changed (the discard button was
removed from the editing view). This (obviously) made it impossible to
discard a draft comment whilst editing it.
Editing a comment, clearing the text, and saving now removes the
comment.
Bug: Issue 7832
Change-Id: I0dae814b9ce2e7a5b57530f839c708e59c215341
This change makes the following replacement:
<content select=".header"> replace it with
the slot equivalent.
Change-Id: Icc68576a3b44559775103163febd429194802981
The removal of the 'Discard' button from comments in the editing state
complicated the UX of the 'Cancel' button a bit.
Previously:
- Discard deleted the draft completely.
- Cancel cancelled whatever edits were made, and reverted to the prior
draft state. Cancel was also hidden on new drafts.
Currently:
- Cancel cancels whatever edits were made, and reverts to whatever edit
was last stored.
This behavior is problematic, for a few reasons:
- Due to some vestigial code, the actual value of the comment's message
(not just the buffer value '_messageText') was updated any time the
draft comment was edited. This was done to prevent multiple drafts
from being created on the same thread -- that issue was made obsolete
by the removal of buttons from individual comments, and their
consolidation to the thread level.
- If a user cancels a draft, then restarts it (having their text
repopulated from localstorage), then cancels it again, the draft is
rendered as a saved draft comment with the localstorage value, despite
having never been saved at all.
So, with this change:
- Cancel cancels whatever edits were made.
- If a user tries to add a draft in the same place...
- ...and the draft has been saved on the server, the textarea is
prepopulated with the value from the comment received from the
server.
- ...and the draft has NOT been saved on the server, the textarea is
rehydrated from localstorage.
This UX makes sense because...
- Case 1 occurs when a user clicks "Edit" on an existing draft comment -
thus expecting to edit the content shown in the UI.
- Case 2 occurs when a user cancels a draft that they started
constructing, but did not save -- thus there is no suggestion of any
prepopulated value by the UI, _and_ the case in which a draft was
erroneously cancelled during editing can be solved.
Bug: Issue 7709
Change-Id: Iacf9a91fbb0226aba3a518f56edd61f28c15a8ef
Some modifications to the styling of paper-button within gr-button
overrode a few deliberate styling decisions made in the delete comment
button (namely, color and padding). This change moves those CSS props
inside a styling mixin so that they are properly applied, and overload
the defaults from inside gr-button.
Change-Id: Ifdfc0a3959f4131e1bb86e8b66d70db2fde79077
The hidden attribute was erroneously left on the cancel button in diff
comments, causing it to not show up by default.
It showed up upon editing an existing draft because the hideOnPublished
CSS overrode the hack for the [hidden] attribute.
In addition, the buggy behavior was actually enforced by a buggy test --
it asserted that the button was not visible, but was labelled as "Cancel
is visible".
Bug: Issue 7664
Change-Id: I9e08c33fc4adf0f1465a52055a3308210625ffde
Unsaved diff drafts are kept in localstorage so they are not lost if
they do not get saved (for example, if the browser crashes). However,
this stored copy of the comment is cleared when the save request is
started, not after it succeeds. As such, if the save request fails (for
example, if the user credentials are invalid), the comment can be truly
lost.
With this change, the localstorage copy is only cleared after save
success. If the request fails, it allows the error message to appear
rather than showing a misleading "All comments saved" toast.
Bug: issue 7289
Change-Id: Id5c8144ecccec50de35f83af616b4e4915b62b5c
The [Save] button cannot be clicked when the comment is disabled (via
`pointer-events: disabled;` in the CSS), and clicking the [Save] button
results in setting disabled to true. However, it is possible to click
save again before the style is applied. It's also possible to trigger
the button using a non-pointer-event (such as a key). With this change,
an extra check is added to the [Save] button's click handler to prevent
clicks when the component is disabled.
Bug: Issue 6972
Change-Id: I16117af31c5e7b5d7d49a025eebb690d8802b64b
The discard confirm dialog is added alongside the delete confirm dialog
and the dialog opening/closing code is refactored to work on either. The
confirm discard dialog only appears if the draft message is savable.
Feature: Issue 6984
Change-Id: I6d88a8734fa7736f685796790e02437b10c81f00
Because the order of buttons switched on most modals (cancel on left,
save on right) this updates the button order in gr-diff-comment for
consistency. Discard is moved to the left of 'save' and 'edit'.'
Change-Id: Ifa08976b77d99bb32fb1d6388f38843ad9e065fa
This change moves the admin-only commment up above the comment next to
the date and styles it more subtly, saving vertical space.
Bug: Issue 7274
Change-Id: Id3095a5439e33055eec3191f6e31d17bb8640615
It makes the most sense (at least for left-to-right language speaking
locales) to have the most final and most destructive UI elements read
last, as interacting with those is the most permanent, and requires the
full context of the information in the element.
In diff comments, actions like 'Save' and 'Discard' (which affect the
entire comment) were on the left, whereas toggling the resolved state
was on the right. This change flips these alignments.
Bug: Issue 5924
Change-Id: Id00e19d662a983fad071df0fb866052b2001beb4
With this change, toast messages show when diff comments are being saved
and notify when saving is complete, even when the diff view is no-longer
loaded. This signal is intended to encourage users to write diff
comments and feel free to navigate away to view other parts of a change.
With this change, new toast messages are able to replace existing toast
messages.
Feature: Issue 6970
Change-Id: I5973673d85c73b14794cb5e5b21327dd0c27ec1d
Bold fonts were inconsistent across platforms and new design specs use
Roboto Medium instead, so switching all currently bold fonts to use
that.
Change-Id: Ibfc5ffc3fc33517acb85acc0194215b4d7864cae
Because of the way iron-autogrow-textarea works, There will never be
a need to have a vertical scroll bar, as the text area expands
accordingly.
In cases where a horizontal scrollbar appeared (due to long lines of
text), the height of the textarea shrank just enough for a vertical
scrollbar to be added as well. There also appeared to be some browser
quirks in which the scroll bars jumped around when they shouldn't.
In order to prevent this from happening, hide overflow-y.
Additionally, add box-sizing: border-box to all iron-autogrow-textarea
instances, so that scrollbars appear correctly in shadow dom.
Bug: Issue 6500
Change-Id: I28f3b47d656a246decd693f637040748ec4fd3a0
Previously, when you clicked 'reply' on a comment, the cursor would
automatically be hovering the 'DRAFT' text. This change makes it so
that only the ⓘ is hoverable, but not the text itself.
Bug: Issue 5810
Change-Id: I0be8d1e777bcfe68fd4b192290c711fcb1430c3a
The release of eslint 4 introduced some breaking changes -- a full
upgrade guide can be found here [1].
In this change, the "no-useless-escape" and "indent" rules are disabled,
"indent" being replaced with "indent-legacy" to minimize breaks due to
lint. Also, extra spaces before comments and other assorted new lint
issues are corrected.
The long-term plan is to move away from the deprecated "indent-legacy"
rule and also reenable the "no-useless-escape" rule, but for now we
want to minimize breakages and not affect PG developer velocity.
[1] http://eslint.org/docs/user-guide/migrating-to-4.0.0
Change-Id: I8044256c95e3fe3f94fa9acc7922600908f8a0f4
This is a partial roll-forward of c/106190
This replaces all loads of iron-test-helpers with a load of a file
that wraps it, and adds that file to test files that do not currently
load iron-test-helpers.
A future CL will also install polymer-resin via common-test-helpers.html.
I tested by running
$ WCT_ARGS="-l chrome" ./polygerrit-ui/app/run_test.sh
Change-Id: Ifb3cd2c8db13d724f57e56e7e78045470d103a43
- Create a shared style module that is included in every custom element
- Add the shared style module to each existing element
Change-Id: I1ee382955afe4ff630548a6640e7c4d03688849d
polymer-resin intercepts polymer property assignments
before they reach XSS-vulnerable sinks like `href="..."`
and text nodes in `<script>` elements.
This follows the instructions in WORKSPACE for adding a new bower
dependency with kaspern's tweak to use the dependency in a rule so
that it's found. //lib/js/bower_components.bzl has already been
rolled-back per those instructions.
The license is the polymer license as can be seen at
https://github.com/Polymer/polymer-resin/blob/master/LICENSE though
I'm not sure that //tools/js/bower2bazel.py recognizes it as such.
Docs for the added component are available at
https://github.com/Polymer/polymer-resin/blob/master/README.mdhttps://github.com/Polymer/polymer-resin/blob/master/getting-started.md
With this change, when I introduce an XSS vulnerability as below,
polymer-resin intercepts and stops it.
Patch that introduces a strawman vulnerability.
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js
@@ -55,6 +55,10 @@
url: '/q/status:abandoned',
name: 'Abandoned',
},
+ {
+ url: location.hash.replace(/^#/, '') || 'http://example.com/#fragment_echoed_here',
+ name: 'XSS Me',
+ },
],
}];
---
Address kaspern's and paladox's comments.
---
Undo version bumps for bower dependencies.
---
Change Soy index template to parallel app/index.html.
---
update polymer-resin to version 1.1.1-beta
----
Load polymer-resin into polygerrit-ui/**/*_test.html
After this, I ran the tests with
-l chrome
-l firefox
I ran a handful of tests with -p and observed that the
console shows "initResin" is called before test cases start
executing.
These changes were done programmaticly by running the script below
(approximately) thus:
```
gerrit/ $ cd polygerrit-ui/app
app/ $ find . -name \*test.html | xargs perl hack-tests.pl
```
```
use strict;
sub removeResin($) {
my $s = $_[0];
$s =~ s@<link rel="import" href="[^"]*/polymer-resin/[^"]*"[^>]*>\n?@@;
$s =~ s@<script src="[^"]*/polymer-resin/[^"]*"></script>\n?@@;
$s =~ s@<script>\s*security\.polymer_resin.*?</script>\n?@@s;
return $s;
}
for my $f (@ARGV) {
next if $f =~ m@/bower_components/|/node_modules/@;
system('git', 'checkout', $f);
print "$f\n";
my @lines = ();
open(IN, "<$f") or die "$f: $!";
my $maxLineOfMatch = 0;
while (<IN>) {
push(@lines, $_);
# Put a marker after core loading directives.
$maxLineOfMatch = scalar(@lines)
if m@/webcomponentsjs/|/polymer[.]html\b|/browser[.]js@;
}
close(IN) or die "$f: $!";
die "$f missing loading directives" unless $maxLineOfMatch;
# Given ./a/b/c/my_test.html, $pathToRoot is "../../.."
# assuming no non-leading . or .. components in the path from find.
my $pathToRoot = $f;
$pathToRoot =~ s@^\.\/@@;
$pathToRoot =~ s@^(.*?/)?app/@@;
$pathToRoot =~ s@\/[^\/]*$@@;
$pathToRoot =~ s@[^/]+@..@g;
my $nLines = scalar(@lines);
open(OUT, ">$f") or die "$f: $!";
# Output the lines up to the last polymer-resin dependency
# loaded explicitly by this test.
my $before = join '', @lines[0..($maxLineOfMatch - 1)];
$before = removeResin($before);
print OUT "$before";
# Dump out the lines that load polymer-resin and configure it for
# polygerrit.
if (1) {
print OUT qq'<link rel="import" href="$pathToRoot/bower_components/polymer-resin/standalone/polymer-resin-debug.html"/>
<script>
security.polymer_resin.install({allowedIdentifierPrefixes: [\'\']});
</script>
';
}
# Emit any remaining lines.
my $after = join '', @lines[$maxLineOfMatch..$#lines];
$after = removeResin($after);
$after =~ s/^\n*//;
print OUT "$after";
close(OUT) or die "$f: $!";
}
```
---
update polymer-resin to version 1.2.1-beta
---
update Soy index template to new style polymer-resin initialization
----
fix lint warnings
----
Load test/common-test-setup.html into *_test.html
Instead of inserting instructions to load and initialize polymer-resin into
every test file, add a common-test-setup.html that does that and also fold
iron-test-helpers loading into it.
----
imported files do not need to load webcomponentsjs
Change-Id: I71221c36ed8a0fe7f8720c1064a2fcc9555bb8df
In the previous fix, the function was not called and since it was
the function that set the class to 'hidden' it did not hide.
Change-Id: Ic8f908802cb515086860f20e2c82d0ddd06c0278
There was a formatting issue caused by delete buttons in a draft change.
It really does not make sense for the button to be there for drafts
anyway. This change hides the delete button for admins on draft changes.
Bug: Issue 6257
Change-Id: I236eff9db1f0ca76ed658f3048132c9c675ee407
