bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

DOM HTMLTitleElement returns null for lang in text/html

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: HARUNAGA Hirotoshi, Assigned: mrbkap)

Tracking

({intl})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

14 years ago
DOM HTMLTitleElement returns null for lang in text/html.

<meta> and <link> have no problem.

and it works if it is served as application/xhtml+xml.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2565&action=view
(Reporter)

Comment 1

14 years ago
this might be a dup of Bug 118704.
Component: DOM → DOM: HTML
So the problem is that the nsIHTMLContentSink api has this SetTitle method that
just takes a string.  So we lose all the attributes on <title> tags in HTML.

That seems a tad wrong... we should probably pass in the parser node here and
add attributes, etc, no?
(Assignee)

Comment 3

14 years ago
That sounds about right. Note that when we do that, all of the serializers will
also need to be updated.
(Assignee)

Comment 4

14 years ago
Taking. I plan to fix this a bit later this week.
Assignee: general → mrbkap
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Keywords: helpwanted
(Assignee)

Comment 5

14 years ago
Created attachment 174722 [details] [diff] [review]
patch v1

Just as a note: I think that I got all of the files in this patch. If you see
any omissions point them out.
Attachment #174722 - Flags: review?(bugmail)
I'm a bit confused by this patch? So we used to call both SetTitle and
AddHeadContent? The change to nsHTMLContentSink seems to indicate so, but the
code in CNavDTD looks like we only used to call SetTitle. Did the
nsHTMLContentSink have code to handle something that never really happened?

In any case, why keep the nsIHTMLContentSink::SetTitle method (with
implementation in various sinks)? Couldn't you just remove it?
(Assignee)

Comment 7

14 years ago
I'm not sure why there was code to handle title in
nsHTMLContentSink::AddHeadContent(). The content sinks having dead code wouldn't
be a terrible surprise to me. I kept both SetTitle() and use AddHeadContent()
for the convenience of DTDs that don't use skipped content (e.g.,
CViewSourceHTML). Once I get around to nuking skipped content entirely, I'll
remove SetTitle().
Comment on attachment 174722 [details] [diff] [review]
patch v1

I'm not super exited about this patch since we're left in kind of a mess where
titles can get into the document two ways. Though it does take us closer to the
state where we want to be, i.e. where titles from the sink looks like just any
other tag. So as long as SetTitle does along with skipped-content i'm happy :)

>Index: parser/htmlparser/src/CNavDTD.cpp
...
>   if (aTag <= NS_HTML_TAG_MAX) {
>     result = mSink->NotifyTagObservers(&aNode);
>+
>   }

You had this removed already right?

