Skip to content

Conversation

@abhi-agg
Copy link
Contributor

@abhi-agg abhi-agg commented Feb 26, 2024

This PR adds capability to record element clicks via Glean's builtin element click recording api (in mozilla/glean.js#1848).

Scenarios tested:

  1. GleanMetrics.recordElementClick() api and
  2. automatic element click event recording (setting data-glean-label attribute for Back to top button)

Steps to test:

  1. Running npm start on local machine
  2. Setting window.Glean.setLogPings(true) in console
  3. Clicking on various buttons and checking in the console logs that glean.element_click ping is sent for each of the button click

EDIT: We are more interested in testing the automatic click event feature. Changed PR accordingly. See #154 (comment) for details.

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

@abhi-agg abhi-agg force-pushed the glean-click-events branch 2 times, most recently from 417372a to 558bb63 Compare February 26, 2024 14:58
 - Manually call the builtin api provided by Glean to record
   element click events
 - Added data-element-label attribute for "Back to top" button
   to automatically record clicks on it
@abhi-agg abhi-agg changed the title Record element clicks via builtin glean api Record element clicks via glean's builtin element click recording feature Feb 26, 2024
@Dexterp37 Dexterp37 removed their request for review February 26, 2024 15:35
Copy link
Contributor

@rosahbruno rosahbruno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More generally, why are we choosing to use the GleanMetrics.recordElementClick in some places and then in others we are using data-glean-label?

Is there a reason we can't use data-glean-label in all the places?

@abhi-agg
Copy link
Contributor Author

abhi-agg commented Feb 27, 2024

More generally, why are we choosing to use the GleanMetrics.recordElementClick in some places and then in others we are using data-glean-label?

Is there a reason we can't use data-glean-label in all the places?

@rosahbruno I just wanted to use both options in the code i.e. the automatic event capturing as well as calling manual apis. Apart from this, no other reason.

If you think we should switch to using just one option, I am fine with that as well. I have no strong opinion on this one 👍🏾

@rosahbruno
Copy link
Contributor

@abhi-agg
Personally, I think we should try and avoid using the manual events if possible. They are the most similar to existing click events, so we know they are going to work like normal. I am more interested in seeing the behavior around the automatic events and making sure we don't run into any issues there.

 - Removed all click event recording API calls
 - Automatic element click recording is not working on some
   elements, hence reverted back to using manual click recording
@abhi-agg
Copy link
Contributor Author

abhi-agg commented Feb 29, 2024

@rosahbruno I had made the switch to automatically capture clicks (i.e. via data-glean-label attribute) at all places. However, during local testing I found out that clicks on some of the elements are not being recorded this way.

I investigated the issue and figured out that <div> tag is somehow creating problems in these cases (please see NavBar.js, Breadcrumbs.js and ReadMore.js for details). However, resolving this issue will take a bit more time. Therefore, in favor of not blocking the Dashboards work, I have done following:

  1. Used automatic click capturing at all possible places where it works
  2. Manually called click recording API where automatic click capturing fails and resolve this in a follow up task (filed https://bugzilla.mozilla.org/show_bug.cgi?id=1882747)

Please let me know if that sounds good with you.

@abhi-agg abhi-agg requested a review from rosahbruno February 29, 2024 11:00
@abhi-agg abhi-agg changed the title Record element clicks via glean's builtin element click recording feature Record clicks via glean's builtin element click recording feature Feb 29, 2024
Copy link
Contributor

@rosahbruno rosahbruno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good. I think as long as we plan to work on those follow ups, we should be good here. I'm not sure why the div tags are causing issues, but I can take a look as well.

@abhi-agg abhi-agg merged commit f7ae938 into mozilla:main Mar 4, 2024
@abhi-agg abhi-agg deleted the glean-click-events branch March 4, 2024 09:18
@abhi-agg
Copy link
Contributor Author

abhi-agg commented Mar 4, 2024

@rosahbruno I found out the issue and I believe I know how to fix it. I will update the details in the ticket https://bugzilla.mozilla.org/show_bug.cgi?id=1882747 today asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants