Closed Bug 974638 Opened 6 years ago Closed 6 years ago

Stylesheet selector should display full href of stylesheet in a tooltip

Categories

(DevTools :: Style Editor, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: jwalker, Assigned: sayan.chowdhury2012)

Details

(Whiteboard: [good first bug][mentor=fayearthur@gmail.com][lang=js])

Attachments

(1 file, 3 obsolete files)

Helps distinguish when a page has several links to files called index.css with different paths (as bugzilla does)
Whiteboard: [good first bug][mentor=fayearthur@gmail.com][lang=js]
Hi,

I want to work on this bug. Can you give some pointers on how to start?

Regards,
Sayan
Hi Sayan, I'm glad you're interested. Here's some info about getting started with developer tools development in general: https://wiki.mozilla.org/DevTools/GetInvolved.

You'll probably want to add the code to this function here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm#588.

Also feel free to ask me any questions you have on irc.mozilla.org on the #devtools channel, I'm 'harth'.
(In reply to Mahdi Dibaiee [:mdibaiee] from comment #3)
> Created attachment 8379739 [details] [diff] [review]
> Bug 974638 - Stylesheets have a tooltip showing full url

Mahdi, Sayan already expressed interest in working on this bug. I'd like to give them the opportunity to work on it because they were first.
Attachment #8379739 - Flags: review?(fayearthur)
(In reply to Heather Arthur [:harth] from comment #4)
> (In reply to Mahdi Dibaiee [:mdibaiee] from comment #3)
> > Created attachment 8379739 [details] [diff] [review]
> > Bug 974638 - Stylesheets have a tooltip showing full url
> 
> Mahdi, Sayan already expressed interest in working on this bug. I'd like to
> give them the opportunity to work on it because they were first.

Oh, I'm really sorry, I didn't notice it :/

I wish you the best.
Assignee: nobody → sayan.chowdhury2012
Comment on attachment 8381915 [details] [diff] [review]
Bug 974638 - Stylesheet selector should display full href of stylesheet in a tooltip

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

Thanks Sayan. Just one thing.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +544,5 @@
>      this._view.setItemClassName(summary, flags.join(" "));
>  
>      let label = summary.querySelector(".stylesheet-name > label");
>      label.setAttribute("value", editor.friendlyName);
> +    label.setAttribute("tooltiptext", editor.styleSheet.href);

Sometimes the `href` is null, like on an inline style sheet (see http://google.com).

We shouldn't show have a tooltip in that case.
Attachment #8381915 - Flags: review?(fayearthur)
I added a condition to check for href's containing null values.
Attachment #8381915 - Attachment is obsolete: true
Attachment #8382536 - Flags: review?(fayearthur)
Comment on attachment 8382536 [details] [diff] [review]
Bug 974638 - Stylesheet selector should display full href of stylesheet in a tooltip

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

Thanks for the update. One note on that, don't worry about uploading a new patch, I can just add the change before checking in.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +544,5 @@
>      this._view.setItemClassName(summary, flags.join(" "));
>  
>      let label = summary.querySelector(".stylesheet-name > label");
>      label.setAttribute("value", editor.friendlyName);
> +    if (editor.styleSheet.href !== null) {

don't need this strong check, probably shouldn't display "" or undefined too.
Attachment #8382536 - Flags: review?(fayearthur) → review+
(In reply to sayan.chowdhury2012 from comment #8)
> Created attachment 8382536 [details] [diff] [review]
> Bug 974638 - Stylesheet selector should display full href of stylesheet in a
> tooltip
> 
> I added a condition to check for href's containing null values.

One more note about this. This patch looks like it has to be applied on top of the original patch. You should upload one patch that applies to the tree. You can achieve this by changing the code and using 'hg qref' to modify your original patch. Cheers.
Attached patch To check inSplinter Review
Rebased patch ready for check in.

Thanks again Sayan.
Attachment #8379739 - Attachment is obsolete: true
Attachment #8382536 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/e36f5045d217
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=fayearthur@gmail.com][lang=js] → [good first bug][mentor=fayearthur@gmail.com][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e36f5045d217
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=fayearthur@gmail.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=fayearthur@gmail.com][lang=js]
Target Milestone: --- → Firefox 30
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.