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)

defect

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)

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]
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;}`
Attached patch cursor-markupview.patch (obsolete) — Splinter Review
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)
Attached patch cursor-markup-view.patch (obsolete) — Splinter Review
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.
Attached patch default-cursor-markupview.patch (obsolete) — Splinter Review
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)
Attached patch cursor.patch (obsolete) — Splinter Review
Comment on attachment 8421218 [details] [diff] [review] cursor.patch Review of attachment 8421218 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8421218 - Flags: review+
Attached patch cursor.patchSplinter Review
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
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: