Closed Bug 1063115 Opened 7 years ago Closed 7 years ago

grey out text for mixed content and tracking protection if the corresponding type has been disabled

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.2

People

(Reporter: mmc, Assigned: alexbardas)

References

Details

Attachments

(3 files, 4 obsolete files)

Points: --- → 2
Component: DOM: Security → General
Flags: qe-verify+
Flags: firefox-backlog+
OS: Mac OS X → All
Product: Core → Firefox
Hardware: x86 → All
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Iteration: --- → 35.1
It's one approach which uses less js and more css for what needs to be displayed.
Attachment #8487628 - Flags: review?(bmcbride)
Attached image tracking-protection.png
A screenshot with how the text is displayed when tracking options is off.
Comment on attachment 8487628 [details] [diff] [review]
Grey out text for tracking protection if corresponding type has been disabled

Review of attachment 8487628 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +1740,5 @@
>            _doorhangerTitle.value =
>              gNavigatorBundle.getFormattedString(
>                "badContentBlocked.blocked.message", [this._brandShortName]);
>            _mixedContent.hidden = false;
> +          _mixedContent.classList.add("tracking-enabled");

This isn't just tracking. Something like block-enabled/block-disabled would be more appropriate.

::: browser/themes/shared/badcontent-doorhanger.inc.css
@@ +17,2 @@
>  .popup-notification-item-message-critical[popupid="bad-content"] {
> +  color: #d74345 !important;

You should consider !important to be the giant hammer of last resort. Or something.

Better off with a :not() added to the .tracking-disabled selector above.
Attachment #8487628 - Flags: review?(bmcbride) → review-
Applied the suggestions from the first review
Attachment #8487628 - Attachment is obsolete: true
Attachment #8488106 - Flags: review?(bmcbride)
Comment on attachment 8488106 [details] [diff] [review]
Grey out text for tracking protection if corresponding type has been disabled

>--- a/browser/themes/shared/badcontent-doorhanger.inc.css
>+++ b/browser/themes/shared/badcontent-doorhanger.inc.css

>+.block-disabled .popup-notification-item-message[popupid="bad-content"]:not(.popup-notification-item-message-critical) {
>+  color: #c1c1c1;
>+}

This looks like it isn't going to work right with exotic OS themes. I suggest you use GrayText, but please test this at least with some high-contrast themes.

>+.block-enabled > .popup-notification-footer {
>+    display: none;
>+}

This part belongs in a content stylesheet. (Also, nit: indentation is off)
Attachment #8488106 - Flags: review-
(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8488106 [details] [diff] [review]
> Grey out text for tracking protection if corresponding type has been disabled
> 
> >--- a/browser/themes/shared/badcontent-doorhanger.inc.css
> >+++ b/browser/themes/shared/badcontent-doorhanger.inc.css
> 
> >+.block-disabled .popup-notification-item-message[popupid="bad-content"]:not(.popup-notification-item-message-critical) {
> >+  color: #c1c1c1;
> >+}
> 
> This looks like it isn't going to work right with exotic OS themes. I
> suggest you use GrayText, but please test this at least with some
> high-contrast themes.

It would also be nice if you replaced the block-disabled class with an attribute that you set on the bound element and let popup-notification-item-message inherit it. So the selector would look something like this:

.popup-notification-item-message[popupid="bad-content"][blockdisabled]:not(.popup-notification-item-message-critical)
I've added an image how tracking protection is visible on windows on a High Contrast theme. (the color is still #c1c1c1 and not greytext). 

Using GreyText color, the difference between the black and grey(in normal mode) is pretty small.
Dao had some interesting suggestions. I've applied them and added both Blair (initial reviewer) and Dao for review.

I like it more how it looks when "#c1c1c1" color is used (than GreyText). The difference is quite noticeable. It also doesn't look that bad on High Contrast themes (I've attached a screenshot). There are some icons (the `x` which closes the doorhanger) which cannot be seen at all on HC.
Attachment #8488106 - Attachment is obsolete: true
Attachment #8488106 - Flags: review?(bmcbride)
Attachment #8488253 - Flags: review?(dao)
Attachment #8488253 - Flags: review?(bmcbride)
(In reply to Alex Bardas :alexbardas from comment #8)
> I like it more how it looks when "#c1c1c1" color is used (than GreyText).
> The difference is quite noticeable. It also doesn't look that bad on High
> Contrast themes (I've attached a screenshot).

There are also white high-contrast themes where #c1c1c1 won't have enough contrast. Also, these themes are just one extreme. There are likely gray Gtk themes where #c1c1c1 would basically disappear.
Comment on attachment 8488253 [details] [diff] [review]
Grey out text for tracking protection if corresponding type has been disabled

>+.popup-notification-item-message[popupid="bad-content"][mixedblockdisabled]:not(.popup-notification-item-message-critical),
>+.popup-notification-item-message[popupid="bad-content"][trackingblockdisabled]:not(.popup-notification-item-message-critical) {
>+  color: #c1c1c1;
>+}

see comment 9

>+.popup-notification-footer[popupid="bad-content"] {
>+  display: none;
>+}
>+
>+.popup-notification-footer[popupid="bad-content"][mixedblockdisabled],
>+.popup-notification-footer[popupid="bad-content"][trackingblockdisabled] {
>+  display: block;
>+}

This still looks like it belongs in a content stylesheet.

Other than that this patch looks better than the previous one.
Attachment #8488253 - Flags: review?(dao)
Attachment #8488253 - Flags: review-
Attachment #8488253 - Flags: feedback+
Comment on attachment 8488253 [details] [diff] [review]
Grey out text for tracking protection if corresponding type has been disabled

Review of attachment 8488253 [details] [diff] [review]:
-----------------------------------------------------------------

Eh, what Dao said.

Colours are hard.
Attachment #8488253 - Flags: review?(bmcbride)
Attachment #8488253 - Attachment is obsolete: true
Attachment #8488353 - Flags: review?(bmcbride)
QA Contact: mwobensmith
Attachment #8488353 - Flags: review?(bmcbride) → review+
Try run looks good:

https://tbpl.mozilla.org/?tree=Try&rev=f23dfeffa78a

I'd like to add some tests, but I'd rather add them to bug 1043805, as a new patch, to have actually more things to test.
Keywords: checkin-needed
seems this patch failed to apply:

patching file browser/base/content/urlbarBindings.xml
Hunk #3 FAILED at 1653
1 out of 6 hunks FAILED -- saving rejects to file browser/base/content/urlbarBindings.xml.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug1063115_grey_out_text_for_tracking_protection_if_corresponding_type_has_been_disabled.diff

could you take a look thanks!
Iteration: 35.1 → 35.2
(In reply to Carsten Book [:Tomcat] from comment #14)
> seems this patch failed to apply:
> 
> patching file browser/base/content/urlbarBindings.xml
> Hunk #3 FAILED at 1653
> 1 out of 6 hunks FAILED -- saving rejects to file
> browser/base/content/urlbarBindings.xml.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh
> bug1063115_grey_out_text_for_tracking_protection_if_corresponding_type_has_be
> en_disabled.diff
> 
> could you take a look thanks!

Yes, it's probably because of the bug 1058412. It needs to be rebased on top of it. I'll updated this patch shortly.
I've just rebased the patch and re-tested it against the newest tree.
Attachment #8488353 - Attachment is obsolete: true
Attachment #8490478 - Flags: review+
Attachment #8490478 - Flags: checkin?
Comment on attachment 8490478 [details] [diff] [review]
Grey out text for tracking protection if corresponding type has been disabled

Please just use checkin-needed in the future. It plays more nicely with our bug marking tools.
Attachment #8490478 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/269fb7d39edd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Verified as fixed using:

FF 35 Aurora
Build Id:20141022004003
OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.