1253719 - Link to update outdated plugins should be in skyblue, not red

Status: RESOLVED INACTIVE

(Toolkit :: Add-ons Manager, defect, P3)

Description

[WBT]

The "Check for Updates..." links pictured at https://support.mozilla.org/en-US/kb/why-do-i-have-click-activate-plugins should be in green, not red, if we want to encourage users to read and click on the links, instead of the current color language which tells people NOT to click on the "Check for Updates" link. 

If I've got the wrong component, please correct the tag.

Comment 1

[:shell escalante]

this seems like a valid color issue - what is your take markus?

Comment 2

[Markus Jaritz [:markus] (UX)]

I agree that red might not be the right color to motivate user to click that button. Same thing applies for the Mpzilla Plugin Checker shown on that page.

Green or blue would be good colors. Probably better to use blue as it is not a confirmation, but a highlight.
Philip, what color-value would you recommend?

Comment 3

[Philip [:pwalm]]

Concurred! #0996F8 (or, "blue") should do nicely.

Comment 4

[[:Towkir] Ahmed]

Hi Andrew Swan, I would like to work on this bug, can you / anyone please tell me which file to look for the code to edit ? then I can submit a patch.
Thanks

Comment 5

[Andrew Swan [:aswan]]

Hi, thanks for helping out!  I don't know off the top of my head where this gets set but I'm assuming the link has an appropriately named css class with corresponding rules somewhere in toolkit/mozapps/extensions/content/extensions.css.
A good first step would be to enable and then open the browser toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox) and use the inspector to find what class the link has and the corresponding styles.  For when you get to the point of writing a patch, you'll want to get a local build environment set up (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build).
Please feel free to ask questions here or on IRC, if you ask a question here, please set the needinfo flag for me to make sure I notice!

Comment 6

[[:Towkir] Ahmed]

Hi Andrew,
I think this value[1] is the target, can you please confirm ??
then I will submit a patch, thanks !

[1] http://searchfox.org/mozilla-central/source/toolkit/themes/mobile/mozapps/plugins/pluginProblem.css#41

Comment 7

[Andrew Swan [:aswan]]

It looks like you've found the right classes.  But the css file you linked to is just used for firefox on android.  There's a separate file for the regular desktop browser:
http://searchfox.org/mozilla-central/source/toolkit/themes/shared/plugins/pluginProblem.css#35-42
You'll need to change both places.
Good work so far, thanks!

Comment 8

[[:Towkir] Ahmed]

Taking this on me

Comment 9

[[:Towkir] Ahmed]

Attachment: bluefromred.patch

And here is the patch

Comment 10

[[:Towkir] Ahmed]

The patch contains color: #0996F8 according to philip (comment 3)

Thanks !

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8766889 [details] [diff] [review]
bluefromred.patch

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

You changed a bunch of unrelated classes (vulnerable plugins with no update available for instance) that I don't think are meant to be changed.
Also please include screenshots with your changes applied.

Comment 12

[[:Towkir] Ahmed]

Sorry about that swan, 
but I did that according to my assumption (comment 6) and your confirmation (comment 7)

Can you please help me find the exact css file ?
Thanks

Comment 13

[Andrew Swan [:aswan]]

You've got the correct files, you just changed a rule that applies to 6 classes but only 2 of those classes should be changed.  You'll need to split it up into 2 rules, one for the existing unchanged classes and one for the classes to be changed.

Comment 14

[[:Towkir] Ahmed]

So, they should be these classes http://searchfox.org/mozilla-central/source/toolkit/themes/shared/plugins/pluginProblem.css#35-36 only set to color: blue; on line number http://searchfox.org/mozilla-central/source/toolkit/themes/shared/plugins/pluginProblem.css#43  ,  right ?

Comment 15

[[:Towkir] Ahmed]

Attachment: possiblesolution.patch

marked the previous patch and comment 14 as obsolete and here is a new possible solution, as I could not test it myself. I am sorry in advance if this is another wrong one. A review is really appreciated.

Thanks :)

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 8769791 [details] [diff] [review]
possiblesolution.patch

-> to aswan, who's been reviewing thus far.

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 8769791 [details] [diff] [review]
possiblesolution.patch

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

You now have two declarations for -moz-handler-vulnerable-updatable, the old red one and the new blue one.  Please remove the former.
Also please provide a screenshot with the changes applied.

Comment 18

[Andreas Wagner [:TheOne] [use NI]]

Ahmed, are you still working on this?

Comment 19

[[:Towkir] Ahmed]

Not really.
I tried, but found no way to reproduce the issue or to verify my fix.
Leaving as open again.
Thanks

Comment 20

[Tzu-Chi Lin]

Hi, Swan

I am interested on this bug.
But I guess the problem doesn't lie in where Ahmed found, I think it's with these lines:
http://searchfox.org/mozilla-central/source/toolkit/themes/shared/plugins/pluginProblem.css#208-213
Here's the outcome if I change the background-color:
http://i.imgur.com/k7SivGq.png
Am I correct? If so, do you have any idea what background color value should be placed with :active?

Thanks for help!

Comment 21

[Andrew Swan [:aswan]]

Hi, it sounds like you're on the right track, can you create a patch with your changes and attach it as well as before and after screenshots to this bug?
Then, even though this bug is under add-ons manager, I think it would make most sense for a more front-end oriented person to look at it.  I would suggest :jaws, you can set the review? or feedback? flag on your patch directed to him, and he can reassign if necessary.

Comment 22

[Tzu-Chi Lin]

Attachment: colorchangetest.patch

Hi, Swan, Wein

I've created a patch and here are screenshots before and after this patch, and sorry it takes me some time as I am still not quite familiar with bugzilla.

