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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

See greyed out text in https://bugzilla.mozilla.org/attachment.cgi?id=8459851

Comment 1

[Alex Bardas :alexbardas]

Attachment: Grey out text for tracking protection if corresponding type has been disabled

It's one approach which uses less js and more css for what needs to be displayed.

Comment 2

[Alex Bardas :alexbardas]

Attachment: tracking-protection.png

A screenshot with how the text is displayed when tracking options is off.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 4

[Alex Bardas :alexbardas]

Attachment: Grey out text for tracking protection if corresponding type has been disabled

Applied the suggestions from the first review

Comment 5

[Dão Gottwald [:dao]]

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)

Comment 6

[Dão Gottwald [:dao]]

(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)

Comment 7

[Alex Bardas :alexbardas]

Attachment: Windows high contrast tracking protection

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.

Comment 8

[Alex Bardas :alexbardas]

Attachment: Grey out text for tracking protection if corresponding type has been disabled

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.

Comment 9

[Dão Gottwald [:dao]]

(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 10

[Dão Gottwald [:dao]]

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.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 12

[Alex Bardas :alexbardas]

Attachment: Grey out text for tracking protection if corresponding type has been disabled

Comment 13

[Alex Bardas :alexbardas]

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.

Comment 14

[Carsten Book [:Tomcat]]

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!

Comment 15

[Alex Bardas :alexbardas]

(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.

Comment 16

[Alex Bardas :alexbardas]

Attachment: Grey out text for tracking protection if corresponding type has been disabled

I've just rebased the patch and re-tested it against the newest tree.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/269fb7d39edd

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/269fb7d39edd

Comment 20

[Catalin Varga [QA][:VarCat]]

Verified as fixed using:

FF 35 Aurora
Build Id:20141022004003
OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64