Closes #3824: Adding the site tracking protection icon on the toolbar #3906

Amejia481 · 2019-07-26T05:30:41Z

Also closes #3920.

There are some states that we are not handling yet as I'm not quite sure how we are going to do that, they could be addressed in future iterations, but if you have any suggestions please let me know.

The state that we are not handling yet:

Tracking protection panel is opened: We are not going to do that.
Tracking protection is disabled for a specific site: The TP icon in the toolbar have to change when this happens. AC doesn't own this list.

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.

codecov · 2019-07-28T12:33:46Z

Codecov Report

Merging #3906 into master will decrease coverage by 1.96%.
The diff coverage is 93.65%.

@@             Coverage Diff              @@
##             master    #3906      +/-   ##
============================================
- Coverage     82.55%   80.58%   -1.97%     
+ Complexity     3660     3572      -88     
============================================
  Files           481      483       +2     
  Lines         15686    15757      +71     
  Branches       2347     2280      -67     
============================================
- Hits          12949    12698     -251     
- Misses         1758     2137     +379     
+ Partials        979      922      -57

Impacted Files	Coverage Δ	Complexity Δ
...java/mozilla/components/concept/toolbar/Toolbar.kt	`85.33% <ø> (ø)`	`0 <0> (ø)`	⬇️
...lla/components/feature/toolbar/ToolbarPresenter.kt	`86.48% <90.9%> (+1.87%)`	`28 <6> (+7)`	⬆️
...zilla/components/browser/toolbar/BrowserToolbar.kt	`91.91% <93.1%> (+0.14%)`	`87 <11> (+11)`	⬆️
...mponents/browser/toolbar/display/DisplayToolbar.kt	`94% <94.18%> (-0.16%)`	`56 <20> (+20)`
...ts/service/glean/scheduler/MetricsPingScheduler.kt	`87.14% <0%> (-11.43%)`	`21% <0%> (-1%)`
...ents/feature/media/session/MediaSessionCallback.kt	`16.66% <0%> (-8.34%)`	`1% <0%> (ø)`
...mozilla/components/feature/media/ext/MediaState.kt	`60% <0%> (-6.67%)`	`0% <0%> (ø)`
...a/mozilla/components/browser/storage/sync/Types.kt	`85% <0%> (-6.49%)`	`0% <0%> (ø)`
...mozilla/components/feature/media/state/MediaMap.kt	`95.45% <0%> (-4.55%)`	`14% <0%> (-1%)`
...zilla/components/feature/media/focus/AudioFocus.kt	`86.66% <0%> (-1.71%)`	`13% <0%> (+1%)`
... and 67 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3e5bc2...b935646. Read the comment docs.

ekager · 2019-07-29T17:26:57Z

components/browser/toolbar/src/main/res/values/strings.xml

@@ -6,6 +6,10 @@
    <!-- Content description (not visible, for screen readers etc.): Description for the overflow menu button in the browser toolbar. -->
    <string name="mozac_browser_toolbar_menu_button">Menu</string>
    <string name="mozac_clear_button_description">Clear</string>
+    <!-- Content description: For the tracking protection toolbar icon, it is set when the site has tracking protection enabled, but none trackers have been blocked or detected. -->
+    <string name="mozac_browser_toolbar_content_description_tracking_protection_on">Tracking Protection is on</string>


Where did these content descriptions come from?

I added them. As every icon means a tracking protection state, we want to communicate that to the accessibility tools like screen readers. I'm not quite sure if they are completely meaningful for these I issued #3925 for investigating about it.

CC @mheubusch

components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/BrowserToolbar.kt

pocmo · 2019-08-01T14:18:20Z

components/browser/toolbar/src/main/res/values/strings.xml

@@ -6,6 +6,10 @@
    <!-- Content description (not visible, for screen readers etc.): Description for the overflow menu button in the browser toolbar. -->
    <string name="mozac_browser_toolbar_menu_button">Menu</string>
    <string name="mozac_clear_button_description">Clear</string>
+    <!-- Content description: For the tracking protection toolbar icon, it is set when the site has tracking protection enabled, but none trackers have been blocked or detected. -->
+    <string name="mozac_browser_toolbar_content_description_tracking_protection_on">Tracking Protection is on</string>


CC @mheubusch

components/concept/toolbar/src/main/java/mozilla/components/concept/toolbar/Toolbar.kt

components/feature/toolbar/src/main/java/mozilla/components/feature/toolbar/ToolbarPresenter.kt

pocmo · 2019-08-01T14:24:41Z

components/feature/toolbar/src/main/java/mozilla/components/feature/toolbar/ToolbarPresenter.kt

    }

    override fun onUrlChanged(session: Session, url: String) {
        renderer.post(url)
+        updateToolbarTrackingProtection(session)


Why do we need that here and why isn't that covered by onTrackerBlockingEnabledChanged and onTrackerBlocked?

I was seeing a issue where I switched from a site that has trackers to a site that doesn't have trackers and wasn't getting notified. I will do some further testing to see if there is any relation

If some reseting is needed when navigating then this probably needs to happen in EngineObserver so that everyone benefits from it and doesn't need to listen to onUrlChanged().

Now, I realized exactly why do we need this here.

onTrackerBlockingEnabledChanged is called when we change tracking protection to ON or OFF.

and

onTrackerBlocked is called when a new tracker is blocked.

But when I have a session that has tracking protection on and has some trackers blocked then I navigate to a new page that doesn't change trackerBlockingEnabled nor has trackers on it. I don't have a way to know anything about this page. Because we are only observing when we change the tracking protection to ON or OFF or when new trackers are blocked and none of these cases happen in that scenario.

components/feature/toolbar/src/main/java/mozilla/components/feature/toolbar/ToolbarPresenter.kt

Amejia481 · 2019-08-05T03:47:12Z

@pocmo ready for the next pass on this review :)

