Closed Bug 1434299 Opened 6 years ago Closed 6 years ago

The “finger pointer” is not displayed over the “enabled” link in about:telemetry

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- verified
firefox60 --- verified

People

(Reporter: emilghitta, Assigned: vprabhu, Mentored)

References

Details

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

Attachments

(1 file)

[Affected versions]:
60.0a1 (BuildId:20180130102929)
59.0b5 (BuildId:20180128191456)
58.0.1 (BuildId:20180128191252)

[Affected platforms]:
Windows 10 64bit.
macOS 10.13.2
Ubuntu 16.04 64bit

[Steps to reproduce]:
1. Launch Firefox with a clean profile.
2. Access the about:telemetry page.
3. Hover over the "enabled" link.

[Expected result]:
The "finger pointer" is displayed.

[Actual result]:
The "finger pointer" is not displayed and the link looks like it can be editable.

[Additional notes]:
For further details regarding this issue please observe following screencast: https://drive.google.com/file/d/1a3NUdKrQQwl-jK-WdXaYnRgdomLQGmC5/view
Blocks: 1432966
Keywords: regression
Seems as though our `href=""` which previously (for years) stood up to make the xhtml link look clickable is now parsed as an absent href attribute, which means we lose the :hover style that would show the pointer/hand.

Maybe if we pull the old href="#" trick this would start working? All else fails we could have it link back to about:telemetry itself.

(( To be clear, the click dispatch that takes the user to the settings pages still works. It's just the appearance that's off ))
Mentor: chutten
Whiteboard: [good first bug][lang=js]
Not critical for 58 uplift. Given [good first bug] status, let's see if we can get this done in 60.
Priority: -- → P3
I am a newbie and interested in working on this. Can I take it up?  @Mike @Georg
Please go ahead! I think the best way to do this might be to create the anchor element using a DOM API (document.createElement), and that way we can avoid the innerHTML use. Please let me know if you have any quesitons.
Assignee: nobody → venkateshprabhu2
Status: NEW → ASSIGNED
(In reply to Chris H-C :chutten from comment #4)
> Please go ahead! I think the best way to do this might be to create the
> anchor element using a DOM API (document.createElement), and that way we can
> avoid the innerHTML use. Please let me know if you have any quesitons.

Chris, 

I have reproduced this and I am able to fix it by referring href to '#' as you have suggested in your previous comment. It works absolutely fine. Should I go ahead with the same? Or, is there a specific reason that you mentioned about creating a new anchor element?
It's excellent news to hear that making it "#" fixes the bug.

We'll soon be asked to remove the innerHTML call (here's the mailing list post: https://mail.mozilla.org/pipermail/firefox-dev/2018-February/006092.html) so since we're in the neighbourhood, we might as well fix it for good :)
(In reply to Chris H-C :chutten from comment #6)
> It's excellent news to hear that making it "#" fixes the bug.
> 
> We'll soon be asked to remove the innerHTML call (here's the mailing list
> post:
> https://mail.mozilla.org/pipermail/firefox-dev/2018-February/006092.html) so
> since we're in the neighbourhood, we might as well fix it for good :)

Chris, I am new to mozilla(opensource in general) and it took me a lot of time to figure out how to approach reading the source code, setting up and building firefox etc., Hence the delay. I am new to javascript as well. I fixed this issue using the following approach:

    convertStringToLink(string) {
    //return "<a href=\"\" class=\"change-data-choices-link\">" + string + "</a>"; //Current line
    //Using dom 
    var a = document.createElement('a');
    a.setAttribute('href','#');
    a.setAttribute('class','change-data-choices-link');
    a.innerHTML = string ;
    var temp = document.createElement('div');
    temp.appendChild(a);
    return temp.innerHTML;
    
  }
    

Is this the way you mentioned? This fixes the bug. Please let me know your thoughts. Thank you.
It is no trouble at all. We are happy you are contributing. I just want you to know that there are people who can help you out. You can, of course, always comment here and I will answer. Or, if you would like something more instant, we have on IRC (https://wiki.mozilla.org/Irc) the channels #introduction and #developers which tend to have people in them at all hours of all days who can help you if you run into difficulties. I don't want you to get stuck and feel isolated. We're here to help!

Thank you for your contribution. The change takes a good approach, creating the anchor and setting the attributes. You can use `textContent` to set the string inside the `a` (to avoid the innerHTML use). And then we have to change... oh dear.

I didn't read far enough ahead. It turns out that the html string from convertStringToLink is being passed into formatStringFromName which complicated matters quite severely. This would turn into rather a more complicated patch than I had thought.

Let's keep it simple.

Please go back to your first patch, the one that just put '#' into place and got it working. That I can review and mark for checkin (Steps 3 and 4 in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction). After your code is in and working, we can file a follow-up bug that can look into replacing the formatStringFromName call with something like BrowserUtils.getLocalizedFragment.

Does this sound good to you?
(In reply to Chris H-C :chutten from comment #8)
> It is no trouble at all. We are happy you are contributing. I just want you
> to know that there are people who can help you out. You can, of course,
> always comment here and I will answer. Or, if you would like something more
> instant, we have on IRC (https://wiki.mozilla.org/Irc) the channels
> #introduction and #developers which tend to have people in them at all hours
> of all days who can help you if you run into difficulties. I don't want you
> to get stuck and feel isolated. We're here to help!
> 
> Thank you for your contribution. The change takes a good approach, creating
> the anchor and setting the attributes. You can use `textContent` to set the
> string inside the `a` (to avoid the innerHTML use). And then we have to
> change... oh dear.
> 
> I didn't read far enough ahead. It turns out that the html string from
> convertStringToLink is being passed into formatStringFromName which
> complicated matters quite severely. This would turn into rather a more
> complicated patch than I had thought.
> 
> Let's keep it simple.
> 
> Please go back to your first patch, the one that just put '#' into place and
> got it working. That I can review and mark for checkin (Steps 3 and 4 in
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Introduction). After your code is in and working, we can file a follow-up
> bug that can look into replacing the formatStringFromName call with
> something like BrowserUtils.getLocalizedFragment.
> 
> Does this sound good to you?

Chris, first of all, thank you for taking so much time to explain everything in the comment. I am happy that new contributors are given so much attention and patience. I have made the change and have submitted the patch for review. I look forward to hear your feedback.
Comment on attachment 8948632 [details]
Bug 1434299 - Changed href to refer to '#' to display "finger pointer" over the "enabled" link in about:telemetry.

https://reviewboard.mozilla.org/r/218046/#review223894

Good job on nailing the commit message first try! Most contributors miss that. :)

I'll send this along to autoland. Your fix ought to be available to the world via Firefox Nightly in a day or so.

...I wonder if we should try uplifting this to Beta so we can get it to more users sooner... Yeah, let's do that.
Attachment #8948632 - Flags: review?(chutten) → review+
(We have to wait for it to hit Nightly so we can verify the bug is fixed before we can ask for it to be "uplifted" to Beta. I got ahead of myself a bit :S )

Thank you for your contribution! Would you like some help finding the next bug you want to work on? If so, let me know what you're interested in doing and maybe I can find something that'll fit.
(In reply to Chris H-C :chutten from comment #12)
> (We have to wait for it to hit Nightly so we can verify the bug is fixed
> before we can ask for it to be "uplifted" to Beta. I got ahead of myself a
> bit :S )
> 
> Thank you for your contribution! Would you like some help finding the next
> bug you want to work on? If so, let me know what you're interested in doing
> and maybe I can find something that'll fit.

Thank you for reviewing. And yes, please help me find my next few bugs(if you come across anything) if possible. For now, I have picked up one more in rel_eng. I can understand python, html, css, js, c and c++. 

Here's the link to the bug I have picked: https://bugzilla.mozilla.org/show_bug.cgi?id=1335161

I am active on irc channels as well for now.
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99f1065086af
Changed href to refer to '#' to display "finger pointer" over the "enabled" link in about:telemetry. r=chutten
Blocks: 1436047
Sure thing! 

(If you'd like to continue working in aboutTelemetry.js, I've filed bug 1436047 to eradicate the innerHTML calls.)
https://hg.mozilla.org/mozilla-central/rev/99f1065086af
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is this worth backporting to 59 or should it ride the 60 train?
Flags: needinfo?(chutten)
Comment on attachment 8948632 [details]
Bug 1434299 - Changed href to refer to '#' to display "finger pointer" over the "enabled" link in about:telemetry.

It's cheap and should apply and we're, what, five weeks out from merge day? Let's take it.

Approval Request Comment
[Feature/Bug causing the regression]: innerHTML sanitization
[User impact if declined]: Wrong pointer styling on about:telemetry link
[Is this code covered by automated tests?]: Nope
[Has the fix been verified in Nightly?]: Works for me on Nightly 2018-02-07
[Needs manual test from QE? If yes, steps to reproduce]:  Not sure. If so, Open about:telemetry, mouse over the blue word after the phrase "Telemetry is collecting pre-release data and upload is"
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Gives a default host to a link anchor, that's all.
[String changes made/needed]: None.
Flags: needinfo?(chutten)
Attachment #8948632 - Flags: approval-mozilla-beta?
Comment on attachment 8948632 [details]
Bug 1434299 - Changed href to refer to '#' to display "finger pointer" over the "enabled" link in about:telemetry.

Fixes a small cosmetic regression. Taking for 59b8.
Attachment #8948632 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This issue is verified fixed using Firefox 60.0a1 (BuildId:20180208103049) on Windows 10 64bit, macOS 10.12.6 and Ubuntu 14.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
This issue is verified fixed using Firefox 59.0b8 (BuildId:20180208193705) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.