[markup view] cursor goes caret when over inter-attribute and node-attribute spaces

RESOLVED FIXED in Firefox 32

Status

P2
normal
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: jwalker, Assigned: lviknesh)

Tracking

Trunk
Firefox 32

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=css][mentor=pbrosset])

Attachments

(1 attachment, 4 obsolete attachments)

STR:

1. Find an element with attributes in the markup view.
2. Mouseover the open tag
3. Move your mouse right until it's over the space after the tag name

Expected results:

* The cursor remains as a pointer because nothing different happens when you click

Actual results

* Cursor shows a caret
Priority: -- → P2
Whiteboard: [good first bug][lang=css][mentor=pbrosset]
(Assignee)

Comment 1

4 years ago
Hai Patrick , 
   Can you help me with this :)
(In reply to vikneshwar from comment #1)
> Hai Patrick , 
>    Can you help me with this :)

This should be a simple CSS fix. There are 2 main CSS files for the markup-view: one that controls theme-related things (colors), and the other one that controls the general layout of the panel.
I think this fix should go in the latter one, because this isn't going to be different depending on the selected theme.

This file is: browser/devtools/markupview/markup-view.css

In this file, we need to make sure the 'default' cursor is used in the markup-view.
I think something like this should work `.editor .open, .editor .close {cursor: default;}`
(Assignee)

Comment 3

4 years ago
Created attachment 8420335 [details] [diff] [review]
cursor-markupview.patch
Attachment #8420335 - Flags: feedback?(pbrosset)
Comment on attachment 8420335 [details] [diff] [review]
cursor-markupview.patch

Review of attachment 8420335 [details] [diff] [review]:
-----------------------------------------------------------------

Please test your patch before submitting it. This one doesn't work.

::: browser/themes/shared/devtools/markup-view.css
@@ +32,5 @@
>  
>  .tag-line {
>    padding-left: 2px;
>  }
> +.editor .open , .editor . close 

3 formatting details:
- remove the trailing whitespace,
- put the rule opening brace '{' on the line of the selector, like all the other rules,
- add an empty line in between all rules

1 syntax error:
- there shouldn't be a space in '. close', this will cause the selector to be invalid and it won't apply.
This means you're not using a text editor with proper syntax highlighting, otherwise you'd have seen it.
Also, this means you haven't tested this patch at all, you'd have seen that the bug wasn't fixed.

Any patch you submit should be thoroughly tested, at the very least manually, but just building again and checking yourself if things look good. And normally, automated non-regression tests should also be written, although in this case I don't think that's required.
Attachment #8420335 - Flags: feedback?(pbrosset)
(Assignee)

Comment 5

4 years ago
Created attachment 8420929 [details] [diff] [review]
cursor-markup-view.patch
Attachment #8420929 - Flags: review?(pbrosset)
Comment on attachment 8420929 [details] [diff] [review]
cursor-markup-view.patch

Review of attachment 8420929 [details] [diff] [review]:
-----------------------------------------------------------------

Much better :) thanks.
I noticed one bug though:
the cursor is fine as long as the node is collapsed, but if you expand a node and hover over the close tag, then the cursor is a caret where it should be default instead.
Attachment #8420929 - Flags: review?(pbrosset)
I think '.tag-line .open, .tag-line .close' is more appropriate for the selector.
Also, less important but the cursor also goes caret over comment nodes. It should be easy to fix it while we're at it in this bug. You can check markup-view.xul to see what classes comment nodes have on them.
(Assignee)

Comment 8

4 years ago
Created attachment 8421199 [details] [diff] [review]
default-cursor-markupview.patch

Added CSS properties to use default cursor for comment node and close tags 
Please , tell me if any more changes are needed . Thank you :)
Attachment #8421199 - Flags: feedback?(pbrosset)
Comment on attachment 8421199 [details] [diff] [review]
default-cursor-markupview.patch

Review of attachment 8421199 [details] [diff] [review]:
-----------------------------------------------------------------

As said in comment 4, it looks like you haven't tested this patch much. If after adding '#template-comment {...}' in the code you had tested, you'd have seen that it had no effect, and would have therefore removed it from the patch. The same applies for '.comment' and '.theme-comment', you'd have seen that adding both had the same effect as just adding one.

Please test your changes! There's no point asking for F? or R? for something you haven't tested and are not sure about.

::: browser/devtools/markupview/markup-view.css
@@ +165,5 @@
>    background: rgba(205,205,255,0.2);
>    outline: 1px solid transparent;
>  }
> +
> +.editor .open, .editor .close {

Why did you keep this selector, you already have '.tag-line .open, .tag-line .close' further below which is what you need to use. This rule isn't needed.

@@ +173,5 @@
> +.comment {
> +  cursor: default;
> +}
> +
> +#template-comment {

There's no element with ID template-comment that is inserted in the markup-view. This rule isn't needed.

@@ +177,5 @@
> +#template-comment {
> +  cursor: default;
> +}
> +
> +.theme-comment {

You already have '.comment' which is enough to cover comment nodes, no need for '.theme-comment' here.

@@ +181,5 @@
> +.theme-comment {
> +  cursor: default;
> +}
> +
> +.tag-line .open, .tag-line .close {

Since all these rules are for doing the same thing, please group all the selectors in the same rule, separated by commas:

.tag-line .open, .tag-line .close, .comment {
  cursor: default;
}

that's 3 lines instead of the 15 lines you added.
Attachment #8421199 - Flags: feedback?(pbrosset)
(Assignee)

Comment 10

4 years ago
Created attachment 8421218 [details] [diff] [review]
cursor.patch
Comment on attachment 8421218 [details] [diff] [review]
cursor.patch

Review of attachment 8421218 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8421218 - Flags: review+
Created attachment 8421249 [details] [diff] [review]
cursor.patch

Fixing the commit message and marking all other patches as obsolete.
Attachment #8420335 - Attachment is obsolete: true
Attachment #8420929 - Attachment is obsolete: true
Attachment #8421199 - Attachment is obsolete: true
Attachment #8421218 - Attachment is obsolete: true
Attachment #8421249 - Flags: review+
Assignee: nobody → lviknesh
Keywords: checkin-needed
Hi,
could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
Sorry about that. I completely forgot. Probably because the patch was only CSS related, I didn't think of it.
Here it is: https://tbpl.mozilla.org/?tree=Try&rev=a7526363a529
Try looks green.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/557bf7ef4fc4
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css][mentor=pbrosset] → [good first bug][lang=css][mentor=pbrosset][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/557bf7ef4fc4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][mentor=pbrosset][fixed-in-fx-team] → [good first bug][lang=css][mentor=pbrosset]
Target Milestone: --- → Firefox 32

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.