jonalmeida

I left a few comments but it doesn't need to block this PR.

components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/BrowserToolbar.kt

jonalmeida · 2019-08-06T18:20:57Z

components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/BrowserToolbar.kt

+     * Sets a listener to be invoked when the site tracking protection indicator icon is clicked.
+     */
+    fun setOnTrackingProtectionClickedListener(listener: (() -> Unit)?) {
+        if (listener == null) {


Is setting null used for this API to remove the listener? I'm not sure I follow how this would be used.

toolbar.setOnTrackingProtectionClickedListener { /* do magic */ } // I no longer want to listen for TP clicked changes.. toolbar.setOnTrackingProtectionClickedListener(null)

Seems strange to want to allow being able to explicitly do that, and use null to allow that.

We can can open an issue for checking that!

...s/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/display/DisplayToolbar.kt

components/feature/toolbar/src/main/java/mozilla/components/feature/toolbar/ToolbarPresenter.kt

…on the toolbar.

Amejia481 requested a review from pocmo July 26, 2019 05:30

pocmo self-assigned this Jul 26, 2019

Amejia481 requested a review from ekager July 26, 2019 14:14

This was referenced Jul 26, 2019

Add Tracking Protection Panel to the toolbar mozilla-mobile/fenix#4310

Merged

Introduce new states to the tracking protection icon #3922

Closed

Amejia481 force-pushed the tc_icon branch from bac043d to 6194303 Compare July 28, 2019 12:33

Amejia481 force-pushed the tc_icon branch from 6194303 to 2c5c226 Compare July 28, 2019 12:34

Amejia481 changed the title ~~Tracking Protection icon on the toolbar draft~~ Closes #3824: Adding the site tracking protection icon on the toolbar Jul 28, 2019

Amejia481 marked this pull request as ready for review July 28, 2019 12:35

Amejia481 requested a review from a team as a code owner July 28, 2019 12:35

Amejia481 force-pushed the tc_icon branch from 2c5c226 to 3e907b4 Compare July 28, 2019 13:12

ekager reviewed Jul 29, 2019

View reviewed changes

Amejia481 force-pushed the tc_icon branch 2 times, most recently from fe12b31 to 2ee9d45 Compare July 31, 2019 22:03

pocmo reviewed Aug 1, 2019

View reviewed changes

Amejia481 force-pushed the tc_icon branch from af68ee7 to b935646 Compare August 5, 2019 01:35

Amejia481 changed the title ~~Closes #3824: Adding the site tracking protection icon on the toolbar~~ Closes #3824: Adding the site tracking protection icon on the toolbar. Aug 5, 2019

Amejia481 force-pushed the tc_icon branch from f8c58db to 61188a6 Compare August 5, 2019 03:12

Amejia481 changed the title ~~Closes #3824: Adding the site tracking protection icon on the toolbar.~~ Closes #3824: Adding the site tracking protection icon on the toolbar Aug 5, 2019

Amejia481 requested review from pocmo and ekager August 5, 2019 03:26

jonalmeida approved these changes Aug 6, 2019

View reviewed changes

Amejia481 force-pushed the tc_icon branch from 61188a6 to 7504dc6 Compare August 6, 2019 21:02

Closes mozilla-mobile#3824: Adding the site tracking protection icon …

7f3db98

…on the toolbar.

Amejia481 force-pushed the tc_icon branch from 7a537ed to 7f3db98 Compare August 6, 2019 21:40

Merge branch 'master' into tc_icon

7bd84cc

csadilek merged commit e16da1f into mozilla-mobile:master Aug 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #3824: Adding the site tracking protection icon on the toolbar #3906

Closes #3824: Adding the site tracking protection icon on the toolbar #3906

Amejia481 commented Jul 26, 2019 •

edited by csadilek

Loading

codecov bot commented Jul 28, 2019 •

edited

Loading

ekager Jul 29, 2019

Amejia481 Jul 30, 2019

pocmo Aug 1, 2019

pocmo Aug 1, 2019

pocmo Aug 1, 2019

Amejia481 Aug 1, 2019

pocmo Aug 1, 2019

Amejia481 Aug 1, 2019

Amejia481 Aug 4, 2019

Amejia481 commented Aug 5, 2019

jonalmeida left a comment

jonalmeida Aug 6, 2019

Amejia481 Aug 6, 2019

Closes #3824: Adding the site tracking protection icon on the toolbar #3906

Closes #3824: Adding the site tracking protection icon on the toolbar #3906

Conversation

Amejia481 commented Jul 26, 2019 • edited by csadilek Loading

Pull Request checklist

After merge

codecov bot commented Jul 28, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amejia481 commented Aug 5, 2019

jonalmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amejia481 commented Jul 26, 2019 •

edited by csadilek

Loading

codecov bot commented Jul 28, 2019 •

edited

Loading