Closed
Bug 1175972
Opened 9 years ago
Closed 9 years ago
Update visuals of tracking protection doorhanger notification
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(firefox42 verified)
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)
Comment 1•9 years ago
|
||
Just look at the Tracking Protection section of this doorhanger (underneath the Site ID)
Flags: needinfo?(alam)
Comment 2•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → liuche
Flags: needinfo?(liuche)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Bug 1175972 - Update visuals of tracking protection doorhanger notification. r=ally
Attachment #8631928 -
Flags: review?(ally)
Assignee | ||
Comment 10•9 years ago
|
||
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
Assignee | ||
Comment 11•9 years ago
|
||
Bug 1147112 - Add doorhanger icons. r=ally
Attachment #8632387 -
Flags: review?(ally)
Assignee | ||
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
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
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8632387 [details] MozReview Request: Bug 1147112 - Add doorhanger icons. r=ally Bug 1147112 - Add doorhanger icons. r=ally
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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 18•9 years ago
|
||
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+
Assignee | ||
Comment 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
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.
Assignee | ||
Comment 21•9 years ago
|
||
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
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b39ef9d08fd5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment 23•9 years ago
|
||
Verified fixed using: Device: Nexus 4 (Android 5.1) Build: Firefox for Android 42.0a1 (2015-07-22)
Updated•9 years ago
|
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•