Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am a beginner and I have made Login Form in HTML. I'm pretty sure it will look horrible to any developer, but hey, that's why I've posted it.

I'd like a general review of this. I'm especially concerned about the quality and enhancements of this form.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
<title>Login Page</title>
<style>
/* Basics */
html, body {
    width: 100%;
    height: 100%;
    font-family: "Helvetica Neue", Helvetica, sans-serif;
    color: #444;
    -webkit-font-smoothing: antialiased;
    background: #f0f0f0;
}
#container {
    position: fixed;
    width: 340px;
    height: 280px;
    top: 50%;
    left: 50%;
    margin-top: -140px;
    margin-left: -170px;
    background: #fff;
    border-radius: 3px;
    border: 1px solid #ccc;
    box-shadow: 0 1px 2px rgba(0, 0, 0, .1);

}
form {
    margin: 0 auto;
    margin-top: 20px;
}
label {
    color: #555;
    display: inline-block;
    margin-left: 18px;
    padding-top: 10px;
    font-size: 14px;
}
p a {
    font-size: 11px;
    color: #aaa;
    float: right;
    margin-top: -13px;
    margin-right: 20px;
 -webkit-transition: all .4s ease;
    -moz-transition: all .4s ease;
    transition: all .4s ease;
}
p a:hover {
    color: #555;
}
input {
    font-family: "Helvetica Neue", Helvetica, sans-serif;
    font-size: 12px;
    outline: none;
}
input[type=text],
input[type=password] ,input[type=time]{
    color: #777;
    padding-left: 10px;
    margin: 10px;
    margin-top: 12px;
    margin-left: 18px;
    width: 290px;
    height: 35px;
    border: 1px solid #c7d0d2;
    border-radius: 2px;
    box-shadow: inset 0 1.5px 3px rgba(190, 190, 190, .4), 0 0 0 5px #f5f7f8;
    -webkit-transition: all .4s ease;
    -moz-transition: all .4s ease;
    transition: all .4s ease;
    }
