Closed Bug 1336207 Opened 7 years ago Closed 7 years ago

Add tooltip to say what "ev" means

Categories

(DevTools :: Inspector, defect, P3)

53 Branch
defect

Tracking

(firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: jidanni, Assigned: aastha.gupta4104, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

One sees "Button: [ev]" but there is not tooltip to pop up and say what "ev" means or might do.
Same with Bubbing and Domo.
Their current tooltips are just their names. That can never be right.
Good point, the (ev) icon should definitely have a title tooltip so people know what it is about.
And the DOM0/1/2 and Bubbling tags just have silly tooltips.

This is an easy bug if someone wants to work on it. I'm happy to help.
Make sure you go over our contribution guide first: https://developer.mozilla.org/en-US/docs/Tools/Contributing

Once ready, you might be interested in looking at the following code locations:

/devtools/client/inspector/markup/markup.xhtml, in this file, the div.markupview-events element is the little (ev) icon in the inspector. Adding a title attribute to it would work fine.

In devtools/client/shared/widgets/tooltip/EventTooltipHelper.js we create the content of the event tooltips, including the little DOM0/1/2 and Bubbling tags. Here's the exact code position where the title attribute is added:
http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/devtools/client/shared/widgets/tooltip/EventTooltipHelper.js#137
As you can see, the title is just the same text as the tag itself. We should introduce an actual localized string for this instead.

devtools/client/locales/en-US/inspector.properties is the properties file that contains our inspector localized strings. So it would be the right place to add the new titles.

Let me know if you need more help.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Priority: -- → P3
Hi Patrick, 
Thanks for mentoring, I will submit a patch soon, assigning for now, and will let you know if I need anything.
Thanks
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Attached patch added tooltip text for ev button (obsolete) — Splinter Review
Hi,
This is my first patch in Mozilla community.Kindly review it and let me know if any changes are required.
Thanks
Thank you very much for getting involved on the project! Your code changes look pretty good at first glance.
I'm going to pass mentorship of this bug to Mike who knows this part better than I do and will be able to help you and review your code changes.
It would also be good to update the DOM0/1/2/ and Bubbling tooltips, I don't see this being changed in your patch.
Anyway, great start. I'll assign the bug to you now.
Assignee: 3ugzilla → aastha.gupta4104
Mentor: pbrosset → mratcliffe
Attachment #8905288 - Flags: review?(mratcliffe)
(In reply to Aastha from comment #6)
> Hi,
> This is my first patch in Mozilla community.Kindly review it and let me know
> if any changes are required.
> Thanks

Nice work, thanks for contributing.

Do you think you could also remove the DOM0/1/2/ and Bubbling tooltips then re-upload the patch?
Flags: needinfo?(aastha.gupta4104)
Attachment #8905288 - Attachment is obsolete: true
Attachment #8905288 - Flags: review?(mratcliffe)
Flags: needinfo?(aastha.gupta4104)
I didn't realize that you were localizing the tooltips. Just a couple of changes and you are done.

Can you create a single patch with the following changes?

1. Add a tooltip to the ev button.
2. Remove the tooltips from the DOM0 and DOM2 tags because they are the same in every language.
3. Localize the tooltips for Capturing and Bubbling (move them to the .properties file).
Flags: needinfo?(aastha.gupta4104)
1. Added a tooltip to the ev button.
2. Removed the tooltips from the DOM0 and DOM2 tags.
3. Localized the tooltips for Capturing and Bubbling.
Attachment #8905506 - Attachment is obsolete: true
Flags: needinfo?(aastha.gupta4104)
Comment on attachment 8906343 [details] [diff] [review]
Made all the changes that had to be made

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

Perfect, great job!
Attachment #8906343 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/336effc1fe44
Add tooltip to say what "ev" means in the Inspector. r=miker
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2f9e7b7fd7
Add tooltip to say what "ev" means in the Inspector. Follow-up: Fix long line. r=eslint-fix
https://hg.mozilla.org/mozilla-central/rev/336effc1fe44
https://hg.mozilla.org/mozilla-central/rev/0e2f9e7b7fd7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have reproduced this Bug with nightly 54.0a1 (2017-02-02) on Ubuntu 16.04, 64 Bit!

The bug's fix is now verified on latest nightly 57.0a1

Build ID     20170912100139

User Agent   Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170913]
I have successfully reproduced this bug with Nightly 57.0a1 (2017-02-02) on windows 10 (32-bit)

this bug is verified fix with  latest nightly 57.0a1 (2017-09-12) (32-bit)

Build ID: 20170912100139
Mozilla/5.0 (Windows NT 10.0; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170913]
As per Comment 16 and Comment 17, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.