>Index: parser/htmlparser/src/nsLoggingSink.cpp
> nsLoggingSink::AddHeadContent(const nsIParserNode& aNode) {
>-  LeafNode(aNode);
>+  eHTMLTags type = (eHTMLTags)aNode.GetNodeType();
>+
>+  if (0 != strchr(gSkippedContentTags, type)) {
>+    NS_ASSERTION(eHTMLTag_title == type, "There shouldn't be any other "
>+                                         "skipped content head nodes.");

Just do |if (type == eHTMLTag_title)| instead, this isn't usefull for anything
but titles anyway.

Hmm... or do you want to avoid the assertion for skipped-content-not-collected?
Feel free to leave as is then. Though do change it to ASSERTION(type ==
eHTMLTag_title..., i'm allergic to that type of comparisons :)

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp
>   nsresult AddText(const nsAString& aString);
>-  nsresult AddTextToContent(nsIContent* aContent,const nsString& aText);
>+  nsresult AddTextToContent(nsIContent* aContent,const nsAString& aText);
>   nsresult FlushText();

Do you have to do this to get it to compile? The old signature is actually
faster.


>+nsresult
>+nsHTMLFragmentContentSink::SetDocumentTitle(const nsAString& aString, const nsIParserNode* aNode)
>+{
>+  NS_ENSURE_TRUE(mNodeInfoManager, NS_ERROR_NOT_INITIALIZED);
>+
>+  nsresult result=NS_OK;

Call this 'rv'

>+
>+  nsCOMPtr<nsINodeInfo> nodeInfo;
>+  result = mNodeInfoManager->GetNodeInfo(nsHTMLAtoms::title, nsnull,
>+                                         kNameSpaceID_None,
>+                                         getter_AddRefs(nodeInfo));
>+  if(NS_SUCCEEDED(result)) {

Use NS_ENSURE_SUCCESS

>+    nsRefPtr<nsGenericHTMLElement> content = NS_NewHTMLTitleElement(nodeInfo);
>+
>+    if (!content) {
>+      result = NS_ERROR_OUT_OF_MEMORY;
>+    } else {

Use an early-return instead. NS_ENSURE_TRUE is nice.

>+      nsIContent *parent = GetCurrentContent();
>+
>+      if (aNode) {
>+        AddAttributes(*aNode, content);
>+      }
>+
>+      if (nsnull == parent) {

if (!parent)

>+        parent = mRoot;
>+      }
>+
>+      result=parent->AppendChildTo(content, PR_FALSE, PR_FALSE);

Whitespace around =

And use NS_ENSURE_SUCCESS

>+
>+      if (NS_SUCCEEDED(result)) {
>+        result=AddTextToContent(content,aString);

Same here.

>+      }
>+    }
>+  }
>+
>+  return result;

Which'll probably leave you with an return NS_OK here.

r=me with that.
Attachment #174722 - Flags: review?(bugmail) → review+
(Assignee)

Comment 9

14 years ago
(In reply to comment #8)
> (From update of attachment 174722 [details] [diff] [review] [edit])
> You had this removed already right?

Yeah.

> 
> Just do |if (type == eHTMLTag_title)| instead, this isn't usefull for anything
> but titles anyway.
> 

I'll do this, no more container tags come through AddHeadContent().

> Do you have to do this to get it to compile? The old signature is actually
> faster.
> 

Unfortunately, yes, because we pass an nsAString to SetTitle(), which calls this
function (indirectly).

> 
> >+nsresult
> >+nsHTMLFragmentContentSink::SetDocumentTitle(const nsAString& aString, const
nsIParserNode* aNode)

This is what I get for copying and pasting fragment sink code :-(. I'll make a
new patch with these changes.
(Assignee)

Comment 10

14 years ago
Created attachment 177101 [details] [diff] [review]
patch v2

This is updated to the comments. Carrying over the r+ and looking for sr=.
Attachment #174722 - Attachment is obsolete: true
Attachment #177101 - Flags: superreview?(peterv)
Attachment #177101 - Flags: review+
Comment on attachment 177101 [details] [diff] [review]
patch v2

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp
>===================================================================

>+nsresult
>+nsHTMLFragmentContentSink::SetDocumentTitle(const nsAString& aString, const nsIParserNode* aNode)
>+{
>+  NS_ENSURE_TRUE(mNodeInfoManager, NS_ERROR_NOT_INITIALIZED);
>+
>+  nsresult rv = NS_OK;
>+
>+  nsCOMPtr<nsINodeInfo> nodeInfo;
>+  rv = mNodeInfoManager->GetNodeInfo(nsHTMLAtoms::title, nsnull,
>+                                         kNameSpaceID_None,
>+                                         getter_AddRefs(nodeInfo));

Line this up correctly. Also, declare rv here instead of earlier.

>+  nsIContent *parent = GetCurrentContent();
>+
>+  if (aNode) {
>+    AddAttributes(*aNode, content);
>+  }

Flip the above two blocks, so that the next block is just after the declaration
of parent.

>+
>+  if (!parent) {
>+    parent = mRoot;
>+  }

>+  rv = AddTextToContent(content,aString);
>+
>+  return rv;

Just |return AddTextToContent(...)| and add a space after the comma.

> NS_IMETHODIMP
> nsHTMLFragmentContentSink::AddLeaf(const nsIParserNode& aNode)
> {
>+  if (eHTMLTag_title == aNode.GetNodeType()) {
>+    nsCOMPtr<nsIDTD> dtd;
>+    mParser->GetDTD(getter_AddRefs(dtd));
>+    NS_ENSURE_TRUE(dtd, NS_ERROR_FAILURE);
>+
>+    nsAutoString skippedContent;
>+    PRInt32 lineNo = 0;
>+
>+    dtd->CollectSkippedContent(eHTMLTag_title, skippedContent, lineNo);
>+
>+    return SetDocumentTitle(skippedContent, &aNode);
>+  }

Add a newline here?

>   NS_ENSURE_TRUE(mNodeInfoManager, NS_ERROR_NOT_INITIALIZED);

> nsresult
>-nsHTMLFragmentContentSink::AddTextToContent(nsIContent* aContent,const nsString& aText) {
>+nsHTMLFragmentContentSink::AddTextToContent(nsIContent* aContent,const nsAString& aText) {

Space after comma.

>Index: content/base/src/mozSanitizingSerializer.cpp
>===================================================================

>+  else if (eHTMLTag_title == type) {
>+    NS_ASSERTION(mParser, "Only CNavDTD treates title this way.");

treats
Attachment #177101 - Flags: superreview?(peterv) → superreview+
(Assignee)

Comment 12

14 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.