input[type=text]:hover,
input[type=password]:hover,input[type=time]:hover {
    border: 1px solid #b6bfc0;
    box-shadow: inset 0 1.5px 3px rgba(190, 190, 190, .7), 0 0 0 5px #f5f7f8;
}
input[type=text]:focus,
input[type=password]:focus,input[type=time]:focus {
    border: 1px solid #a8c9e4;
    box-shadow: inset 0 1.5px 3px rgba(190, 190, 190, .4), 0 0 0 5px #e6f2f9;
}
#lower {
    background: #ecf2f5;
    width: 100%;
    height: 69px;
    margin-top: 20px;
      box-shadow: inset 0 1px 1px #fff;
    border-top: 1px solid #ccc;
    border-bottom-right-radius: 3px;
    border-bottom-left-radius: 3px;
}
input[type=checkbox] {
    margin-left: 20px;
    margin-top: 30px;
}
.check {
    margin-left: 3px;
    font-size: 11px;
    color: #444;
    text-shadow: 0 1px 0 #fff;
}
input[type=submit] {
    float: right;
    margin-right: 20px;
    margin-top: 20px;
    width: 80px;
    height: 30px;
    font-size: 14px;
    font-weight: bold;
    color: #fff;
    background-color: #acd6ef; /*IE fallback*/
    background-image: -webkit-gradient(linear, left top, left bottom, from(#acd6ef), to(#6ec2e8));
    background-image: -moz-linear-gradient(top left 90deg, #acd6ef 0%, #6ec2e8 100%);
    background-image: linear-gradient(top left 90deg, #acd6ef 0%, #6ec2e8 100%);
    border-radius: 30px;
    border: 1px solid #66add6;
    box-shadow: 0 1px 2px rgba(0, 0, 0, .3), inset 0 1px 0 rgba(255, 255, 255, .5);
    cursor: pointer;
}
input[type=submit]:hover {
    background-image: -webkit-gradient(linear, left top, left bottom, from(#b6e2ff), to(#6ec2e8));
    background-image: -moz-linear-gradient(top left 90deg, #b6e2ff 0%, #6ec2e8 100%);
    background-image: linear-gradient(top left 90deg, #b6e2ff 0%, #6ec2e8 100%);
}
input[type=submit]:active {
    background-image: -webkit-gradient(linear, left top, left bottom, from(#6ec2e8), to(#b6e2ff));
    background-image: -moz-linear-gradient(top left 90deg, #6ec2e8 0%, #b6e2ff 100%);
    background-image: linear-gradient(top left 90deg, #6ec2e8 0%, #b6e2ff 100%);
}
</style>

</head>

<body>

    <!-- Begin Page Content -->
    <div id="container">
        <form action="login_process.php" method="post">
            <label for="loginmsg" style="color:hsla(0,100%,50%,0.5); font-family:"Helvetica Neue",Helvetica,sans-serif;"><?php  echo @$_GET['msg'];?></label>
            <label for="username">Username:</label>
            <input type="text" id="username" name="username">
            <label for="password">Password:</label>
            <input type="password" id="password" name="password">
            <div id="lower">
                <input type="checkbox"><label class="check" for="checkbox">Keep me logged in</label>
                <input type="submit" value="Login">
            </div><!--/ lower-->
        </form>
    </div><!--/ container-->
    <!-- End Page Content -->
</body>
</html>
share|improve this question
    
Since username and password fields are mandatory so you can mark these fields as required. <input type="text" id="username" name="username" required> –  Sandeep Apr 21 at 11:13
    
I don't think the required attribute is a thing in XHTML. –  Thom Wiggers Apr 21 at 11:14
    
Great Question! –  Utkarsh Apr 21 at 11:40
1  
@Sandeep I know, but he's using XHTML. –  Thom Wiggers Apr 21 at 12:01
2  
As @paul mentioned below: Use HTML5. This means use <!DOCTYPE html> instead of what you have right now in your doctype declaration. Reference –  Nikolay Apr 21 at 14:32
show 6 more comments

6 Answers 6

This is one of those places where if you ask 10 developers the same question, you will get at least 15 answers, and they are all valid.

My 2 cents:

  • It's 2014. Use HTML5.

  • Discover jsfiddle.net for this kind of thing.

  • Put "required" on the username and password fields, it's free validation.

  • There's nothing wrong with in-page CSS. I often mix sitewide and in-page as needed. For the purpose of this example, it works just fine. (This does NOT mean ignore CSS classes)

  • Instead of echo @$_GET['msg'] (and resulting security issues) try echo $messages[$_GET['msg']] where $_GET['msg'] is a number. Define your messages server-side.

  • Learn this pattern for unset variables: the ternary operator.

    echo !empty($_GET['msg']) ? $messages[$_GET['msg']] : '' ;
    
  • The login message does not need a label (but it's not "wrong" to have it). incorporate the HTML in the stored message for cleaner output when there is no message

  • Your checkbox needs both a name= and value=

share|improve this answer
    
two questions: 1) what is the specific advantage(s) of HTML5 (in this case)? 2) Why not echo empty($_GET['msg']) ? '' : $messages[$_GET['msg']]; (without the negation)? –  Rolazaro Azeveires Apr 21 at 22:48
1  
@ whoever edited my answer: it's simply "required", not class="required". –  paul Apr 22 at 8:57
2  
@Rolazaro Azeveires: HTML5 does everything XHTML does, and more. Do you still use Windows 2000 or OSX Panther? Both were great in their day. –  paul Apr 22 at 8:59
1  
@Rolazaro Azeveires: There's absolutely nothing wrong with empty($_GET['msg']) ? '' : $messages[$_GET['msg']]; That's just part of the "... 15 answers, and they are all valid" –  paul Apr 22 at 9:01
    
+99 for that reference about Win2K and OSX Panther. LOL. –  azngunit81 Apr 23 at 7:11
show 1 more comment

Security-wise, you need to make sure you a) never trust the user and b) never trust the user.

You should realise that anyone has access to the GET parameters, lines like

<?php  echo @$_GET['msg'];?>

should thus be avoided. Always call htmlspecialchars() on stuff you get from users etc when printing output. Otherwise I could craft an URL like http://yourwebsite/form.php?msg=<script>alert("lololol");</script> and inject arbitrary javascript on your webpages. It's quite easy to trick someone into clicking such a link, making them run my code in their session.

People often find using error suppression (@) bad style, by the way. It's also quite slow.

Also, you need to make sure your form isn't vulnerable to CSRF attacks. Protecting against this is usually done by adding a one-time, per request randomly generated token to your form. This topic is too big to cover here, so I'd ask Google. OWASP is generally a great resource for website security.

HTML-wise, you should pay attention that you are writing XHTML, going by your DOCTYPE. That means you should take care to write valid XML. In XML you need to close every tag. You also need to close tags which only have an 'open' tag, like <img>, <br> and <input>. This is done by adding a forward slash (/) before the closing >. So <input> needs to be <input /> in XHTML.

If you want your form fields to actually submit results, make sure that they all have a name attribute. The checkbox currently doesn't.

Lastly you're using <label> tags quite nicely. You should however note that for needs to reference an id of an <input> tag, so the <label> for the checkbox doesn' reference the correct field.

I'm not quite a CSS wizard so I'll leave that up to someone else, but it looks pretty decent.

Overall, once you fix these minor bugs, it's a better form than many I've seen in the wild :)

share|improve this answer
    
htmlentities() is overkill for most purposes, htmlspecialchars() gets the job done well. –  bjb568 Apr 21 at 22:34
    
@bjb568 I always forget which one to use :) –  Thom Wiggers Apr 22 at 11:10
add comment

I'm not an expert on HTML, but I am quite decent in using it and can give you the following pieces of advice:

  1. Seperate the CSS from your HTML page, it is already good that you have seperated (most) CSS of the HTML elements, but you want to use an external style sheet, and include that in your page. This makes both parts more clear (tip: Do try to not mix programming languages) and allows you to reuse the stylesheet on multiple pages easily.

  2. Use class tags where they are applicable. The class tags can be reused and should be used multiple times generally, while the id tags are for unique elements. This means that <div id="container"> and <div id="lower"> should be a class, and not an id.

  3. Do not secretly sneak in some CSS in your HTML tags. You did it with your loginmsg label, you should give it an id field and reference that in your stylesheet.

  4. I would refrain from suppressing warning messages (the @) in PHP, they are usually there for a reason.

  5. You are (appereantly) sending the msg, which should appear in the loginmsg label, as a GET argument, thus you are receiving user input. In this case it is one that could easily be tampered with, POST arguments can also get tampered, but that is slightly more difficult.
    You should validate your input there, malicious attackers can try to inject javascript code into it, that will redirect someone to another (evil) page, as one of the examples.

This is by no means a complete review, but it are all the points that I have spotted.

share|improve this answer
add comment

I'll only mention items not already addressed in other answers. Viewing the form in Firefox, I see this:enter image description here

I've circled the odd extra space before the Username: label and drawn an arrow to show the missing bottom of the box.

  1. The extra space is what the user will see in the case that your PHP script doesn't return a message. One way to handle that would be to simply insert a <br/> tag between the two labels.

  2. The missing bottom of the box is due to the way you've sized the container in your CSS. It's a wee bit short.

share|improve this answer
    
Actually, <label for="loginmsg"> shouldn't be a label at all, as there is no corresponding <input id="loginmsg"> form element. Furthermore, it should not be emitted at all if there is no message to display. –  200_success Apr 21 at 17:35
add comment

I got width scroll bar window in my browser.

`

html, body {
    width: 100%;
    height: 100%;
}

`

I am getting unnecessary scroll bars.

Width and height are not needed, please simply remove them from your CSS.

share|improve this answer
add comment

Another thing:

Wrap your checkbox within it's label, so that both the box and the "Keep me logged in" text are clickable and will de/activate the check. It's not essential, but it's good UX.

<div id="lower">
    <label class="check" for="checkbox"><input type="checkbox"> Keep me logged in</label>
    <input type="submit" value="Login">
</div>
share|improve this answer
    
Instead of wrapping the input in label, you could simply add an id for the input that matches the label's for. <label class="check" for="checkbox">Keep me logged in</label><input type="checkbox" id="checkbox"/> –  SNag Apr 22 at 14:24
    
Also, when wrapping an input inside a label, the for attribute can be omitted; use for only if you don't wrap the input inside a label. Both approaches are right (wrapping or for), just pick whatever fits your need. –  thaJeztah Apr 25 at 19:43
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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