Closed Bug 1175972 Opened 5 years ago Closed 4 years ago

Update visuals of tracking protection doorhanger notification

Categories

(Firefox for Android :: Theme and Visual Design, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- verified

People

(Reporter: Margaret, Assigned: liuche)

References

Details

Attachments

(6 files)

antlam can explain what we need to do here :)
Flags: needinfo?(alam)
Attached image prev_mob_dh2.png
Just look at the Tracking Protection section of this doorhanger (underneath the Site ID)
Flags: needinfo?(alam)
Chenxia,

like we talked about, we'll need to pull out the "Enabled" from the title and put it in the next line in "enabled green". "Disabled" can just be the "text grey" from our palette for now.

Also, currently in Nightly, we have two buttons at all times, this mock has 1 depending on what action is available to the user. Can we try this out? I feel like it's better in terms of a clear actionable item the user can do, but I can see why we might want two buttons at times.
Flags: needinfo?(liuche)
Assignee: nobody → liuche
Flags: needinfo?(liuche)
Status: NEW → ASSIGNED
Need some copy for the re-enable tracking protection state, but as a filler, will use the desktop strings:

"Tracking elements detected. You have disabled protection on this website."

This is a bit long, so we need to come up with a better string. Matej, would you come up with a string for us here? It's the "Control Center Doorhanger" (last item), but for the case where the user has disabled tracking protection.

https://docs.google.com/document/d/134Ez8FpoMVkJ72a4ux8t6mPVNTU8iNPwAa5On4cUQwU/edit
Flags: needinfo?(matej)
Following the example in the mockup, I'm thinking something like this:

Tracking protection
Disabled
Blocking tracking elements may affect your browsing experience.

I've also left a comment in the gdoc for the existing string.
Flags: needinfo?(matej)
Thanks for the strings, Matej, I got confused and looked for them in the google doc.

My impression of this string is that it is somewhat discouraging of tracking protection, because it focuses on the negative aspects of blocking trackers. We'd like to encourage use of tracking protection (I think) and relatedly, Tanvi or Aislinn mentioned that for Mobile people are more likely to blame degraded experience on the website rather than the browser, so if there is another message we'd like to send, we can do that.

Thanks!
Flags: needinfo?(matej)
In the gdoc, Aislinn suggested changing the mobile string for the enabled state to match what's on desktop. That would make it:

Tracking protection
Enabled
Attempts to track your online behavior have been blocked.


So for the disabled state it could be:

Tracking protection
Disabled
Attempts to track your online behavior are not being blocked.


Or:

Tracking protection
Disabled
Enable to block attempts to track your online behavior.
Flags: needinfo?(matej)
Bug 1175972 - Update visuals of tracking protection doorhanger notification. r=ally
Attachment #8631928 - Flags: review?(ally)
Comment on attachment 8631928 [details]
MozReview Request: Bug 1175972 - Update visuals of tracking protection doorhanger notification. r=ally

Bug 1175972 - Update visuals of tracking protection doorhanger notification. r=ally
Bug 1147112 - Add doorhanger icons. r=ally
Attachment #8632387 - Flags: review?(ally)
The review request in comment 11 is actually for bug 1175972, of course, but reviewboard isn't yet smart enough to handle a stack of commits that has multiple bugs.
Comment on attachment 8631928 [details]
MozReview Request: Bug 1175972 - Update visuals of tracking protection doorhanger notification. r=ally

Bug 1175972 - Update visuals of tracking protection doorhanger notification. r=ally
Comment on attachment 8632387 [details]
MozReview Request: Bug 1147112 - Add doorhanger icons. r=ally

Bug 1147112 - Add doorhanger icons. r=ally
Chenxia, does this include the icon in the URL bar?

I know we said this doesn't include the one in the doorhanger, but I wanted to make sure we updated the URL bar icon too.
Flags: needinfo?(liuche)
Yep, the icon is updated in the urlbar too - I realize I was really hilariously terrible at taking screenshots, because actually none of them shows all the changes lol. But trust me, the same icon is used everywhere :P
Flags: needinfo?(liuche)
Comment on attachment 8631928 [details]
MozReview Request: Bug 1175972 - Update visuals of tracking protection doorhanger notification. r=ally

https://reviewboard.mozilla.org/r/12975/#review11853

I have some questions, but it's almost there..

::: mobile/android/base/widget/ContentSecurityDoorHanger.java:82
(Diff revision 3)
> -                    final int padding = mResources.getDimensionPixelSize(R.dimen.doorhanger_section_padding_small);
> +                    mSecurityState.setText(R.string.doorhanger_tracking_state_enabled);

nit indentation

::: mobile/android/base/widget/ContentSecurityDoorHanger.java:83
(Diff revision 3)
> -                    View v = input.getView(getContext());
> +                    mSecurityState.setTextColor(getResources().getColor(R.color.affirmative_green));

