When in view source mode, the attributes of a tag after an attribute with an empty value are not displayed correctly. For example, the HTML page at http://alioth.debian.org/ has this img tag: <img src="/themes/gforge/images/logo.png" border="0" alt="" width="198" height="52" /> Note that the alt attribute has an empty value. In the view source mode, that img tag is displayed like this: <img src="/themes/gforge/images/logo.png" border="0" alt="198" height="52" ="" /> Note that the alt attribute and the attributes after it are incorrect. The problem occurs in HTMLViewSourceDocument::addViewSourceToken: Attribute* attr = token->attrs->attributeItem(currAttr); String name = attr->name().toString(); if (doctype) addText(name, "webkit-html-doctype"); else { m_current = addSpanWithClassName("webkit-html-attribute-name"); addText(name, "webkit-html-attribute-name"); if (m_current != m_tbody) m_current = static_cast<Element*>(m_current->parent()); } if (attr->value().isNull() || attr->value().isEmpty()) currAttr++; <=== PROBLEM The last statment, currAttr++, skips an empty attribute value, causing attribute (name, value) pairs to get out of sync.
Created attachment 17215 [details] Preliminary patch for discussion This patch is intended to indicate the location of the bug and suggest a possible fix. I don't know the code well enough to know if this fix is correct. I first tried removing the currAttr++ if statement, but that broke the "!DOCTYPE" tag. So in this patch I moved the currAttr++ if statement into the if (doctype) block. I wonder if we can simply do currAttr++ without checking if attr->value() is null or empty: if (doctype) { addText(name, "webkit-html-doctype"); currAttr++; } else {
David, I'd appreciate your comments on this view source mode bug and my fix. Thanks.
What version of Safari/WebKit are you seeing this on? I suggest setting the "review?" flag on the patch even though it's not for committing. That's how they get reviewed. Thanks!
Comment on attachment 17215 [details] Preliminary patch for discussion I'm using the latest revision of WebKit/WebCore/html/HTMLViewSourceDocument.cpp, r21704. I'm not using Safari. Safari's "View Source" command doesn't seem to be implemented using the HTMLViewSourceDocument class.
(In reply to comment #4) > (From update of attachment 17215 [details] [edit]) > Safari's "View Source" command doesn't seem to be > implemented using the > HTMLViewSourceDocument class. The Source view in the Web Inspector uses it.
Thanks. Yes, you can confirm this bug using the Source view in the Web Inspector.
I don't think hyatt is the right reviewer for these. (I don't see any comments from him in this bug, and he's certainly not the only person to have worked on the HTML Tokenizer or HTML serialization). Marking these as "review by hyatt" will only make them slower to be reviewed.
Eric, could you suggest the right reviewer for my patches? I found that hyatt is the only person who checked in changes to HTMLViewSourceDocument.cpp in the svn log. That's why I asked him to review my patches.
Usually, we just leave the name empty, so any reviewer who feels qualified can review the patch.
Comment on attachment 17215 [details] Preliminary patch for discussion Looks generally ok, but needs a ChangeLog and test cases. r-
Created attachment 18001 [details] Proposed patch v1 I added a ChangeLog entry. Where are the test cases for the Web Inspector located?
(In reply to comment #11) > Where are the test cases for the Web Inspector located? Since the Web Inspector uses WebKit itself to do rendering, there are (currently) no special web-inspector-specific tests. Having said that, if you figure out a way to write web-inspector-specific tests (especially if they may be automated), they would certainly be appreciated!
(In reply to comment #11) > Where are the test cases for the Web Inspector located? There are a couple in fast/inspector, and there is a View Source test in fast/frames.
Created attachment 18053 [details] Proposed patch v1 (with a new regression test) Thank you for the hints. I added a new regression test for this bug. Please review the patch. Thanks!
Comment on attachment 18053 [details] Proposed patch v1 (with a new regression test) I think the patch breaks the no-value case <element attrA attrB="valB">
Created attachment 18063 [details] Proposed patch v2 Mitz, you're right. Thanks for pointing that out. I studied the code more and I think I understand the code now. The key is to study HTMLTokenizer::parseTag. This patch should be correct, although it may not be the best fix. 1. I believe that it is not necessary to test attr->value().isNull() because in HTMLTokenizer::parseTag an attribute with no value is added with: currToken.addAttribute(m_doc, attrName, emptyAtom, inViewSourceMode()); 2. I use 'begin' because it equals 'i + 1', which is what I need. If you think using 'i + 1' is clearer than 'begin', I can do that.
Created attachment 18065 [details] Proposed patch v2.1 I improved the regression test a little bit.
Created attachment 18076 [details] Proposed patch v3 I figured out a better fix. It's much easier to determine when we start processing the next attribute than to determine when we're done with the current attribute. So whenever we see the guide character 'a', signaling the beginning of an attribute, we increment currAttr. This requires initializing currAttr to -1 so that after the first increment it starts at 0. So I have to declare currAttr as 'int', and have to add a static cast to unsigned when I compare it with token->attrs->length(), an unsigned value.
Comment on attachment 18076 [details] Proposed patch v3 This patch is correct, but it leaves the code in poor shape. I think it would be better to define the 'attr' variable outside the the for loop, then when you see an 'a', assign the current attribute to 'attr' and increment 'currAttr'. Then you won't need the assignment in the else clause (since 'attr' will have been assigned before) and you won't need to start the counter at -1. The regression test should not dump the render tree, but instead do a text dump. This is accomplished by adding something like <script> if (window.layoutTestController) { layoutTestController.dumpAsText(); layoutTestController.dumpChildFramesAsText(); } </script> to the test. layoutTestController is an interface DumpRenderTree provides to tests to control its behavior.
Comment on attachment 18076 [details] Proposed patch v3 r- based on comments from Mitz - please address those and repost. Thanks!
Created attachment 18245 [details] Proposed patch v4 Mitz, thanks a lot for the review. I made your suggested changes in this patch. Note that I set attr to NULL if (token->attrs && currAttr < token->attrs->length()) is false, and test for a non-NULL attr to match the behavior of the original code as much as possible. I considered making the simpler change below, but am not sure if it's safe: Index: HTMLViewSourceDocument.cpp =================================================================== --- HTMLViewSourceDocument.cpp (revision 29095) +++ HTMLViewSourceDocument.cpp (working copy) @@ -128,6 +128,7 @@ unsigned size = guide->size(); unsigned begin = 0; unsigned currAttr = 0; + Attribute* attr = NULL; for (unsigned i = 0; i < size; i++) { if (guide->at(i) == 'a' || guide->at(i) == 'x' || guide->at(i) == 'v') { // Add in the string. @@ -135,9 +136,8 @@ begin = i + 1; - if (token->attrs && currAttr < token->attrs->length()) { if (guide->at(i) == 'a') { - Attribute* attr = token->attrs->attributeItem(currAttr); + attr = token->attrs->attributeItem(currAttr++); String name = attr->name().toString(); if (doctype) addText(name, "webkit-html-doctype"); @@ -147,10 +147,7 @@ if (m_current != m_tbody) m_current = static_cast<Element*>(m_current->parent()); } - if (attr->value().isNull() || attr->value().isEmpty()) - currAttr++; } else { - Attribute* attr = token->attrs->attributeItem(currAttr); String value = attr->value().domString(); if (doctype) addText(value, "webkit-html-doctype"); @@ -164,9 +161,7 @@ if (m_current != m_tbody) m_current = static_cast<Element*>(m_current->parent()); } - currAttr++; } - } } }
Comment on attachment 18245 [details] Proposed patch v4 In general this fix looks fine. The style guidelines require using 0 instead of NULL for new C++ code. Also, an ideal test case would say "PASS" or "FAILURE" so as not to require user interaction. However the change as a whole looks fine. I'll let someone who actually has knowledge about the viewsource code give the final review.
Created attachment 18315 [details] Proposed patch v4.1 The only difference from patch v4 is to use 0 instead of NULL for null pointers. I think I can improve the patch more if I understand the code better. Specifically, I suspect that the condition (token->attrs && currAttr < token->attrs->length()) must be true because the guide string and the attribute array should be created in sync in HTMLTokenizer.cpp; if the condition (token->attrs && currAttr < token->attrs->length()) is false, I believe it indicates a bug in HTMLTokenizer::parseTag().
Comment on attachment 18315 [details] Proposed patch v4.1 r=me But can't we also fix this by distinguishing null attribute values and empty ones?
Landed in <http://trac.webkit.org/projects/webkit/changeset/29427>.
(In reply to comment #25) > Landed in <http://trac.webkit.org/projects/webkit/changeset/29427>. Is this bug resolved then? :)
(In reply to comment #26) > (In reply to comment #25) > > Landed in <http://trac.webkit.org/projects/webkit/changeset/29427>. > > Is this bug resolved then? :) > Yes. Sorry, and thank you Dave and Eric.