Summary

Fixed text fields that have the wrong cursor displayed when hovered over by the cursor

Review Request #8403 — Created Sept. 17, 2016 and submitted Oct. 25, 2016, 9:55 p.m.

Information

Owner

larmiej

Repository

Review Board

Branch

master

Bugs

4432

Depends On

Commit

7489e4b...

Reviewers

Groups

reviewboard, students

People

Description

The cursor of some text fields in an edit state displayed the standard
arrow instead of the text cursor when the cursor hovered over the text
fields. Fixed text fields by setting the cursor style to text in the
Code Mirror css to ensure we have the right cursor being displayed to
the user.

Testing Done


Opened the review dialog      
Added a review header
Hovered over non-text lines
Ensure standard arrow is not diplayed on hover
Also, in the review Dialog, Clicked on "Add Comment"
Added a single line comment
Hovered over non-text lines
Ensure standard arrow is not diplayed on hover

Issues

Description	From	Last Updated
Can you wrap your description and testing done at 72 chars?	brennie	Sept. 23, 2016, 8:12 a.m.
Can you try to limit your summary to 50 chars? (Git doesnt like it when it goes over.)	brennie	Sept. 23, 2016, 8:12 a.m.
"non-text" in your summary Your summary should also be in the imperitive mood, i.e., "Fix text fields..."	brennie	Oct. 6, 2016, 3:39 p.m.
This is not the fix we were hoping for. We like the minimum height when the fields are opened. What …	david	Sept. 20, 2016, 7:13 p.m.
'formataddr' imported but unused	reviewbot	Sept. 20, 2016, 7:15 p.m.
'django_reset' imported but unused	reviewbot	Sept. 20, 2016, 7:15 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 20, 2016, 7:15 p.m.
This file comes from a third party library, and we probably shouldn't make changes to it (since we might drop …	david	Sept. 24, 2016, 7:51 a.m.

reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 1)

Show all issues

This is not the fix we were hoping for. We like the minimum height when the fields are opened.

What we want instead is to change the CSS to ensure we have the right cursor.

Additionally, once you fix that, the summary and description need a lot of love. See https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ for an explanation of what we look for in these, along with examples.

