Closed Bug 1088257 Opened 5 years ago Closed 5 years ago

Share button label and tooltip are mixed up

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: dao, Assigned: pduguet33, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1084419 +++

The share button uses social-share-button.label as the tooltip and social-share-button.tooltiptext as the label. This makes no sense.

We should remove the label and tooltiptext properties here, because they should be picked up automatically based on the id:

http://hg.mozilla.org/mozilla-central/annotate/d8de0d7e52e0/browser/components/customizableui/CustomizableWidgets.jsm#l396
This is the modified file that does not have the label or tooltip properties for the share button. I was not sure if this was a patch or what.
Attachment #8511211 - Flags: review?(dao)
Comment on attachment 8511211 [details]
This is the updated file with the unnecessary lines (398, 399) of code removed.

You need to create the patch using "hg diff", e.g. "hg diff > ~/mypatch.diff". You can read more about it here: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F

Before you create the patch, make sure Mercurial is set up properly:
https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Basic_configuration
Attachment #8511211 - Flags: review?(dao)
As of my first bug to start up, can u please assign me this bug.
(In reply to Siddartha L K [:siddu] from comment #3)
> As of my first bug to start up, can u please assign me this bug.

Please feel free to just attach a patch, we'll then formally assign the bug to you.
Whiteboard: [good first bug] → [good first bug][lang=js]
This looks like fixed. I don't see the tooltip and label properties.
(In reply to tejdeepg from comment #5)
> This looks like fixed. I don't see the tooltip and label properties.

They're still there as far as I can tell.
Attached patch Fix patch (obsolete) — Splinter Review
Fixed with this patch. My first one so hoping I did correctly :)
Attachment #8522562 - Flags: review?(dao)
Comment on attachment 8522562 [details] [diff] [review]
Fix patch

Looks good, but this patch is using DOS line endings (\r\n) rather than Unix line endings (\n), which prevents it from applying cleanly. I'm not sure how this happened. Do you think you can fix it?
Attachment #8522562 - Flags: review?(dao) → feedback+
This is probably because I made it on ubuntu and transfered it to windows before submitting it. I'm going to fix that and work solely from the VM.
Attachment #8522562 - Attachment is obsolete: true
Attachment #8523008 - Flags: review?(dao)
Comment on attachment 8523008 [details] [diff] [review]
Patch with Unix line endings

Looks good, thanks!
Attachment #8523008 - Flags: review?(dao) → review+
Assignee: nobody → pduguet33
Keywords: checkin-needed
Is there any test to produce or run ? What are the next steps ?
https://hg.mozilla.org/integration/fx-team/rev/9433adad2df8
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
(In reply to Paul Duguet from comment #12)
> Is there any test to produce or run ? What are the next steps ?

Unless your patch causes unexpected problems, we're done here and the fix will appear in one of the next nightly builds.
Attachment #8511211 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9433adad2df8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.