Bug 145570 – Green ✓ color on build-safari.apple.com too bright. Added color selection.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Matthew Daiter]

New green color is too bright for some people. Needed to make a toggle switch to change coloring of the checkmarks/status indicators.

[Matthew Daiter]

<rdar://problem/20994468>

[Matthew Daiter]

Created attachment 254112 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 254112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254112&action=review

> Tools/ChangeLog:3
> +        Changed the greens/reds so that there would be less eye-strain

Could you please attach a screenshot for how the preferences look?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/index.html:37
> +    <script src="Scripts/Deuteranopia.js"></script>

We already have Settings.js, I don't see a need for a new file implementing another bit of settings.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Deuteranopia.js:1
> +//Meant to be used for the Deu attributes button

WebKit style is to have a space after "//", a full sentence, and then a period.

Also, we almost never abbreviate any words.

I don't think that spelling out deuteranopia is needed, I'd call it "accessible colors" throughout the code and in UI.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Deuteranopia.js:3
> +function changePreferencesForColor(){
> +  var deu = getDeuCookie("deu");

WebKit style is to have the brace on a new line, and 4 space indents. Please take a look at <http://www.webkit.org/coding/coding-style.html>.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Deuteranopia.js:6
> +    setDeuCookie("deu", "false");

If you were adding code to Settings.js instead of a new file, it would be easier to see that we use a different technique for storing settings.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Deuteranopia.js:10
> +  else{
> +    setDeuCookie("deu", "true");
> +  }

No braces around single line blocks.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Deuteranopia.js:15
> +  //Loading cookies

We very rarely add comments explaining what the code does. We use comments to explain why it does certain non-obvious things.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Deuteranopia.js:16
> +  var checkmark_deu = getDeuCookie("deu");

Names should be camelCase.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Deuteranopia.js:26
> +    link.href = 'Styles/StatusLineViewBubbleDEU.css';

Switching stylesheets is unnecessarily complicated, you can just use document.body.classList.toggle("accessibleColors"), as we do for showing settings, and make bubble colors depend on that class.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:85
> +

This is a spurious edit, please revert it.

[Matthew Daiter]

Created attachment 254132 [details]
Patch

[WebKit Commit Bot]

Attachment 254132 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:294:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 254132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254132&action=review

Thank you, this approach looks much better.

> Tools/ChangeLog:7
> +        Mainly shifting CSS around, doing off-loading of CSS files from browser to change colors. Used cookies to store colors. Colors necessary for accessibility. Main goal for this iteration was to cut down on code

Please update the ChangeLog, it is out of date now.

We usually wrap ChangeLogs to some reasonable width, please match entries below.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/index.html:34
> -
> +    

Please revert this spurious change.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:139
> +function loadAccessibilityColors(){

WebKit style is to put the brace on a new line.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:142
> +    if (!isCurrentlyActivated)
> +        isCurrentlyActivated = false;

What is the purpose of this assignment?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:144
> +        toggleAccessibleColors();

This function doesn't exist.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:145
> +}

I would write this function like this:

I would write it like this:

function applyAccessibleColorSetting()
{
    var useAccessibleColors = settings.getObject("useAccessibleColors");
    if (useAccessibleColors)
        toggleAccessibleColors();
}

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:156
> +    if (isCurrentlyActivated == true)
> +        settings.setObject("accessibilityOptions", false);
> +    else
> +        settings.setObject("accessibilityOptions", true);

There is no need to use an if statement to flip a boolean, you could simply write:

settings.setObject("useAccessibleColors", !isCurrentlyActivated);

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:168
> +    var toggle = document.createElement("div");

Please create the variable just before it's used, and use a more descriptive name.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:178
> +
>      unhideButton.addEventListener("click", function () { settings.clearHiddenPlatforms(); });
>      unhideButton.textContent = "Show All Platforms";
>      unhideButton.classList.add("cellButton", "unhide", "hidden");
> +
>      header.appendChild(unhideButton);
>      row.appendChild(header);
>  
> +

Please revert these spurious changes.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:291
> +        toggle.setAttribute("class", "cellButton unhide hidden");

This element ends up being positioned strangely, under the last column heading ("Other"). As it's not only about this header, it should be elsewhere, in a location that clearly indicated that it's a global setting that applies to all bubbles.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:294
> +	settings.addSettingListener("toggleAccessibility", updateAccessibilityColors);

A tab character here.

I think that changing settings, dispatching a notification and listening for it is a little too roundabout, we don't expect to have any other listeners. This change can be applied directly from click event handler.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:300
> +            toggleAccessibleColors();

This function doesn't seem to exist.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:106
> +        document.body.classList.toggle("accessible-colors");

This should be in Main.js, not in Settings.js. The Settings object is responsible for storing the settings, and is partially responsible for displaying them, but should not be responsible for implementing their effects.

[Matthew Daiter]

Created attachment 254190 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 254190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254190&action=review

Almost good to go!

> Tools/ChangeLog:10
> +        Mainly shifting CSS around, doing off-loading of CSS files from 
> +        browser to change colors. Used cookies to store colors. Colors 
> +        necessary for accessibility. Main goal for this iteration was to 
> +        cut down on code. Cut down on notifications, other code bogging down system

The ChangeLog comment should describe the patch overall, not just the changes form the previous iteration. This is what will be landed to svn, and remain in eternity.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:142
> +    if (useAccessibleColors){

Please remove braces around the single line statement (and if there were multiple lines, we'd need a space before the opening brace).

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:147
> +function updateAccessibilityColors()

Maybe toggleAccessibleColors()? We should also standardize on one form, either "accessible colors" or "accessibility colors".

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:150
> +    if (isCurrentlyActivated == undefined)

We use "===" in JS code to make the behavior more predictable. I don't think that it matters in this case, but it would be good to do the same for consistency.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:284
> +        toggleAccessibilityColorButton.setAttribute("class", "cellButton unhide hidden accessibilityButton");

I don't think that cellButton is right any more, as the button is not in a cell.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:285
> +        toggleAccessibilityColorButton.textContent = "enable accessibility colors";

As discussed in person, this should change to "disable accessibility(accessible?) colors" when the setting is already enabled.

[Matthew Daiter]

Created attachment 254212 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 254212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254212&action=review

All my comments are nitpicks at this point, so r+. Since you don't have committer access yet, you can upload a new patch with "Reviewed by Alexey Proskuryakov." in the ChangeLog, and only mark it cq?, not r?. Or you can just upload normally if you'd like to get one final review round.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:139
> +function applyAccessibleColorSetting()

Still some inconsistent wording: accessible instead of accessibility as used in most places.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:142
> +    var toggleAccessibilityColorButton = document.querySelectorAll("div.unhide.hidden.accessibilityButton")[0];

Please just add an id for the button.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:147
> +    }
> +    else

Else should go on the same line with the brace:

    } else

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:159
> +    var toggleAccessibilityColorButton = document.querySelectorAll("div.unhide.hidden.accessibilityButton")[0];

Please use the id here too.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:179
> -    var header = document.createElement("th");
>      var unhideButton = document.createElement("div");
>      unhideButton.addEventListener("click", function () { settings.clearHiddenPlatforms(); });
>      unhideButton.textContent = "Show All Platforms";
>      unhideButton.classList.add("cellButton", "unhide", "hidden");
> +
> +    var header = document.createElement("th"); 

Please undo this change. The code was easier to read when elements inside the header were created after the line that created the header element.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Styles/Main.css:208
> +    top: 13.5px;

Did you check how this looks on a non-Retina screen? I don't know what to expect, but sometimes subpixel positioning ends up causing trouble when there are no subpixels

[Matthew Daiter]

Created attachment 254229 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 254229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254229&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:173
> -    var header = document.createElement("th");
> +    var header = document.createElement("th"); 

Still a spurious edit here.

It is always a good idea to check a patch right before uploading it, essentially as your own review pass.

[WebKit Commit Bot]

Comment on attachment 254229 [details]
Patch

Clearing flags on attachment: 254229

Committed r185193: <http://trac.webkit.org/changeset/185193>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matthew Daiter]

Reopening to attach new patch.

[Matthew Daiter]

Created attachment 254322 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 254322 [details]
Patch

I can see why this can be seen as desirable, but we'd get overlapped elements on smaller screens with this. Also, the correct marking would have been r?, not r+.

[Matthew Daiter]

Alright, makes sense. Will leave as is (with absolute positioning)