first run page redesign

first run page redesign

[Thomas Greiner, 2013-07-15 17:37:01]

Quite a bunch of changes necessary. Also don't add typo.png again - it's no
longer used.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
File chrome/content/ui/firstRun.html (right):

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:29: <p id="dataCorruptionWarning"
class="i18n_firstRun_dataCorruptionWarning" hidden="true"></p>
Why did you move this?

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:50: <img class="feature-image"
src="/skin/features/malware.png">
Use background-image instead of image tags.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:62: <img class="feature-image"
src="/skin/features/social.png">
Use background-image instead of image tags.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:74: <img class="feature-image"
src="/skin/features/tracking.png"/>
Use background-image instead of image tags.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:90: <img src="/skin/features/social.png">
Use background-image instead of image tags.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:94: <img src="/skin/features/tracking.png">
Use background-image instead of image tags.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:98: <img src="/skin/features/malware.png">
Use background-image instead of image tags.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:104: <div id="activate-features"
class="i18n_firstRun_feature_activate" status="overview">
This is not a standard attribute of DIV. Either use class for this or replace it
with data-status. Also don't forget to update the references to it in
firstRun.js.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:113: <a id="donate" href=""
class="i18n_firstRun_donate" target="_blank"></a>
Remove href="". If there is no URL associated with it, it should not have an
href attribute.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:117: <a id="share-facebook"
href="https://www.facebook.com/adblockplus" target="_blank" >
No need for changing this. Instead of putting the image into the anchor tag,
make it a background image of the anchor tag (as it was before).

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.html:130: <footer>
Why does this still exist? If you don't want to use a footer then remove it.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
File chrome/content/ui/firstRun.js (left):

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:215: // Don't open the share page if the sharing
script would be blocked
Don't remove the comment here. It's not obvious what we're doing here.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:23: //var shade;
Remove unnecessary comments.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:24: var scrollTimer;
Not needed anymore.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:51: var toggleButton =
document.getElementById('activate-features');
Use double quotes instead of single quotes for consistency. (same below)

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:55:
if(toggleButton.getAttribute("status")=="overview")
Code Style:
if (toggleButton.getAttribute("status") == "overview")

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:58: setTimeout(function(){fade('in', expandedView,
750, true)},750);
Code Style:
setTimeout(function()
{
  fade("in", expandedView, 750, true);
}, 750);

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:59: toggleButton.innerHTML ="show overview again";
Code Style:
toggleButton.innerHTML = "show overview again";

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:62: } else
if(toggleButton.getAttribute("status")=="expanded")
Code Style:
}
else if (toggleButton.getAttribute("status") == "expanded")

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:65: setTimeout(function(){fade('in', overView,
750, true)},750);
Code Style:
setTimeout(function()
{
  fade("in", overView, 750, true);
}, 750);

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:66: toggleButton.innerHTML ='activate features';
Code Style:
toggleButton.innerHTML = "activate features";

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:67:
toggleButton.setAttribute("status","overview");
Code Style: "status", "overview"

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:70: // jQuery-style fading
I doubt that we need JavaScript animations so CSS transitions should be
sufficient - except if you want to make the rest of your code compatible with
IE6. ;)

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:71: function fade(type, el, duration, IEsupport) {
Code Style: bracket in new line

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:71: function fade(type, el, duration, IEsupport) {
Why is duration a parameter if you only use 750?

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:71: function fade(type, el, duration, IEsupport) {
Please check the code style of the content of this function.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:76: gap      = interval / duration;
Do you really need that many variables?

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:87: function func() {
Not a good name for this function.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:94: if(opacity <= 0 || opacity >= 1) {
window.clearInterval(fading); }
Code Style (also below):
if (opacity <= 0 || opacity >= 1)
  window.clearInterval(fading);

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:104: donateLink.href = Utils.getDocLink("donate");
Weird indentation level. Also please leave some space between this block and the
block below.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:117: //shade.addEventListener("mouseout",
stopScroll, false);
Remove unnecessary comments.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:138: window.addEventListener("resize",
onWindowResize, false);
Not needed anymore.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:139: document.addEventListener("scroll", onScroll,
false);
Not needed anymore.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:141: var activateFeaturesButton =
document.getElementById('activate-features');
Remove "Button" from the variable name. It doesn't necessarily have to be a
button.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:144: onWindowResize();
Not needed anymore.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:152: //shade.style.opacity =
(document.documentElement.scrollHeight == currentHeight) ? "0.0" : "0.5";
It looks like onScroll isn't used by anything anymore. If that's the case please
remove it entirely.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:155: function onWindowResize()
Not needed anymore.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/content/ui/firstR...
chrome/content/ui/firstRun.js:160: function toggleTypoCorrectionEnabled()
Not needed anymore.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/locale/en-US/firs...
File chrome/locale/en-US/firstRun.properties (right):

http://codereview.adblockplus.org/11039060/diff/8001/chrome/locale/en-US/firs...
chrome/locale/en-US/firstRun.properties:1: firstRun_title=Adblock Plus has been
<strong>installed</strong>
This is also being used in the <title> tag which doesn't interpret HTML code.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/locale/en-US/firs...
chrome/locale/en-US/firstRun.properties:11:
firstRun_feature_typo_description=Automatically correct typos in the address bar
to protect you from visiting malicious websites when misspelling an internet
address.
Those two are no longer needed. Don't add them again.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/locale/en-US/firs...
chrome/locale/en-US/firstRun.properties:20: firstRun_share=Please
<strong>donate</strong> or <strong>tell your friends</strong>.
Doesn't appear to be used anywhere.

http://codereview.adblockplus.org/11039060/diff/8001/chrome/locale/en-US/firs...
chrome/locale/en-US/firstRun.properties:23: firstRun_or_tell=or <strong>tell
your friends</strong>
Make this one string instead of three because they are dependent on each other.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:17: .clearfix
Shouldn't be necessary anymore if you can get rid of all the floats.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:22: *
Why do you need to apply that to every single element? Should be sufficient to
apply it to body.

You can probably also get rid of some of the !important below by removing this.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:45: button, .arrow
.arrow doesn't exist anymore.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:220: float: left;
Please avoid floats if possible.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:325: height: 36px;
You should be able to avoid using fixed numbers for the height if you use
padding.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:344: #share > #donate-block
#donate-block should be unique even without |#share >|

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:350: #share > #share-block
#share-block should be unique even without |#share >|

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:352: margin: 0 0 0 0;
|margin: 0;| does the same.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:385: float: left;
Please try to avoid floats.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:399: /*social */
Code Style:
/* social */

May even be unnecessary.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:510: /*box-shadow: 0 0 0 1px #999;*/
Remove this comment.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:516: float: left;
Please avoid using floats if possible.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:585: float: left;
Please avoid floats if possible.

http://codereview.adblockplus.org/11039060/diff/18001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:603: /* Adjust font size on smaller screens */
Why do you keep this portion? You're not using it anyway.

[Thomas Greiner, 2013-07-26 14:22:55]

Mostly styling issues.

http://codereview.adblockplus.org/11039060/diff/28001/chrome/content/ui/first...
File chrome/content/ui/firstRun.html (right):

http://codereview.adblockplus.org/11039060/diff/28001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:36: <p id="dataCorruptionWarning"
class="i18n_firstRun_dataCorruptionWarning" hidden="true"></p>
Change to <section>.

http://codereview.adblockplus.org/11039060/diff/28001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:41: <p
class="i18n_firstRun_acceptableAdsExplanation"></p>
Links inside the text are not set.

http://codereview.adblockplus.org/11039060/diff/28001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:49: <div id="feature-malware-image"></div>
This ID is not unique. If you want a style to apply to more than one element
then use its class attribute.

http://codereview.adblockplus.org/11039060/diff/28001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:53: <div id="feature-social-image"></div>
This ID is not unique. If you want a style to apply to more than one element
then use its class attribute.

http://codereview.adblockplus.org/11039060/diff/28001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:57: <div id="feature-tracking-image"></div>
This ID is not unique. If you want a style to apply to more than one element
then use its class attribute.

http://codereview.adblockplus.org/11039060/diff/28001/chrome/content/ui/first...
chrome/content/ui/firstRun.html:112: <div id="activate-features"
class="i18n_firstRun_feature_activate" data-status="overview">
If you put the status as a class name in <body> instead you could use CSS
transitions instead of the jQuery fade function.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/11039060/diff/43001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:20: 
:)

http://codereview.adblockplus.org/11039060/diff/43001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:52: if(toggleButton.getAttribute("data-status") ==
"overview")
Code Style:
if (...)

http://codereview.adblockplus.org/11039060/diff/43001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:54: fade("out", overView, 750, true);
750ms is a bit long. I'd prefer max 500ms.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:76: // jQuery-style fading
As discussed, CSS transitions should be used whenever possible.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:18: .clearfix
You can get rid of this (including the HTML elements) if you can remove the two
floats.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:31: font-family: Segoe UI, Helvetica, Arial,
sans-serif;
Either bundle this font with the extension or don't reference it at all. The
current approach creates inconsistent experiences.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:54: 
Remove empty line.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:71: display: block;
Not necessary.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:77: max-width: 957px; /*border parallel fix*/
Why 957px? Seems quite random… why not 950px?

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:79: border-right: none;
You can do this instead:

border: 1px #57ab5b;
border-style: dashed none;

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:137: max-width: 960px !important;
Why did you add !important? It works without it because ID selectors overrule
tag name selectors.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:138: padding: 40px 0px 0px 0px  !important;
Why did you add !important? It works without it because ID selectors overrule
tag name selectors.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:146: #can-do-more > h2
You're overusing > a bit in the area starting here. Is it really necessary?

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:163: float: left;
Do you really need a float here? Why not make it an inline-block?

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:173: #can-do-more > ul > li > div
Why not give those DIVs a class name or use existing class names instead of
using this very strict syntax?

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:191: #feature-malware-image
You don't need an ID to uniquely identify it:
#feature-malware .feature-image

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:197: #feature-social-image
You don't need an ID to uniquely identify it:
#feature-social .feature-image

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:203: #feature-tracking-image
You don't need an ID to uniquely identify it:
#feature-tracking .feature-image

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:243: max-width: 624px;
Where does this number come from?

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:248: max-width: 478px;
Where does this number come from?

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:250: float: left;
Do you really need a float here? Why not make it an inline-block?

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:369: padding: 50px 100px 40px 100px !important;
Why did you add !important? It works without it.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:399: background-repeat: no-repeat;
You don't need background-repeat because width and height are already determined
by
#share-buttons > a

Alternatively, you can move it there.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:405: background-repeat: no-repeat;
You don't need background-repeat because width and height are already determined
by
#share-buttons > a

Alternatively, you can move it there.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:411: background-repeat: no-repeat;
You don't need background-repeat because width and height are already determined
by
#share-buttons > a

Alternatively, you can move it there.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:436: margin: 20px;
If you get rid of the margin and change it from an <p> to a <section> element it
doesn't look like it doesn't belong into the page anymore.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:541: 
:)

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:592: 
:)

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:627: color: #003366 !important;
Why did you add !important? It works without it because ID selectors overrule
tag name selectors.

