8
\$\begingroup\$

I'm working on a page that shows groups of users. The following JavaScript function is used to show more details about a user (such as email and login) when you click on their name.

But there's a twist: clicking on one of those pieces of information will highlight it to make it easier to copy. This is accomplished by adding an in-line onclick function to each span element containing the user info.

function showUserDetailsInRow(row, user) {
  row.innerHTML = "";
  var expanderCell = document.createElement("div");
  expanderCell.className = "GroupsCell";
  expanderCell.innerHTML = "-";

  var spanOpen = "<span style='font-style:normal; cursor:text;' onclick='var r=document.body.createTextRange(); r.moveToElementText(this); r.select();'>",
   spanClose = "</span>";

  var mainCell = document.createElement("div");
  mainCell.className = "GroupsCell";
  mainCell.innerHTML = "<b>Name:</b> " + spanOpen + user.displayName + spanClose + "<br/>" + (user.isDomainGroup ? "<b>(Domain Group)</b>" : ("<b>Email:</b> " + spanOpen + user.email + spanClose + "<br/>" + "<b>Login:</b> " + spanOpen + user.loginName + spanClose + "<br/>"));

  var removeCell = document.createElement("div");
  removeCell.className = "btn";
  removeCell.innerHTML = "remove from group";

  row.appendChild(expanderCell);
  row.appendChild(mainCell);
  row.appendChild(removeCell);
}

This works just as I'd expect, but it itches a bit.

  • I feel dirty putting code in a string. Is there a better way to accomplish this without greatly increasing the complexity of this function? Would it be worthwhile or more efficient to offload that dynamically repeated inline function into a single function definition elsewhere in my script instead?

  • I also feel guilty building the innerHTML of the mainCell element via string concatenation (and using tags like <b> and <br/>); should I instead be creating <div> elements via document.createElement, decorating them with class names, and appending those to mainCell? Is there a benefit (whether it be performance, complexity, flexibility, or the like) to doing it one way or another?

Here's a working example of the showUserDetailsInRow() function, along with the displayGroupmembers() function that I use to attach it to the mouseup event on row elements. 

var exampleGroup = {
  Members: [{
    displayName: "Marcellus Gilmore Edson",
    email: "[email protected]",
    loginName: "pbc\\medison",
    isDomainGroup: false
  }, {
    displayName: "George Washington Carver",
    email: "[email protected]",
    loginName: "pbc\\gcarver",
    isDomainGroup: false
  }, {
    displayName: "John Harvey Kellogg",
    email: "[email protected]",
    loginName: "pbc\\jkellogg",
    isDomainGroup: false
  }, {
    displayName: "Mister Peanut",
    email: "[email protected]",
    loginName: "pbc\\mpeanut",
    isDomainGroup: false
  }, {
    displayName: "pbc\\Battle Creek Sanitarium Members",
    email: "",
    loginName: "pbc\\Battle Creek Sanitarium Members",
    isDomainGroup: true
  }]
}
displayGroupMembers(exampleGroup);

function showUserDetailsInRow(row, user) {
  row.innerHTML = "";
  var expanderCell = document.createElement("div");
  expanderCell.className = "GroupsCell";
  expanderCell.innerHTML = "-";
  var spanOpen = "<span style='font-style:normal; cursor:text;' onclick='var r=document.body.createTextRange(); r.moveToElementText(this); r.select(); delete r;'>",
    spanClose = "</span>";
  var mainCell = document.createElement("div");
  mainCell.className = "GroupsCell";
  mainCell.innerHTML = "<b>Name:</b> " + spanOpen + user.displayName + spanClose + "<br/>" + (user.isDomainGroup ? "<b>(Domain Group)</b>" : ("<b>Email:</b> " + spanOpen + user.email + spanClose + "<br/>" + "<b>Login:</b> " + spanOpen + user.loginName + spanClose + "<br/>"));
  var removeCell = document.createElement("div");
  removeCell.className = "btn";
  removeCell.innerHTML = "remove from group";
  row.appendChild(expanderCell);
  row.appendChild(mainCell);
  row.appendChild(removeCell);
}