Something that might be making this confusing to diagnose is that there's a regression in the current release-3.0.x branch where the last text field on the review request page isn't taking up the correct height (it's supposed to fill the available height). Most text fields won't show this issue.

What you can do is go to the review dialog and add a review text header. Add a few words on one line. Note how the mouse cursor, when hovering over the first line, has a standard "I" cursor, but when hovering on the following lines (where there's not yet any text), the cursor is an arrow.

What you should be doing in this change is ensuring that the appropriate element in the text field (which uses CodeMirror, a text editor component) has the correct cursor type (text) by default. You'll have to investigate the elements that comprise the field and try to find the most suitable parent element for this. You won't want to guess, but rather try to determine what each element means and is for, and put the CSS rule in the correct place for it.

Change Summary:

Added changes to css for CodeMirror text field.
Please Note that I updated my branch with master and therefore that resulted in multiple code diff
My change only pertains to the file reviewboard/static/lib/css/codemirror.css

Summary:

-	fixing error of text fields in edit states having the wrong cursor when hovering over non-text lines by setting default minimum height of text field to 0
+	Text fields in edit states have the wrong cursor when hovering over non-text lines

Description:

~		fixing error of text fields in edit states having the wrong cursor when hovering over non-text lines by setting default minimum height of text field to 0
	~	The cursor of some text fields in an edit state displayed the standard arrow instead of the text cursor when the cursor hovered over non-text lines.
	+
	+	This change sets the cursor style to text in the Code Mirror css to ensure we have the right cursor being displayed to the user when hovering over the non-text lines.

Testing Done:

~		Checked text fields to ensure non-text lines do not appear. And text fields have a single line default entry.
	~	Opened the review dialog
	+	Added a review header
	+	Hovered over non-text lines
	+	Ensure standard arrow is not diplayed on hover
	+	Also, in the review Dialog, Clicked on "Add Comment"
	+	Added a single line comment
	+	Hovered over non-text lines
	+	Ensure standard arrow is not diplayed on hover

Commit:

-	0bd789df234f1519d521726a9163db39be38edb6
+	b660a61be9a7f9fbb15951e2df5f63e1e2428ac0

Diff:

Revision 2 (+908 -284)

Show changes

	reviewboard/settings.py
	reviewboard/staticbundles.py
	reviewboard/accounts/trophies.py
	reviewboard/accounts/forms/auth.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests.py
	reviewboard/extensions/tests.py
	reviewboard/notifications/email.py
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/testing/testcase.py
    reviewboard/reviews/ui/image.py
    reviewboard/notifications/tests.py
    reviewboard/extensions/tests.py
    reviewboard/reviews/ui/tests.py
    reviewboard/accounts/trophies.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/settings.py
    reviewboard/notifications/email.py
    reviewboard/staticbundles.py
    reviewboard/accounts/forms/auth.py

Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/views/regionCommentBlockView.es6.js
    reviewboard/static/rb/js/models/imageReviewableModel.es6.js
    reviewboard/static/lib/css/codemirror.css
    reviewboard/static/rb/css/pages/review-request.less
    reviewboard/static/rb/js/models/imageReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/testing/testcase.py
    reviewboard/reviews/ui/image.py
    reviewboard/notifications/tests.py
    reviewboard/extensions/tests.py
    reviewboard/reviews/ui/tests.py
    reviewboard/accounts/trophies.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py
    reviewboard/settings.py
    reviewboard/notifications/email.py
    reviewboard/staticbundles.py
    reviewboard/accounts/forms/auth.py

Ignored Files:
    reviewboard/static/rb/css/pages/image-review-ui.less
    reviewboard/static/rb/js/views/regionCommentBlockView.es6.js
    reviewboard/static/rb/js/models/imageReviewableModel.es6.js
    reviewboard/static/lib/css/codemirror.css
    reviewboard/static/rb/css/pages/review-request.less
    reviewboard/static/rb/js/models/imageReviewableModel.js
    reviewboard/static/rb/js/views/imageReviewableView.es6.js

brennie Sept. 20, 2016, 6:20 p.m.

Once you've fixed up the issues from Review Bot, make sure to mark them as fixed or dropped (when appropriate).

reviewboard/notifications/email.py (Diff revision 2)
Show all issues
```
 'formataddr' imported but unused
```
reviewboard/settings.py (Diff revision 2)
Show all issues
```
 'django_reset' imported but unused
```

reviewboard/settings.py (Diff revision 2)

Show all issues

 'from settings_local import *' used; unable to detect undefined names

You'll need to make sure your branch is cleanly rebased onto master. Don't pull master into your branch. Rather, do:

$ git checkout master
$ git pull origin master
$ git checkout your-feature-branch
$ git rebase master

If you pulled master into your branch, you might have some issues cleaning some of that up. If so, come to us in Slack for help.

Change Summary:

Reverted merge and did a rebase on master

Commit:

-	b660a61be9a7f9fbb15951e2df5f63e1e2428ac0
+	10e429add952e0ac250b92f2b641f05a24df6dfe

Diff:

Revision 3 (+1)

Show changes

reviewboard/static/lib/css/codemirror.css

Tool: Pyflakes
Ignored Files:
    reviewboard/static/lib/css/codemirror.css



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/lib/css/codemirror.css

Ship it!

```
Can you wra
```

Show all issues

Can you wrap your description and testing done at 72 chars?

Show all issues

Can you try to limit your summary to 50 chars? (Git doesnt like it when it goes over.)

Summary:

-	Text fields in edit states have the wrong cursor when hovering over non-text lines
+	Text fields having wrong cursor over nontext line

Description:

~		The cursor of some text fields in an edit state displayed the standard arrow instead of the text cursor when the cursor hovered over non-text lines.
	~	This change sets the cursor style to text in the Code Mirror css.
-
-		This change sets the cursor style to text in the Code Mirror css to ensure we have the right cursor being displayed to the user when hovering over the non-text lines.

Testing Done:

~		Opened the review dialog
~		Added a review header
~		Hovered over non-text lines
	~	Added a review header
	~	Hovered over non-text lines
	~	Ensure standard arrow is not diplayed on hover
-		Ensure standard arrow is not diplayed on hover
-		Also, in the review Dialog, Clicked on "Add Comment"
-		Added a single line comment
-		Hovered over non-text lines
-		Ensure standard arrow is not diplayed on hover

The code looks okay I believe (though there might be a more specific CSS selector that should be used for this), but the Description should be able to tell a story about what the problem is, so that a person doesn't have to go to the bug report or review request or dig into code.

I like to follow a "What was wrong, why, and what did we do about it" form. We have a guide and examples here: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

Description:

~		This change sets the cursor style to text in the Code Mirror css.
	~	The cursor of some text fields in an edit state
	+	displayed the standard arrow instead of the text
	+	cursor when the cursor hovered over non-text lines.
	+	This change sets the cursor style to text in the
	+	Code Mirror css to ensure we have the right cursor
	+	being displayed to the user when hovering over the
	+	non-text lines.

Testing Done:

~		Added a review header
~		Hovered over non-text lines
~		Ensure standard arrow is not diplayed on hover
	~	Opened the review dialog
	~	Added a review header
	~	Hovered over non-text lines
	+	Ensure standard arrow is not diplayed on hover
	+	Also, in the review Dialog, Clicked on "Add Comment"
	+	Added a single line comment
	+	Hovered over non-text lines
	+	Ensure standard arrow is not diplayed on hover

Description:

~		The cursor of some text fields in an edit state
~		displayed the standard arrow instead of the text
~		cursor when the cursor hovered over non-text lines.
~		This change sets the cursor style to text in the
~		Code Mirror css to ensure we have the right cursor
	~	The cursor of some text fields in an edit state displayed the standard
	~	arrow instead of the text cursor when the cursor hovered over
	~	non-text lines.This change sets the cursor style to text in the Code
	~	Mirror css to ensure we have the right cursor being displayed to the
	~	user when hovering over the non-text lines.
-		being displayed to the user when hovering over the
-		non-text lines.

Description:

		The cursor of some text fields in an edit state displayed the standard
		arrow instead of the text cursor when the cursor hovered over
~		non-text lines.This change sets the cursor style to text in the Code
	~	non-text lines. This change sets the cursor style to text in the Code
		Mirror css to ensure we have the right cursor being displayed to the
		user when hovering over the non-text lines.

reviewboard/static/lib/css/codemirror.css (Diff revision 3)

Show all issues

This file comes from a third party library, and we probably shouldn't make changes to it (since we might drop in a replacement when we update the version of CodeMirror that we use).

We have some style overrides for .CodeMirror in reviewboard/static/rb/css/pages/reviews.less. Can you make this change there, instead?

Commit:

-	10e429add952e0ac250b92f2b641f05a24df6dfe
+	7489e4bd447bcee8f713baf8ca1d0e31c96cd6d3

Diff:

Revision 4 (+1)

Show changes

reviewboard/static/rb/css/pages/reviews.less

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/css/pages/reviews.less

Show all issues

"non-text" in your summary

Your summary should also be in the imperitive mood, i.e., "Fix text fields..."

david Oct. 5, 2016, 10:40 p.m.

It doesn't look like you updated the summary as requested.

larmiej Oct. 6, 2016, 3:39 p.m.

Yeah, I realized I updated my description instead. Thanks for catching that @David

Description:

		The cursor of some text fields in an edit state displayed the standard
~		arrow instead of the text cursor when the cursor hovered over
~		non-text lines. This change sets the cursor style to text in the Code
~		Mirror css to ensure we have the right cursor being displayed to the
~		user when hovering over the non-text lines.
	~	arrow instead of the text cursor when the cursor hovered over the text
	~	fields. Fixed text fields by setting the cursor style to text in the
	~	Code Mirror css to ensure we have the right cursor being displayed to
	~	the user.

Summary:

-	Text fields having wrong cursor over nontext line
+	Fixed text fields that have the wrong cursor displayed when hovered over by the cursor

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (8c043ea)