http://codereview.adblockplus.org/11039060/diff/43001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:635: font-family: arial, sans-serif;
Not necessary because you already defined it for the whole body.

[Thomas Greiner, 2013-08-07 14:17:53]

1. Use !important only in cases when there's absolutely no other way.
2. I hope you already got rid of typo.png ;)
3. Check back with Ben about finalizing the texts in firstRun.properties

http://codereview.adblockplus.org/11039060/diff/81001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/11039060/diff/81001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:49: setLinks("share-headline",
Utils.getDocLink("contribute"));
Please move the setLinks call from lines 58-59 here so that they are all in the
same location.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:80: // you can click activate-feature or one of
the icons to toggle the features area
Start comment with a capital letter.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:94: var arrowRight = E("arrow-right");
Instead of changing classes of all those elements you should rather just change
the class of one common container element.

This should also allow you to get rid of some unnecessary ID attributes.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:96: if
(activateFeatures.getAttribute("data-status") == "overview")
You should be able to get rid of the data-status attribute if you use a
container element's class name to determine that.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:104: activateFeaturesLabel.innerHTML = "show
overview again";
Don't use innerHTML. Instead you can use innerText or textContent (depending on
which one can be used).

http://codereview.adblockplus.org/11039060/diff/81001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:113: /* Next timeout has to be done because of an
js bug.
If it's a bug you should also add the URL to the bug report.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:132: activateFeaturesLabel.innerHTML = "activate
features";
Don't use innerHTML.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:22: src: local ('Ø'),
This is not self-explanatory. Please explain why this needs to be here (a
reference would be nice as well).

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:30: src: local ('Ø'),
This is not self-explanatory. Please explain why this needs to be here (a
reference would be nice as well).

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:54: cursor: pointer;
Should not be necessary. Anchor tags have cursor:pointer; by default.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:89: * next to 960px(page witdth) which makes
You probably mean "closest to".

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:89: * next to 960px(page witdth) which makes
typo: "witdth"

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:169: .opacity-zero
Don't call classes like that. The class name should be descriptive of the state
and not of its properties.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:171: opacity: 0 !important;
Use more targeted selectors instead of !important.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:174: .opacity-one
Don't call classes like that. The class name should be descriptive of the state
and not of its properties.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:176: opacity: 1 !important;
Use more targeted selectors instead of !important.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:179: .display-none
Don't call classes like that. The class name should be descriptive of the state
and not of its properties.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:181: display: none !important;
Use more targeted selectors instead of !important.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:184: .display-block
Don't call classes like that. The class name should be descriptive of the state
and not of its properties.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:186: display: block !important;
Use more targeted selectors instead of !important.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:194: font-size: 0px;
0px? What's that supposed to do?

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:338: transition: opacity 0.5s ease-in-out;
Doesn't seem to do anything.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:390: }
It seems odd that those two margins differ. Those shouldn't need to be separate.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:415: width: auto;
Doesn't seem necessary.

