Multi-line comments wrap in inspector view

RESOLVED FIXED in Firefox 43

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: darktrojan, Assigned: Rohan Rawat)

Tracking

unspecified
Firefox 43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [good first bug][lang=js][lang=css][polish-backlog])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8652676 [details]
This looks silly.

Text nodes have newlines replaced with spaces so that they only take up one line in the inspector. Comments should too.
Whiteboard: [good first bug][lang=js][lang=css][polish-backlog]
(Assignee)

Comment 1

2 years ago
I would like to work on this. Where is the relevant code located? I tried browser/devtools/inspector/breadcrumbs.js but didn't find anything.
Flags: needinfo?(geoff)
The code that generates the DOM tree preview in the inspector is browser/devtools/markupview/markup-view.js
There are various MarkupContainer implementations in this file, one for each type of node shown in the inspector, and each of them create the necessary markup using some HTML template in browser/devtools/markupview/markup-view.xhtml.
For comments, see the <span id="template-comment"> in that file. This is the piece of HTML that is used to generate comment nodes in the inspector.

I hope that's enough to dig around by yourself a little more. If not, let me know, I can give more hints about how to solve this bug.
Flags: needinfo?(geoff)
(Assignee)

Comment 3

2 years ago
Created attachment 8654818 [details] [diff] [review]
fix.patch
Attachment #8654818 - Flags: review?(pbrosset)
Comment on attachment 8654818 [details] [diff] [review]
fix.patch

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

This essentially makes comment nodes look like text nodes. I think that's good.
Can you push to TRY to make sure no tests fail because of this (I highly doubt we have a test that checks this kind things, but you never know).
Attachment #8654818 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 5

2 years ago
I have filed a request for level 1 commit access here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1200723

Would you be able to vouch for me ?
Flags: needinfo?(pbrosset)
(In reply to Rohan Rawat from comment #5)
> I have filed a request for level 1 commit access here:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1200723
> 
> Would you be able to vouch for me ?
Don't worry I'll push to try for you for now. Let's keep this request bug open and I'll vouch for you after we've worked a little bit more together. Are you interested in working on other bugs? Feel free to take a look at http://firefox-dev.tools , it should help you find good bugs to work on (bugs that are either mentored or marked as easy). Also, feel free to join our #devtools IRC channel to ask for bugs.
Try build URL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e5c0c3a9db
Assignee: nobody → rohan1395
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Ah one more thing, the patch isn't formatted correctly I think. It's just a diff, it has no commit message nor an author.
This should help: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
and this: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
(Assignee)

Comment 8

2 years ago
Created attachment 8655965 [details] [diff] [review]
Added the 'white-space: normal' rule to the comments template
(Assignee)

Updated

2 years ago
Attachment #8655965 - Flags: review?(pbrosset)
Comment on attachment 8655965 [details] [diff] [review]
Added the 'white-space: normal' rule to the comments template

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

This looks good. Thanks for creating this new patch.
Just one last thing needed: can you add the suffix ", r=<reviewer-username>" to the commit message?
So:
Bug 1198569 - Added the 'white-space: normal' rule to the comments template, r=pbro
Once done, you don't need to re-ask for review, you can simply upload your new patch, mark the old one as obsolete, and then add the keyword "checkin-needed" at the top of the bug. The try build isn't done, but the tests are green on one platform already, and knowing the type of change you made, I'm not expecting anything to go badly.
So just set the flag after you've uploaded the new patch, that will put the bug on the sheriffs' radar and they will check it in the repository.
Attachment #8655965 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8655977 [details] [diff] [review]
Added the 'white-space: normal' rule to the comments template
Attachment #8655977 - Flags: review?(pbrosset)
(Assignee)

Updated

2 years ago
Attachment #8655965 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8655977 - Flags: review?(pbrosset)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
You might have attached the wrong patch, the new one is identical to the previous one. Also you forgot to set the review flag to R+ when attaching it (in the drop-down where you can choose R? to ask someone to review, you can also choose R+).
Keywords: checkin-needed
Attachment #8654818 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Created attachment 8656020 [details] [diff] [review]
Added the 'white-space: normal' rule to the comments template

Sorry about that. I am still trying to figure out bzexport.
(Assignee)

Updated

2 years ago
Attachment #8655977 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
Created attachment 8656021 [details] [diff] [review]
Added the 'white-space: normal' rule to the comments template
(Assignee)

Updated

2 years ago
Attachment #8656020 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8656021 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/bcda316bf139
Keywords: checkin-needed

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
https://hg.mozilla.org/mozilla-central/rev/bcda316bf139
You need to log in before you can comment on or make changes to this bug.