nit indentation

::: mobile/android/base/widget/ContentSecurityDoorHanger.java:86
(Diff revision 3)
> -                    group.addView(v);
> +                    mSecurityState.setText(R.string.doorhanger_tracking_state_disabled);

nit indentation

::: mobile/android/base/widget/ContentSecurityDoorHanger.java:87
(Diff revision 3)
> -                } catch(JSONException ex) { }
> +                    mSecurityState.setTextColor(getResources().getColor(R.color.rejection_red));

nit indendation

::: mobile/android/base/widget/ContentSecurityDoorHanger.java
(Diff revision 3)
> -                        default:

please readd the default label. Since it seems like that should never be hit, it would be worth logging an error that supplied type does not match any existing ones.

::: mobile/android/base/widget/DoorHanger.java:158
(Diff revision 3)
> +    protected void addLink(String label, final String url) {

So this function looks like it is unchanged from being moved to here from DefaultDoorhanger, correct ?

::: mobile/android/base/widget/DefaultDoorHanger.java
(Diff revision 3)
> -            } catch (JSONException e) { }

Is it expected that this will throw regularly?

::: mobile/android/base/widget/ContentSecurityDoorHanger.java
(Diff revision 3)
> -        if (sSpinnerTextColor == -1) {

Why do we no longer need this? It doesn't seem to have anything to do with the rest of this patch?

::: mobile/android/base/moz.build:513
(Diff revision 3)
> +    'widget/ContentSecurityDoorHanger.java',

If this is not a new file but a rename, where is the removal of the old file name from the moz.build file?
Attachment #8631928 - Flags: review?(ally)
Comment on attachment 8632387 [details]
MozReview Request: Bug 1147112 - Add doorhanger icons. r=ally

https://reviewboard.mozilla.org/r/13057/#review11861

::: mobile/android/base/resources/layout/doorhanger.xml:21
(Diff revision 2)
> +                   android:layout_marginRight="@dimen/doorhanger_section_padding_small"

nit missing indent

::: mobile/android/base/resources/layout/site_identity.xml:24
(Diff revision 2)
> +                   android:layout_marginRight="@dimen/doorhanger_section_padding_small"/>

nit missing indent

::: mobile/android/base/widget/DefaultDoorHanger.java:47
(Diff revision 2)
> +        switch (mType) {

Do we really need a switch here for a single case? Unless you have follow up plans?
Attachment #8632387 - Flags: review?(ally) → review+
https://reviewboard.mozilla.org/r/12975/#review11865

::: mobile/android/base/moz.build:513
(Diff revision 3)
> +    'widget/ContentSecurityDoorHanger.java',

There was a file that was copied, and then modified - there are no files that needed to be removed. This is simply a new type of Doorhanger.

::: mobile/android/base/widget/ContentSecurityDoorHanger.java:82
(Diff revision 3)
> -                    final int padding = mResources.getDimensionPixelSize(R.dimen.doorhanger_section_padding_small);
> +                    mSecurityState.setText(R.string.doorhanger_tracking_state_enabled);

I don't actually see any problems in indentation here, so dropping all these issues.

::: mobile/android/base/widget/DefaultDoorHanger.java
(Diff revision 3)
> -            } catch (JSONException e) { }

This is just code removal, but this isn't expected to throw, but also not catastrophic at all if it does. Then we'd merely be missing either the title or the favicon in the doorhanger.

::: mobile/android/base/widget/DoorHanger.java:158
(Diff revision 3)
> +    protected void addLink(String label, final String url) {

Yes.
https://reviewboard.mozilla.org/r/13057/#review11869

::: mobile/android/base/resources/layout/doorhanger.xml:21
(Diff revision 2)
> +                   android:layout_marginRight="@dimen/doorhanger_section_padding_small"

I'm not seeing any of these indent problems.

::: mobile/android/base/widget/DefaultDoorHanger.java:47
(Diff revision 2)
> +        switch (mType) {

Yes, when we add more icons for doorhanger types, we will need to add more cases.
url:        https://hg.mozilla.org/integration/fx-team/rev/b39ef9d08fd55c990012447ff7f99e65a7df9795
changeset:  b39ef9d08fd55c990012447ff7f99e65a7df9795
user:       Chenxia Liu <liuche@mozilla.com>
date:       Wed Jul 08 15:42:11 2015 -0700
description:
Bug 1175972 - Update visuals of tracking protection doorhanger notification. r=ally
https://hg.mozilla.org/mozilla-central/rev/b39ef9d08fd5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Verified fixed using:
Device: Nexus 4 (Android 5.1)
Build: Firefox for Android 42.0a1 (2015-07-22)
You need to log in before you can comment on or make changes to this bug.