http://codereview.adblockplus.org/11039060/diff/81001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:448: margin: 0 !important;
You don't need that if you just replace line 427 with
margin: 0 7px;

[Thomas Greiner, 2013-08-22 10:42:12]

Almost there.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:93: if (activateFeatures.getAttribute("class") ==
"overview")
You can use classList.contains instead.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:98: activateFeatures.classList.add("expanded");
Why don't you simply add the class "expanded" to #can-do-more when it's expanded
and remove it when it's not?

By doing that you should be able to get rid of many of those classList.add and
classList.remove calls on #can-do-more-expanded, #can-do-more-overview and
#activate-features.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:102: if ((activateFeaturesLabel.textContent) &&
(typeof (activateFeaturesLabel.textContent) != "undefined"))
No need for checking it twice. Checking
if ("textContent" in activateFeaturesLabel)
should be sufficient.

Doesn't require to be annotated.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:104: activateFeaturesLabel.textContent = "show
overview again";
Should be translatable.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:107: activateFeaturesLabel.innerText = "show
overview again";
Should be translatable.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:137: if ((activateFeaturesLabel.textContent) &&
(typeof (activateFeaturesLabel.textContent) != "undefined"))
No need for checking it twice. Checking
if ("textContent" in activateFeaturesLabel)
should be sufficient.

Doesn't require to be annotated.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:139: activateFeaturesLabel.textContent = "activate
features";
Should be translatable.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/content/ui/first...
chrome/content/ui/firstRun.js:142: activateFeaturesLabel.innerText = "activate
features";
Should be translatable.

http://codereview.adblockplus.org/11039060/diff/91001/chrome/locale/en-US/fir...
File chrome/locale/en-US/firstRun.properties (right):

http://codereview.adblockplus.org/11039060/diff/91001/chrome/locale/en-US/fir...
chrome/locale/en-US/firstRun.properties:20: firstRun_donate_label=Support with
money
Is that the final text?

http://codereview.adblockplus.org/11039060/diff/91001/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/91001/chrome/skin/firstRun.cs...
chrome/skin/firstRun.css:442: line-height: 0px;
Please add an explanation.

[Thomas Greiner, 2013-08-26 11:33:50]

http://codereview.adblockplus.org/11039060/diff/101002/chrome/content/ui/firs...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/11039060/diff/101002/chrome/content/ui/firs...
chrome/content/ui/firstRun.js:99: activateFeatures.classList.add("expanded");
Review comment not addressed.

http://codereview.adblockplus.org/11039060/diff/101002/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/101002/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:299: /*max-width: 625px;*/
I assume you forgot to remove that comment ;)

http://codereview.adblockplus.org/11039060/diff/101002/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:355: 
I assume you forgot to use those.

http://codereview.adblockplus.org/11039060/diff/101002/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:461: 
:)

[Thomas Greiner, 2013-08-26 16:25:04]

LGTM with those remaining comments addressed

http://codereview.adblockplus.org/11039060/diff/110003/chrome/content/ui/firs...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/11039060/diff/110003/chrome/content/ui/firs...
chrome/content/ui/firstRun.js:165: function scrollPage()
Doesn't seem to be used anywhere.