before: http://i.imgur.com/jeURLzf.png
after: http://i.imgur.com/k7SivGq.png

And as Swan suggested, Wein, could you give me some feedback on what color should be used on :active property(when the link is clicked)?

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8787181 [details] [diff] [review]
colorchangetest.patch

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

Thanks for the patch. The colors below come from https://firefoxux.github.io/StyleGuide/#/colours

Note that we should have a :hover state, if one does not exist already please add this. And we should modify the :active state to use :hover:active. Thanks!

::: toolkit/themes/shared/plugins/pluginProblem.css
@@ +206,4 @@
>  }
>  
>  :-moz-handler-vulnerable-updatable .action-link {
> +  background-color: #0996fb;

This should be #0996f8

Then we should have the :hover color be: #0675d3
And the :hover:active color should be: #0568ba

Comment 24

[Tzu-Chi Lin]

Attachment: colorchangetest.patch

Thanks for help! Here's the new patch(and flag previous patch as obsolete)
How do you guys think about this?

Comment 25

[Tzu-Chi Lin]

And since Wein mentions we should have a :hover state, I actually find current :active state very confusing. The "Check for update..." button is now a child of .hoverBox class, so if I click on the button(which becomes :active), the whole .hoverBox becomes :active, the background color becomes dark, and the "Active [Plugin Name]" link becomes red, which seems pretty confusing to me.

I think when "Check for updates..." button is clicked(:active), the whole .hoverBox(and the link to active the plugin) shouldn't change, since clicking the .hoverBox and the link means to activate the plugin.

Maybe the only way to change this is to change the html structure by making the update button not a child of .hoverBox since we can't prevent :active state from propagating to parent element, I think I can't make any changes here.(or can I?)

Comment 26

[[:Towkir] Ahmed]

As Lin is doing good on this and submitted patch too
I think it can be assigned to him.

To Lin:
You can follow this https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html to get bug permission or maybe :jaws or :aswan can help you about that. for now I am assigning this for you.
Thanks

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Thank you for assigning the bug Ahmed. I should have done that sooner so thank you for helping out.

Yes, the HTML structure should be changed. Can you change the HTML structure as part of fixing this bug?

Comment 28

[Tzu-Chi Lin]

Attachment: colorchangetest.patch

Hi, guys

Thanks Ahmed for assigning the bug to me.

So, basically I've encountered a HUGE lot of problems during changing the html structure, I've tried my best to fix the problem meanwhile reserve the current behaviors.(including 1. The color of "Active Plugin" message should change to red  both when the background is clicked and it's clicked itself. 2. Behaviors of "Check for Updates" button shouldn't be related with the behaviors of background and "Active Plugin" link.)

Currently the only behavior different from the original one is that when you click on the Stop icon at the center or the text below, the link to activate the plugin won't be active, but it's still considered acceptable in my opinion.

Here is my patch, hope it's not too dirty.

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 8790057 [details] [diff] [review]
colorchangetest.patch

I'm not a good reviewer for non-trivial front-end patches, redirecting.

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8790057 [details] [diff] [review]
colorchangetest.patch

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

I will need to look deeper at your patch but you should see how you can fix the following feedback first.

::: toolkit/themes/shared/plugins/pluginProblem.css
@@ +27,5 @@
>    width: 100%;
>    height: 100%;
>  }
> +
> +.hoverBox > * {

We can't use the universal selector as the right-most selector because it has really bad performance implications. See https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS#Avoid_Universal_Rules.21 for more details.

@@ +52,5 @@
> +:-moz-handler-clicktoplay .activeBox:active ~ label .msgClickToPlay,
> +:-moz-handler-vulnerable-updatable .activeBox:active ~ label .msgTapToPlay,
> +:-moz-handler-vulnerable-updatable .activeBox:active ~ label .msgClickToPlay,
> +:-moz-handler-vulnerable-no-update .activeBox:active ~ label .msgTapToPlay,
> +:-moz-handler-vulnerable-no-update .activeBox:active ~ label .msgClickToPlay,

We should be using the adjacent sibling selector here to improve CSS performance.

Comment 31

[Andrew Williamson [:eviljeff]]

Are you still able to work on this bug?

Comment 32

[Tzu-Chi Lin]

Hi, I am a little busy recently, but I am still working on it.

Comment 33

[Andrew Williamson [:eviljeff]]

Tzu-Chi Lin we're going to de-assign you for now, please re-assign yourself if/when you are able to start again.

Comment 34

[Aman Mehta]

I would like to work on this issue, can anybody brief me with the status of this bug now?

Comment 35

[Andrew Swan [:aswan]]

Aman, welcome and thanks for your interest!
You can read through some of the recent comments above to see where this was left off when it was last worked on.  Good first steps would be reproducing the scenario covered by this bug locally (so that you can test your changes when they're ready) and doing a local Firefox build so you can start making modifications.
Ping Jared to see if he has any more comments on the actual substance of the changes needed.

Comment 36

[Aman Mehta]

Hey :aswan, :jaws I have made some changes to my mozilla-central  codebase locally. Since I am new to bugzilla, can you guide me on how I can run my code locally and see the corresponding changes. For this bug, how do I see the color of "check for updates" after doing the changes.

Comment 37

[Andrew Swan [:aswan]]

Instructions for building and running Firefox from a local source tree are on MDN:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Also, if you have a question for somebody, please check the "Need more information from" box below and enter the person who you're asking, to make sure they notice that somebody is waiting for an answer...

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for sharing the link Andrew. Aman, you can use artifact builds for this bug since it won't require changes to C++. See https://developer.mozilla.org/en-US/docs/Artifact_builds for more information, but you should first follow the steps in the link that Andrew posted.

Comment 39

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.