Closed
Bug 970962
Opened 11 years ago
Closed 11 years ago
[markup view] cursor goes caret when over inter-attribute and node-attribute spaces
Categories
(DevTools :: Inspector, defect, P2)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: jwalker, Assigned: lviknesh)
Details
(Whiteboard: [good first bug][lang=css][mentor=pbrosset])
Attachments
(1 file, 4 obsolete files)
643 bytes,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
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
Updated•11 years ago
|
Priority: -- → P2
Updated•11 years ago
|
Whiteboard: [good first bug][lang=css][mentor=pbrosset]
Assignee | ||
Comment 1•11 years ago
|
||
Hai Patrick ,
Can you help me with this :)
Comment 2•11 years ago
|
||
(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•11 years ago
|
||
Attachment #8420335 -
Flags: feedback?(pbrosset)
Comment 4•11 years ago
|
||
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•11 years ago
|
||
Attachment #8420929 -
Flags: review?(pbrosset)
Comment 6•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
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•11 years ago
|
||
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 9•11 years ago
|
||
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•11 years ago
|
||
Comment 11•11 years ago
|
||
Comment on attachment 8421218 [details] [diff] [review]
cursor.patch
Review of attachment 8421218 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM
Attachment #8421218 -
Flags: review+
Comment 12•11 years ago
|
||
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+
Updated•11 years ago
|
Assignee: nobody → lviknesh
Keywords: checkin-needed
Comment 13•11 years ago
|
||
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
Comment 14•11 years ago
|
||
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
Comment 16•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css][mentor=pbrosset] → [good first bug][lang=css][mentor=pbrosset][fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 11 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•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•