http://codereview.adblockplus.org/11039060/diff/110003/chrome/content/ui/firs...
chrome/content/ui/firstRun.js:176: function stopScroll()
Doesn't seem to be used anywhere.

http://codereview.adblockplus.org/11039060/diff/110003/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/110003/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:173: .fade-out
Rename this to "fade" because "fade-out" is not accurate anymore.

http://codereview.adblockplus.org/11039060/diff/110003/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:178: .fade-in
Doesn't seem to be used anywhere.

http://codereview.adblockplus.org/11039060/diff/110003/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:188: .show
Doesn't seem to be used anywhere.

http://codereview.adblockplus.org/11039060/diff/110003/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:371: .arrow
Merge this one with the existing .arrow rule below.

[Thomas Greiner, 2013-09-27 10:17:20]

http://codereview.adblockplus.org/11039060/diff/130001/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/130001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:194: -webkit-transition: 0.5s linear;
Property name is missing and behavior is inconsistent.

http://codereview.adblockplus.org/11039060/diff/130001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:259: -webkit-transition: 0.5s linear;
Property name is missing and behavior is inconsistent.

http://codereview.adblockplus.org/11039060/diff/130001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:664: -webkit-border-radius: 9999px;
Please don't only do it for WebKit browsers.

[Wladimir Palant, 2013-10-02 13:21:39]

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
File chrome/content/ui/firstRun.html (right):

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
chrome/content/ui/firstRun.html:147: <a id="contributors">Contributor
Credits</a>
This string should be localized.

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
chrome/content/ui/firstRun.js:46: // Set up URL
URL => URLs

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
chrome/content/ui/firstRun.js:89: function showFeature()
I think the proper name for this function would be toggleFeature().

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
chrome/content/ui/firstRun.js:116: },520);
JavaScript doesn't guarantee any particular order for timeouts - e.g. if the
browser doesn't get to run any code for a second, it can run all the callbacks
in random order. What you want should be done by adding a zero ms timeout inside
the previous one.

Note that display property not being animatable is behavior according to spec,
not a "bug" (and definitely not a JS one).

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
chrome/content/ui/firstRun.js:133: }
We are using CSS transitions - why does this have to be so complicated? IMHO
adding/removing "expanded" class on the container should be sufficient to
trigger all the necessary transitions. The problem is apparently hiding the
element after fading out - display:none won't work for that as mentioned above.
Changing max-height seems to be working however, along these lines:

#can-do-more-expanded
{
  opacity: 0;
  max-height: 0;
  -moz-transition: opacity 0.5s ease-out, max-height 0.5s step-end;
}
#can-do-more.expanded > #can-do-more-expanded
{
  opacity: 1;
  max-height: 10000px;
  -moz-transition: opacity 0.5s step-end, max-height 0.5s step-end;
}

http://codereview.adblockplus.org/11039060/diff/117001/chrome/locale/en-US/fi...
File chrome/locale/en-US/firstRun.properties (right):

http://codereview.adblockplus.org/11039060/diff/117001/chrome/locale/en-US/fi...
chrome/locale/en-US/firstRun.properties:19: firstRun_share_headline=<a>Give us a
hand</a> in making the web a better place
Period at the end of this sentence?

http://codereview.adblockplus.org/11039060/diff/145001/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/145001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:21: src: url(fonts/CreteRound-Regular.otf);
Do we really need a custom font for the first-run page? I'm not even asking
about whether adding 50 kB to the download size is really worth it, the
attribution requirement of its license is something that we have to comply with
- and we currently don't have a place for third-party licenses in either Firefox
or Chrome.

http://codereview.adblockplus.org/11039060/diff/145001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:40: }
I generally consider wildcard CSS rules like this one bad style, you never know
what they will apply to and what kind of unexpected effects they will cause.
What's wrong with setting a margin and padding on the elements where you
actually need it? It doesn't look like there are too many.

http://codereview.adblockplus.org/11039060/diff/145001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:75: height: 200px;
Why the hardcoded height? We don't have to support IE here yet and modern
browsers are generally capable of working without any hardcoded sizes.

Or is that only an alternative way of writing |margin-bottom: 100px|?

http://codereview.adblockplus.org/11039060/diff/145001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:89: margin: 21px auto;
This overrides the margin property three lines above.

http://codereview.adblockplus.org/11039060/diff/145001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:102: font-weight: 600;
Aren't you duplicating browser defaults here? At least in Firefox that tag
defaults to |font-weight: bold| which is equivalent to |font-weight: 700| - I
doubt that there is any visible difference.