function displayGroupMembers(group) {
  var members = document.getElementById("MembersInnerContainer");
  members.innerHTML = "";
  for (var i = 0, len = group.Members.length; i < len; i++) {
    var memberRow = document.createElement("div");
    memberRow.className = i % 2 === 0 ? "GroupsRow" : "GroupsRow Alt";
    var memberCell = document.createElement("div");
    memberCell.className = "GroupsCell";
    memberCell.innerHTML = "+ " + group.Members[i].displayName;
    memberRow.appendChild(memberCell);
    (function(i, group) {
      memberRow.onmouseup = function(e) {
        e = window.event || e;
        e.target = e.target || e.srcElement;
        if (e.target.tagName !== "SPAN") {
          var isOpen = this.getAttribute("expanded");
          if (isOpen && isOpen === "true") {
            this.setAttribute("expanded", "false");
            this.innerHTML = "";
            var memberCell = document.createElement("div");
            memberCell.className = "GroupsCell";
            memberCell.innerHTML = "+ " + group.Members[i].displayName;
            this.appendChild(memberCell);
          } else {
            this.setAttribute("expanded", "true");
            showUserDetailsInRow(this, group.Members[i]);
          }
        }
      }
    })(i, group);
    members.appendChild(memberRow);
  }
}
.SelectedGroupMembers {
  display: inline-block;
  width: 90%;
  border-width: 1px;
  border-style: solid;
  margin-top: 10px;
  margin-left: auto;
  margin-right: auto;
  margin-bottom: 10px;
  text-align: left;
}
.GroupsHeader {
  width: 100%;
  font-weight: bold;
  padding-top: 5px;
  padding-bottom: 5px;
  cursor: default;
}
.InnerContainer {
  width: 100%;
  overflow: auto;
}
.CreateGroupLink {
  font-weight: bold;
  font-size: 12pt;
  padding-bottom: 3px;
  cursor: pointer;
  border-top-width: 1px;
  border-style: solid;
  border-left: 0px;
  border-right: 0px;
  border-bottom: 0px;
}
.GroupsRow {
  width: 100%;
  color: black;
  cursor: pointer;
  text-align: left;
}
.GroupsCell {
  margin-left: 5px;
  display: inline-block;
  vertical-align: text-top;
}
.GroupsCell span:hover {
  background-color: white;
  color: black;
}
#MembersInnerContainer {
  max-height: 500px;
}
#RefreshGroupMembers {
  display: inline-block;
  float: right;
  padding-left: 5px;
  padding-right: 5px;
  font-size: 8pt;
  margin-right: 5px;
}
.btn {
  padding-left: 4px;
  padding-right: 4px;
  padding-top: 1px;
  padding-bottom: 4px;
  cursor: pointer;
  color: white;
  display: inline-block;
  border-width: 1px;
  border-style: solid;
  margin-bottom: 2px;
}
.btn:hover {
  color: black;
}
[expanded="true"] .GroupsCell {
  font-style: italic;
  margin-left: 6px;
}
.GroupsRow .btn {
  margin-top: 2em;
  vertical-align: text-top;
  font-size: 8pt;
  margin-left: 5px;
}
.GreenTheme .btn {
  background-color: rgb(90, 110, 90);
  border-color: rgb(50, 80, 50);
}
.GreenTheme .btn:hover {
  background-color: rgb(150, 220, 150);
}
.GreenTheme .GroupsHeader {
  color: white;
  background-color: rgb(90, 110, 90);
}
.GreenTheme .InnerContainer {
  background-color: rgb(220, 245, 220);
}
.GreenTheme .SelectedGroupPanel {
  border-color: rgb(90, 110, 90);
  background-color: rgb(240, 250, 240);
  color: black;
}
.GreenTheme .SelectedgroupMembers {
  border-color: rgb(90, 110, 90);
}
.GreenTheme .CreateGroupLink {
  color: rgb(90, 110, 90);
  background-color: rgb(250, 255, 250);
  border-color: rgb(90, 110, 90);
}
.GreenTheme .CreateGroupLink:hover {
  color: black;
  background-color: white;
}
.GreenTheme .GroupsRow.Alt {
  background-color: rgb(250, 255, 250);
}
.GreenTheme .GroupsRow:hover {
  background-color: rgb(165, 225, 165);
}
<div class="GreenTheme">
  <div class="SelectedGroupMembers">
    <div class="GroupsHeader">&nbsp;Group Members
      <div id="RefreshGroupMembers" class="btn">reload</div>
    </div>
    <div class="InnerContainer" id="MembersInnerContainer">
      loading group members...
    </div>
    <div class="CreateGroupLink" id="AddMember">
      &nbsp;+add user to this group
    </div>
  </div>
</div>

(Since this code needs to work on IE8, it's clobbering the mouseup event instead of adding an event listener to it for the sake of brevity and performance. Add that to the list of things I feel guilty about.)

Improve this question
\$\endgroup\$
1
  • \$\begingroup\$ Wow, and as if I didn't already feel guilty enough, I just realized that document.body.createTextRange() is IE-specific. I might have to use the gecko approach of window.getSelection() and selection.addRange() along with document.createRange()... \$\endgroup\$
    – Thriggle
    Nov 7, 2015 at 3:37

1 Answer 1

Reset to default
4
\$\begingroup\$

In the order I spot things (so in no particular order):

  • Don't use innerHTML, especially not with user input. Imagine a user called Bob<script>alert('I am evil');</script>
  • You can use document.createElement() to create elements, document.createTextNode() to create a text node, or element.textContent = ... to set plain text.
  • Don't use explicit styling in your JavaScript. Give it a className and use CSS to style.
  • If you don't use string concatenation to create your elements, you can actually use a proper JavaScript function as the event handler.
  • Even better, you can probably make use of event delegation, since all the elements are of similar functionality.

You might also benefit the use of a templating engine such as Handlebars or Mustache.

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ >"Imagine a user called Bob<script>alert('I am evil');</script>" Adding script tags after the page has already loaded won't do anything, but I definitely see your point; there could be an img element with an in-line onload event, for example. These user properties are coming from our user directory via server side code in this case, so it's less of a concern, but avoiding the vulnerability entirely sounds like a good habit to get into. Thanks for your input! \$\endgroup\$
    – Thriggle
    Nov 9, 2015 at 15:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.