Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTML tags are written using capital letters #54

Closed
wants to merge 6 commits into from
Closed

Conversation

mbest
Copy link
Member

@mbest mbest commented Feb 22, 2012

konrad-kruczynski:
KO searches for and writes HTML tags written using capital letters. That makes no problem with HTML, but will give a lot of them in XHTML. Shouldn't small letters be used instead?

@SteveSanderson
Copy link
Contributor

Hi Konrad

Thanks for reporting this. To be sure I'm fixing the right thing, could you provide a runnable example of some code that will fail? I haven't personally had any issues like this, nor do I know of anyone else who has (though I guess very few people use XHTML these days).

I've just modified part of the code that writes to .innerHTML to use lower case tag names, but I'm unsure whether this is the only part of the code you're referring to, or whether there will be issues elsewhere.

Anyway, if you can create an example of what doesn't work (e.g., on jsfiddle.net - it lets you specify the DTD) that would be really helpful! Thanks

@SteveSanderson
Copy link
Contributor

Whoops, didn't mean to close this. Re-opened.

@konrad-kruczynski
Copy link
Author

I could do not do that using jsfiddle, probably because it is sending content as html anyway. However please follow http://sirius.cs.put.poznan.pl/~inf84832/koxhtml/test.xhtml This is based on your example, select box should be filled with countries' names, however isn't. Opening console on google chrome gives a message "options binding applies only to SELECT elements". If I change capital tag names to lowercase one in ko source, everything works properly.

@mgielda
Copy link

mgielda commented Feb 14, 2011

Hi Steve,

I ran Konrad's example and it still doesn't work with the latest k.o. (1.2.0pre) in XHTML. After changing all "SELECT" strings into "select" and "OPTION" to "option", it starts to work again. Perhaps the comparisons should ignore case?

Cheers,
Michael

@robocoder
Copy link

Not sure how authoritative this source is, but I'll agree with thalain about making the comparison case-insensitive. http://reference.sitepoint.com/javascript/Element/tagName

@SteveSanderson
Copy link
Contributor

Thanks for the additional info - will investigate.

@mbest
Copy link
Member

mbest commented Feb 22, 2012

Here's an informative post about tagName/nodeName: http://ejohn.org/blog/nodename-case-sensitivity/

@SteveSanderson
Copy link
Contributor

Thanks for this Michael!

I've added one final tweak - if we factor the case conversions out into a common function (ko.utils.tagNameUpper), the minified code will be a little smaller, and in the future we could even enable the case conversions conditionally, depending on whether it's an XHTML doc or not.

If you're happy with this, so am I - please go ahead and merge, or just let me know you're satisfied and I'll merge it.

@mbest
Copy link
Member

mbest commented Feb 26, 2012

I like having a common function, but I think it should be lower case. If we did want to make it conditional in the future, we'd convert for HTML (which is case insensitive anyway) and not convert for XHTML (which is case sensitive). So the names would then have to be lower case.

@SteveSanderson
Copy link
Contributor

Not sure I follow you. Why would we convert HTML tag names' case, when
we know they will be uppercase anyway?

The goal isn't to normalise HTML with XHTML; it's just to be able to
do case-insensitive tag name comparisons.

Or am I misunderstanding?

On 26 Feb 2012, at 10:40, Michael Best
reply@reply.github.com
wrote:

I like having a common function, but I think it should be lower case. If we did want to make it conditional in the future, we'd convert for HTML (which is case insensitive anyway) and not convert for XHTML (which is case sensitive). So the names would then have to be lower case.


Reply to this email directly or view it on GitHub:
#54 (comment)

@mbest
Copy link
Member

mbest commented Feb 26, 2012

Why would we convert HTML tag names' case, when we know they will be uppercase anyway?