http://codereview.adblockplus.org/11039060/diff/145001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:206: height: 115px;
I guess we are hardcoding the size of the text here and ignoring the fact that
translations might have a different size? Can this be avoided?

http://codereview.adblockplus.org/11039060/diff/145001/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:218: #can-do-more-overview > li > div
Seeing this makes me think that the HTML source is missing a few classes. E.g.,
<div class="feature-malware-image"> should probably be <div class="feature-image
feature-malware-image">

[Wladimir Palant, 2013-10-02 13:25:43]

Looks like I was partly reviewing an older patchset. Other than the one below my
comments are still valid however.

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
File chrome/content/ui/firstRun.html (right):

http://codereview.adblockplus.org/11039060/diff/117001/chrome/content/ui/firs...
chrome/content/ui/firstRun.html:147: <a id="contributors">Contributor
Credits</a>
On 2013/10/02 13:21:39, Wladimir Palant wrote:
> This string should be localized.

Never mind, this is apparently already addressed.

[Wladimir Palant, 2013-10-05 09:15:59]

LGTM with the minimal changes outlined below.

http://codereview.adblockplus.org/11039060/diff/164002/chrome/content/ui/firs...
File chrome/content/ui/firstRun.js (right):

http://codereview.adblockplus.org/11039060/diff/164002/chrome/content/ui/firs...
chrome/content/ui/firstRun.js:102: }
Nit: One line of code is enough for the entire function now:

E("can-do-more").classList.toggle("expanded");

http://codereview.adblockplus.org/11039060/diff/164002/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/164002/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:194: transition: opacity 0.3s step-end;
Shouldn't it be ease-in-out rather than step-end here?

http://codereview.adblockplus.org/11039060/diff/164002/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:209: -o-transition: none;
We don't set -o-transition above, we shouldn't need it here either (Presto
engine is dead and current releases won't be running on it).

http://codereview.adblockplus.org/11039060/diff/164002/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:284: -webkit-transition-delay: 0.2s;
-moz-transition-delay seems to be missing here. Then again, Firefox supports
unprefixed transition and transition-delay properties as of Firefox 16 - which
is the lowest version we are supporting already. So I guess dropping
-moz-transition is the better course of action.

http://codereview.adblockplus.org/11039060/diff/164002/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:309: -o-transition-property: all;
As above, -o-transition-property and -moz-transition-property shouldn't be
necessary.

[Wladimir Palant, 2013-10-05 10:15:58]

I pushed the following changesets:

Patchset 15 here: https://hg.adblockplus.org/adblockplus/rev/d1b20ac55a50

Addressing my remaining comments:
https://hg.adblockplus.org/adblockplus/rev/46952b500bfd

Fixing display of Chinese social icons (was this functionality tested?):
https://hg.adblockplus.org/adblockplus/rev/a4012520553e

Updating adblockpluschrome repository:
https://hg.adblockplus.org/adblockpluschrome/rev/db7bf664d382

Sven, Thomas, please look over my changes and confirm that these are fine.

[Thomas Greiner, 2013-10-05 15:51:19]

@Wladimir LGTM

@Sven Found one more thing. I leave it up to you to decide which hover effect
you want (limited to the text or to the whole button, which is the one I
suggested) but the current approach is not acceptable.

http://codereview.adblockplus.org/11039060/diff/164002/chrome/skin/firstRun.css
File chrome/skin/firstRun.css (right):

http://codereview.adblockplus.org/11039060/diff/164002/chrome/skin/firstRun.c...
chrome/skin/firstRun.css:411: #activate-features-label:hover
This is a weird behavior. It's neither restricted to only the text nor to the
whole button due to the text being a block element. Please change it to:

#activate-features:hover a
{
  color: #1f6822;
  text-decoration: none;
}

[Wladimir Palant, 2013-10-08 06:03:35]

On 2013/10/05 15:51:19, Thomas Greiner wrote:
> This is a weird behavior. It's neither restricted to only the text nor to the
> whole button due to the text being a block element. Please change it to:
> 
> #activate-features:hover a
> {
>   color: #1f6822;
>   text-decoration: none;
> }

I pushed that suggestion as
https://hg.adblockplus.org/adblockplus/rev/a7214e6af195, LGTM.

[Thomas Greiner, 2013-10-08 08:25:52]

LGTM