Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Make SVGs and CSS dynamically recolourable #77

Merged
merged 15 commits into from
Jan 7, 2016
6 changes: 6 additions & 0 deletions src/SlashCommands.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ var MatrixClientPeg = require("./MatrixClientPeg");
var MatrixTools = require("./MatrixTools");
var dis = require("./dispatcher");
var encryption = require("./encryption");
var Tinter = require("./Tinter");

var reject = function(msg) {
return {
Expand All @@ -42,6 +43,11 @@ var commands = {
return reject("Usage: /nick <display_name>");
},

tint: function(room_id, args) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(most of) the other commands have a comment saying what they do. wouldn't hurt here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Tinter.tint(args);
return success();
},

encrypt: function(room_id, args) {
if (args == "on") {
var client = MatrixClientPeg.get();
Expand Down
214 changes: 214 additions & 0 deletions src/Tinter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
/*
Copyright 2015 OpenMarket Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

var dis = require('./dispatcher');

var registered = false;
if (!registered) {
dis.register(_onAction);
}

var keyRgb = [
"rgb(118, 207, 166)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic numbers are magic

what do these correspond to?

Can we calculate these rather than hard-coding them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented to clarify where they came from. They can't really be calculated as they're constants describing shades of green.

"rgb(234, 245, 240)",
"rgba(118, 207, 166, 0.2)",
];

// Some algebra workings for calculating the tint % of Vector Green & Light Green
// x * 118 + (1 - x) * 255 = 234
// x * 118 + 255 - 255 * x = 234
// x * 118 - x * 255 = 234 - 255
// (255 - 118) x = 255 - 234
// x = (255 - 234) / (255 - 118) = 0.16

var keyHex = [
"#76CFA6",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"#EAF5F0",
"#D3EFE1",
];

// cache of our replacement colours
// defaults to our keys.
var colors = [
keyHex[0],
keyHex[1],
keyHex[2],
];

var cssFixups = [
// {
// style: a style object that should be fixed up taken from a stylesheet
// attr: name of the attribute to be clobbered, e.g. 'color'
// index: ordinal of primary, secondary or tertiary
// }
];

// CSS attributes to be fixed up
var cssAttrs = [
"color",
"backgroundColor",
"borderColor",
];

var svgFixups = [
// {
// node: a SVG node that needs to be fixed up
// attr: name of the attribute to be clobbered, e.g. 'fill'
// index: ordinal of primary, secondary
// }
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I finally grok how and why you're doing this. I don't like it because it is leaking DOM nodes all over the place because Tinter is a singleton when really the things it is manipulating are UI elements. Either strip this class to be pure-functional, or move it all to a separate UI component (the one you're writing). This means that you would have something like:

React.createClass({

  _fixup: function(event) {
    // do fixup logic that Tinter.js is doing
  },

  render: function() {
    return <object onload={this._fixup} className="mx_Svg" type="image/svg+xml" data="img/leave.svg" />
  }
});

This is much nicer since this will drop out of scope when the component is unmounted, stopping any leaks. My next questions would be:

  • Do you need mx_Svg anymore given you no longer need to do a full-scan of document?

  • You still need to retroactively apply tints to mounted svg components when people type /tint. This would be something that would be useful to be done via the dispatcher so you don't need to maintain a list of currently mounted svg components. So you'd have:

    onAction: function(payload) {
      if (payload.action !== "tint_change") { return; }
      // apply new tint values from payload
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this would be a nicer architecture and stop it from leaking DOM nodes everywhere. However, how would the tint_change dispatch know where to find all of the tintable elements in order to tint them in the '// apply new tint values from payload' comment above?

I feel a bit inclined to move the proposed restructure here into a maintenance bug rather than invest more time in making the architecture perfect, given the leaks will be very minor.


var svgAttrs = [
"fill",
"stroke",
];

var svgNodes = {};

var cached = false;

function calcCssFixups() {
for (var i = 0; i < document.styleSheets.length; i++) {
var ss = document.styleSheets[i];
for (var j = 0; j < ss.cssRules.length; j++) {
var rule = ss.cssRules[j];
for (var k = 0; k < cssAttrs.length; k++) {
var attr = cssAttrs[k];
for (var l = 0; l < keyRgb.length; l++) {
if (rule.style && rule.style[attr] === keyRgb[l]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add if (!rule.style) continue; at line 88, and simplify this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, although it's very rare for a CSS rule to not have a style (it happens with weird rules like animation data which don't actually define styles), so it doesn't buy us much. have done it anyway.

cssFixups.push({
style: rule.style,
attr: attr,
index: l,
});
}
}
}
}
}
}

function calcSvgFixups(nodes) {
var svgs = nodes || document.getElementsByClassName("mx_Svg");
var fixups = [];
for (var i = 0; i < svgs.length; i++) {

var svgDoc = svgs[i].contentDocument;
if (!svgDoc) continue;
var tags = svgDoc.getElementsByTagName("*");
for (var j = 0; j < tags.length; j++) {
var tag = tags[j];
for (var k = 0; k < svgAttrs.length; k++) {
var attr = svgAttrs[k];
for (var l = 0; l < keyHex.length; l++) {
if (tag.getAttribute(attr) && tag.getAttribute(attr).toUpperCase() === keyHex[l]) {
fixups.push({
node: tag,
attr: attr,
index: l,
});
}
}
}
}
}

return fixups;
}

function applyCssFixups() {
for (var i = 0; i < cssFixups.length; i++) {
var cssFixup = cssFixups[i];
cssFixup.style[cssFixup.attr] = colors[cssFixup.index];
}
}

function applySvgFixups(fixups) {
for (var i = 0; i < fixups.length; i++) {
var svgFixup = fixups[i];
svgFixup.node.setAttribute(svgFixup.attr, colors[svgFixup.index]);
}
}

function _onAction(payload) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is almost entirely isolated from the rest of the project if it weren't for listening on the global dispatcher like this. I would prefer to do a direct function call to the Tinter from the new Tinter component you're writing as per:

<M-matthew> actually in fact if the svg thing was its own component it could probably handle it relatively elegantly

This matches other logic files which also have a set of public-facing methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, as it stands, does this not race? Every tintable thing fires off a svg_onload event, and we don't seem to specify which things we want to tint anywhere so this will be tinting ALL THE THINGS! The only reason why it doesn't is because it doesn't get svg_onload events fired at it all the time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to call Tinter directly rather than via dispatch. It doesn't tint 'all the things' as it passes the specific SVG DOM element to be tinted.

if (payload.action !== "svg_onload") return;
// XXX: we should probably faff around with toggling the visibility of the node to avoid flashing the wrong colour.
// (although this would result in an even worse flicker as the element redraws)
var fixups = calcSvgFixups([ payload.svg ]);
if (fixups.length) {
svgFixups = svgFixups.concat(fixups); // XXX: this leaks fixups
applySvgFixups(fixups);
}
}

function hexToRgb(color) {
if (color[0] === '#') color = color.slice(1);
if (color.length === 3) {
color = color[0] + color[0] +
color[1] + color[1] +
color[2] + color[2];
}
var val = parseInt(color, 16);
var r = (val >> 16) & 255;
var g = (val >> 8) & 255;
var b = val & 255;
return [r, g, b];
}

function rgbToHex(rgb) {
var val = (rgb[0] << 16) | (rgb[1] << 8) | rgb[2];
return '#' + (0x1000000 + val).toString(16).slice(1)
}

module.exports = {
tint: function(primaryColor, secondaryColor, tertiaryColor) {
if (!cached) {
calcCssFixups();
svgFixups = calcSvgFixups();
cached = true;
}

if (!secondaryColor) {
var x = 0.16; // average weighting factor calculated from vector green & light green
var rgb = hexToRgb(primaryColor);
rgb[0] = x * rgb[0] + (1 - x) * 255;
rgb[1] = x * rgb[1] + (1 - x) * 255;
rgb[2] = x * rgb[2] + (1 - x) * 255;
secondaryColor = rgbToHex(rgb);
}

if (!tertiaryColor) {
var x = 0.19;
var rgb1 = hexToRgb(primaryColor);
var rgb2 = hexToRgb(secondaryColor);
rgb1[0] = x * rgb1[0] + (1 - x) * rgb2[0];
rgb1[1] = x * rgb1[1] + (1 - x) * rgb2[1];
rgb1[2] = x * rgb1[2] + (1 - x) * rgb2[2];
tertiaryColor = rgbToHex(rgb1);
}

colors = [primaryColor, secondaryColor, tertiaryColor];

// go through manually fixing up the stylesheets.
applyCssFixups();

// go through manually fixing up SVG colours.
// we could do this by stylesheets, but keeping the stylesheets
// updated would be a PITA, so just brute-force search for the
// key colour; cache the element and apply.
applySvgFixups(svgFixups);
}
};
32 changes: 20 additions & 12 deletions src/components/structures/RoomView.js
Original file line number Diff line number Diff line change
Expand Up @@ -1093,6 +1093,10 @@ module.exports = React.createClass({
});
},

onSvgLoad: function(event) {
dis.dispatch({ action: "svg_onload", svg: event.target });
},

render: function() {
var RoomHeader = sdk.getComponent('rooms.RoomHeader');
var MessageComposer = sdk.getComponent('rooms.MessageComposer');
Expand Down Expand Up @@ -1174,7 +1178,7 @@ module.exports = React.createClass({
if (this.state.syncState === "ERROR") {
statusBar = (
<div className="mx_RoomView_connectionLostBar">
<img src="img/warning.svg" width="24" height="23" alt="/!\ "/>
<img src="img/warning.svg" width="24" height="23" title="/!\ " alt="/!\ "/>
<div className="mx_RoomView_connectionLostBar_textArea">
<div className="mx_RoomView_connectionLostBar_title">
Connectivity to the server has been lost.
Expand All @@ -1193,8 +1197,8 @@ module.exports = React.createClass({
<div className="mx_RoomView_tabCompleteImage">...</div>
<div className="mx_RoomView_tabCompleteWrapper">
<TabCompleteBar entries={this.tabComplete.peek(6)} />
<div className="mx_RoomView_tabCompleteEol">
<img src="img/eol.svg" width="22" height="16" alt="->|"/>
<div className="mx_RoomView_tabCompleteEol" title="->|">
<object onLoad={ this.onSvgLoad } className="mx_Svg" type="image/svg+xml" data="img/eol.svg" width="22" height="16"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to factor out this whole <div> structure, complete with its onSvgLoad, to a separate react component so you don't have to c&p the idiom 150 times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Auto-complete
</div>
</div>
Expand All @@ -1204,7 +1208,7 @@ module.exports = React.createClass({
else if (this.state.hasUnsentMessages) {
statusBar = (
<div className="mx_RoomView_connectionLostBar">
<img src="img/warning.svg" width="24" height="23" alt="/!\ "/>
<img src="img/warning.svg" width="24" height="23" title="/!\ " alt="/!\ "/>
<div className="mx_RoomView_connectionLostBar_textArea">
<div className="mx_RoomView_connectionLostBar_title">
Some of your messages have not been sent.
Expand Down Expand Up @@ -1267,8 +1271,8 @@ module.exports = React.createClass({
var fileDropTarget = null;
if (this.state.draggingFile) {
fileDropTarget = <div className="mx_RoomView_fileDropTarget">
<div className="mx_RoomView_fileDropTargetLabel">
<img src="img/upload-big.svg" width="45" height="59" alt="Drop File Here"/><br/>
<div className="mx_RoomView_fileDropTargetLabel" title="Drop File Here">
<object onLoad={ this.onSvgLoad } className="mx_Svg" type="image/svg+xml" data="img/upload-big.svg" width="45" height="59"/><br/>
Drop File Here
</div>
</div>;
Expand Down Expand Up @@ -1305,25 +1309,29 @@ module.exports = React.createClass({

if (call.type === "video") {
zoomButton = (
<div className="mx_RoomView_voipButton" onClick={this.onFullscreenClick}>
<img src="img/fullscreen.svg" title="Fill screen" alt="Fill screen" width="29" height="22" style={{ marginTop: 1, marginRight: 4 }}/>
<div className="mx_RoomView_voipButton" onClick={this.onFullscreenClick} title="Fill screen">
<object onLoad={ this.onSvgLoad } className="mx_Svg" type="image/svg+xml" data="img/fullscreen.svg" width="29" height="22" style={{ marginTop: 1, marginRight: 4 }}/>
</div>
);

videoMuteButton =
<div className="mx_RoomView_voipButton" onClick={this.onMuteVideoClick}>
<img src={call.isLocalVideoMuted() ? "img/video-unmute.svg" : "img/video-mute.svg"} width="31" height="27"/>
<img src={call.isLocalVideoMuted() ? "img/video-unmute.svg" : "img/video-mute.svg"}
alt={call.isLocalVideoMuted() ? "Click to unmute video" : "Click to mute video"}
width="31" height="27"/>
</div>
}
voiceMuteButton =
<div className="mx_RoomView_voipButton" onClick={this.onMuteAudioClick}>
<img src={call.isMicrophoneMuted() ? "img/voice-unmute.svg" : "img/voice-mute.svg"} width="21" height="26"/>
<img src={call.isMicrophoneMuted() ? "img/voice-unmute.svg" : "img/voice-mute.svg"}
alt={call.isMicrophoneMuted() ? "Click to unmute audio" : "Click to mute audio"}
width="21" height="26"/>
</div>

if (!statusBar) {
statusBar =
<div className="mx_RoomView_callBar">
<img src="img/sound-indicator.svg" width="23" height="20" alt=""/>
<img src="img/sound-indicator.svg" width="23" height="20"/>
<b>Active call</b>
</div>;
}
Expand All @@ -1334,7 +1342,7 @@ module.exports = React.createClass({
{ videoMuteButton }
{ zoomButton }
{ statusBar }
<img className="mx_RoomView_voipChevron" src="img/voip-chevron.svg" width="22" height="17"/>
<object onLoad={ this.onSvgLoad } type="image/svg+xml" className="mx_RoomView_voipChevron mx_Svg" data="img/voip-chevron.svg" width="22" height="17"/>
</div>
}

Expand Down
7 changes: 6 additions & 1 deletion src/components/views/messages/MFileBody.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ limitations under the License.
var React = require('react');
var filesize = require('filesize');
var MatrixClientPeg = require('../../../MatrixClientPeg');
var dis = require("../../../dispatcher");

module.exports = React.createClass({
displayName: 'MFileBody',
Expand All @@ -45,6 +46,10 @@ module.exports = React.createClass({
return linkText;
},

onSvgLoad: function(event) {
dis.dispatch({ action: "svg_onload", svg: event.target });
},

render: function() {
var content = this.props.mxEvent.getContent();
var cli = MatrixClientPeg.get();
Expand All @@ -57,7 +62,7 @@ module.exports = React.createClass({
<span className="mx_MFileBody">
<div className="mx_MImageBody_download">
<a href={cli.mxcUrlToHttp(content.url)} target="_blank">
<img src="img/download.png" width="10" height="12"/>
<object onLoad={ this.onSvgLoad } className="mx_Svg" type="image/svg+xml" data="img/download.svg" width="12" height="14"/>
Download {text}
</a>
</div>
Expand Down
7 changes: 6 additions & 1 deletion src/components/views/messages/MImageBody.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ var filesize = require('filesize');
var MatrixClientPeg = require('../../../MatrixClientPeg');
var Modal = require('../../../Modal');
var sdk = require('../../../index');
var dis = require("../../../dispatcher");

module.exports = React.createClass({
displayName: 'MImageBody',
Expand Down Expand Up @@ -96,6 +97,10 @@ module.exports = React.createClass({
return MatrixClientPeg.get().mxcUrlToHttp(content.url, 480, 360);
},

onSvgLoad: function(event) {
dis.dispatch({ action: "svg_onload", svg: event.target });
},

render: function() {
var content = this.props.mxEvent.getContent();
var cli = MatrixClientPeg.get();
Expand All @@ -118,7 +123,7 @@ module.exports = React.createClass({
</a>
<div className="mx_MImageBody_download">
<a href={cli.mxcUrlToHttp(content.url)} target="_blank">
<img src="img/download.png" width="10" height="12"/>
<object onLoad={ this.onSvgLoad } className="mx_Svg" type="image/svg+xml" data="img/download.svg" width="12" height="14"/>
Download {content.body} ({ content.info && content.info.size ? filesize(content.info.size) : "Unknown size" })
</a>
</div>
Expand Down
Loading