Improve diff comment bubble tooltips to show rendered comment content
Review Request #6932 — Created Feb. 10, 2015 and submitted
Information | |
---|---|
wudi | |
Review Board | |
master | |
f1b0a6b... | |
Reviewers | |
reviewboard, students | |
This is the first part in improving the diff comment bubbles. The tooltip previews for the comment text will show the full rendered comment text together with the name of the reviewer, with the size of the tooltip maximized to the screen width. Updated js tests as the comment reviewer needs to be loaded with each comment.
Ran tests, added markdown and plain text comments to check visually.
Added unit testing for XSS vulnerabilites. Manually checked by creating comments containing script tags for both markdown and plain text, scripts were escaped and not executed for both first load of the page and subsequent asynchronous updating of comments.
Description | From | Last Updated |
---|---|---|
Can we set a maximum width for this popup? The entire width of the page is a little excessive. |
|
|
Can you go through and make sure that the styles of this match exactly what you see on the "View … |
|
|
Col: 80 E501 line too long (89 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (93 > 79 characters) |
![]() |
|
Col: 25 E131 continuation line unaligned for hanging indent |
![]() |
|
This will allow non-Markdown text to inject tags into the page. Make sure you escape the text. |
|
|
Blank line between these. Though, you probably want RB.UserSession.instance instead. |
|
|
Can you make use of Underscore.js templates instead? That will help to keep all this more readable and maintainable. |
|
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
The opening ' characters should all line up, and indentation for the templates should happen within the string rather than … |
|
|
What if the text is neither expected type nor SafeText? |
|
|
This should end in a period, and use the imperative mood ("Return" rather than "Returns"). |
|
|
Have seen the "insert in alphabetical order" comment before, perhaps applicable in this case. |
SU Sunxperous | |
Same here for alphabetical order... |
SU Sunxperous | |
A bit nitpicking, but is it better for row to be $row instead? |
SU Sunxperous | |
I feel the name of the function doesn't clearly describe what this is for. Without the context of this being … |
|
|
"HTML". |
|
|
This isn't the right thing to pass to the comments in the serializedCommentBlocks. You should just be passing a dictionary, … |
|
|
This should stay a standalone function. If you're having a problem with this, then you can instead pull out this.tooltipTemplate … |
|
|
This can be one statement: list.append(tooltipTemplate({ ... })); |
|
|
Leftover debug code? |
|
|
Can you rename list to be $list? |
|
|
Because this is just one statement, it doesn't make a lot of sense to define a new function for it, … |
|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+40 -10) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/diffCommentModel.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/diffCommentModel.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
reviewboard/reviews/context.py (Diff revision 2) Col: 25 E131 continuation line unaligned for hanging indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+44 -10) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/diffCommentModel.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/diffCommentModel.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
Change Summary:
Deserialize html_text_fields in baseCommentModel
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+44 -10) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
Change Summary:
Reduce max-width of tooltip
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+44 -10) |
||||
Added Files: |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
Looking good.
I want to make sure that the text is displayed just like it would be for reviews. I'd make use of the
.rich-text
CSS class for the tooltips, so that all styling is consistent.Also, please add testing (manual and unit tests) on XSS vulnerabilities. Neither rich text nor plain text should be able to inject any tags.
-
reviewboard/reviews/context.py (Diff revision 5) This will allow non-Markdown text to inject tags into the page. Make sure you escape the text.
-
-
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 5) Can you make use of Underscore.js templates instead? That will help to keep all this more readable and maintainable.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+75 -13) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
Change Summary:
Add testing for XSS vulnerabilities
Testing Done: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||
Diff: |
Revision 7 (+92 -13) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+93 -13) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
-
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 8) The opening ' characters should all line up, and indentation for the templates should happen within the string rather than outside of it.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+93 -13) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
Would you mind uploading a screenshot that compares what is shown in the tooltip to what is shown on the reviews page?
Change Summary:
Make the tooltips styling consistent with the styling in the "View Reviews" page
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+90 -16) |
||||
Added Files: |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
I only see one trivial thing left.
-
reviewboard/reviews/markdown_utils.py (Diff revision 10) This should end in a period, and use the imperative mood ("Return" rather than "Returns").
-
-
reviewboard/reviews/tests.py (Diff revision 9) What if the text is neither expected type nor
SafeText
?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+90 -16) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
-
reviewboard/reviews/context.py (Diff revision 11) Have seen the "insert in alphabetical order" comment before, perhaps applicable in this case.
-
-
reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js (Diff revision 11) Just curious, should this not be placed in
beforeEach
, instead of being repeated three times for eachit
block? -
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 11) A bit nitpicking, but is it better for
row
to be$row
instead?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+80 -18) |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/css/pages/search.less reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/css/pages/search.less reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
-
reviewboard/reviews/markdown_utils.py (Diff revision 12) I feel the name of the function doesn't clearly describe what this is for. Without the context of this being in Markdown-related code, it's confusing what this is for.
How about
markdown_render_conditional
? -
-
reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js (Diff revision 12) This isn't the right thing to pass to the comments in the
serializedCommentBlocks
. You should just be passing a dictionary, just like what's passed to the page. aUserSession
is a very different thing. -
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 12) This should stay a standalone function. If you're having a problem with
this
, then you can instead pull outthis.tooltipTemplate
into a variable in_updateTooltip
. -
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 12) This can be one statement:
list.append(tooltipTemplate({ ... }));
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+75 -16) |
||||
Added Files: |

-
Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/ui/base.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/css/pages/search.less reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/ui/base.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/css/pages/search.less reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+75 -16) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/ui/base.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/css/pages/search.less reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/ui/base.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/css/pages/search.less reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js
-
Just a few simple comments left.
-
-
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 14) Can you rename
list
to be$list
? -
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 14) Because this is just one statement, it doesn't make a lot of sense to define a new function for it, even with the small amount of duplication below.
We should also not be calling
gettext()
on the username.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+78 -19) |

-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/ui/base.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/css/pages/search.less reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js Tool: Pyflakes Processed Files: reviewboard/reviews/markdown_utils.py reviewboard/reviews/tests.py reviewboard/reviews/ui/base.py reviewboard/reviews/context.py Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js reviewboard/static/rb/css/pages/search.less reviewboard/static/rb/js/models/abstractCommentBlockModel.js reviewboard/static/rb/js/resources/models/baseCommentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/models/commentEditorModel.js