Makes sense, but we also know that XHTML tag names will always be lower case (in XHTML, <p> is a paragraph tag and <P> isn't). Barring other factors, you could choose either way equally.

If we did want to make the conversion conditional and be sure that XHTML was handled correctly, we should not convert XHTML tag names, and should use lower case for comparison.

Personally, I think the lower case names look better and match the rest of the DOM strings (attributes, type, etc.) that are all lower case. We're also using lower case strings for things like createElement, so let's keep all tag names consistent.

@mbest
Copy link
Member

mbest commented Feb 27, 2012

I changed the code to use lower case tag names. I also found a couple places that still had tagName.toLowerCase() and changed those to use the new common function.

@mbest mbest mentioned this pull request Feb 29, 2012
Use a different set of element type values depending on whether the document is xhtml or html.
@mbest
Copy link
Member

mbest commented Mar 5, 2012

@SteveSanderson - I thought of another way to handle the tag name comparisons. Basically, I have a function that's called with the element whose result is compared to a variable. This allows us a lot of flexibility in determining how the comparison will work. For now, I check the case of the document element and set the element type variables with the matching case.

I'm using vars at the root level so that the compiler can optimize them the best way. For example, the compiler can inline the getElementType function and can remove unused elementType* vars. Originally I was thinking of comparing the element constructors but then realized that it wouldn't work well in a cross-frame environment.

@robocoder
Copy link

Looking at the "Results" section of Resig's article, wouldn't we want the case-insensitive match?

@mbest
Copy link
Member

mbest commented Mar 5, 2012

I realized I could use the element prototypes as strings to avoid the cross-frame issues. I haven't committed this because it adds quite a bit to the file size and I don't know if we want to go this route anyway. But here is the code:

if (typeof HTMLOptGroupElement == "undefined" || document.documentElement != "[object HTMLHtmlElement]") {
    var getElementType = function(element) {
        return element.tagName
    };
    var elementTypes_input = "INPUT",
        elementTypes_form = "FORM",
        elementTypes_select = "SELECT",
        elementTypes_option = "OPTION",
        elementTypes_optgroup = "OPTGROUP",
        elementTypes_script = "SCRIPT",
        elementTypes_textarea = "TEXTAREA";
} else {
    var getElementType = function(element) {
        return "" + element;
    };
    var elementTypes_input = "[object HTMLInputElement]",
        elementTypes_form = "[object HTMLFormElement]",
        elementTypes_select = "[object HTMLSelectElement]",
        elementTypes_option = "[object HTMLOptionElement]",
        elementTypes_optgroup = "[object HTMLOptGroupElement]",
        elementTypes_script = "[object HTMLScriptElement]",
        elementTypes_textarea = "[object HTMLTextAreaElement]";
}

I'm checking specifically for the HTMLOptGroupElement object because IE8 supports element prototypes but doesn't include HTMLOptGroupElement (it uses HTMLOptionElement for both option and opgroup).

@mbest
Copy link
Member

mbest commented Mar 7, 2012

According to the W3C, "using instance-of features to distinguish one subclass of Node from another ... isn't portable and should be avoided." So I guess we should just stick with using tagName.

@SteveSanderson
Copy link
Contributor

Not sure we can do the case-sensitive comparisons as in commit 3ef0767. In the mixed-mode case (HTML doc containing an XHTML frame or vice-versa), we can't rely on the current document's mode to tell us about a given element. Doing the comparisons case-insensitively avoids any such issue.

I've merged as far as 3734c4f into master ("always use lower-case tag names"), so if you're happy with this we can close this issue now. If you think we should pursue the 3ef0767-style mechanism let's continue discussing.

I don't feel strongly about preferring uppercase or lowercase - the reason I went for uppercase was only to optimize perf in the majority of cases (almost everyone uses HTML, hardly anyone uses XHTML) by eliminating string case conversions. It seems a little odd to optimize perf for XHTML by going for lowercase but I haven't benchmarked and I guess the difference would probably be completely negligible. I'm happy to stick with the lowercase - we can always change it again in the future if we need.

@mbest
Copy link
Member

mbest commented Mar 9, 2012

Good point about the cross-frame support. I'm happy with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants