Closed Bug 782623 Opened 12 years ago Closed 9 years ago

Name field in Meta tags often empty

Categories

(Firefox :: Page Info Window, defect)

Product:
Firefox
Component:
Page Info Window
Version:
14 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 39
Tracking Flags:
Tracking Status
firefox39 --- verified

People

(Reporter: oliver.henshaw, Assigned: mail, Mentored)

Details

(Whiteboard: [lang=js][bugday-20150701])

Attachments

(2 files)

Make pageInfo display the 'property' attribute of meta tags.
1.17 KB, patch
jaws
: review+
Details | Diff | Splinter Review
bug782623_fixMetaTagPropertyAttribute_withLinebreak.diff
1.23 KB, patch
jaws
: review+
Details | Diff | Splinter Review 
For guardian articles, e.g. http://www.guardian.co.uk/environment/2012/aug/14/eu-waste the Meta tags list in "Page Info" contains many entries with blank Name field but non-blank Content fields.

Looking at the page source, some likely candidates for the problematic entries are "link rel=canonical", "meta name=description", "link rel=image_src".


Tested on Fedora 17 Live image + updated firefox:

firefox-14.0.1-1.fc17.x86_64
xulrunner-14.0.1-3.fc17.x86_64
(In reply to Oliver Henshaw from comment #0)
> For guardian articles, e.g.
> http://www.guardian.co.uk/environment/2012/aug/14/eu-waste the Meta tags
> list in "Page Info" contains many entries with blank Name field but
> non-blank Content fields.

The Guardian webpage claims to be HTML5, but Open Graph uses <meta property="...". In HTML5 the meta tag does not have a property attribute. Open Graph is RDFa so you probably need a different DOCTYPE e.g.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN" "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-1.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xmlns:v="http://rdf.data-vocabulary.org/#">
Mentor: jaws
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=js]
Assignee: nobody → mail
Status: NEW → ASSIGNED
For this bug, the change will need to be made at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.js?rev=8d3304b7e0e0#514.

We currently supply `metaNodes[i].name || metaNodes[i].httpEquiv`. We can update this to also include the `property` attribute: `metaNodes[i].name || metaNodes[i].httpEquiv || metaNodes[i].getAttribute("property")`.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> For this bug, the change will need to be made at
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/
> pageInfo.js?rev=8d3304b7e0e0#514.
> 
> We currently supply `metaNodes[i].name || metaNodes[i].httpEquiv`. We can
> update this to also include the `property` attribute: `metaNodes[i].name ||
> metaNodes[i].httpEquiv || metaNodes[i].getAttribute("property")`.

This is rather hacky. I think instead that you should just add a "property" attribute to nsIDOMHTMLMetaElement.idl so that you can reference it as metaNodes[i].property
Ms2ger, are you OK with doing what comment #3 is recommending?
Flags: needinfo?(Ms2ger)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)

Or if bz is quicker to respond.
Flags: needinfo?(bzbarsky)
No, that's nonsense. nsIDOM* interfaces don't expose anything to JS, and haven't for years. getAttribute() is perfectly fine if that's something we want to do. Not sure if we need to expose those, though. What is the use case for this list?
Flags: needinfo?(Ms2ger)
Ms2ger is right.  nsIDOMHTMLMetaElement is irrelevant to JS.  Adding things to HTMLMetaElement.webidl _could_ be done, but I don't see a good reason to add this there, esp. since it would have to be [ChromeOnly] or something (unless you're proposing this be added to the HTML spec).
Flags: needinfo?(bzbarsky)
(In reply to :Ms2ger from comment #6)
> No, that's nonsense. 

Ok, let's proceed with the solution from comment #2.

> What is the use case for this list?

I'm not sure I understand what you're asking. Are you asking what's the point of this bug? I thought I added some more background in comment #2 but I guess I forgot to add it there.

There are many pages that use name="property" that currently display a blank column in the Page Info dialog. It would be nice if we added support for those pages, and it doesn't come at much of a cost to us if we only have to update pageInfo.js to do so.
I implemented the solution described in comment #2.
Attachment #8567565 - Flags: review?(jaws)
Comment on attachment 8567565 [details] [diff] [review]
Make pageInfo display the 'property' attribute of meta tags.

Review of attachment 8567565 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/pageinfo/pageInfo.js
@@ +512,5 @@
>      var metaTree = document.getElementById("metatree");
>      metaTree.view = gMetaView;
>  
>      for (var i = 0; i < length; i++)
> +      gMetaView.addRow([metaNodes[i].name || metaNodes[i].httpEquiv || metaNodes[i].getAttribute("property"), metaNodes[i].content]);

This looks good. We'll want to wrap the line after the comma since it's getting pretty long.

Do you want to post an updated patch and add r=jaws to the end of the patch summary?
Attachment #8567565 - Flags: review?(jaws) → review+
Flags: needinfo?(mail)
Thanks for the feedback. 
I implemented the solution described in comment #2 and wrapped the line accordingly. I wasn't entirely sure about the code style conventions (I looked here (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style), especially regarding the indentation of the wrapped line and the question of whether or not braces should be added. Hopefully I did it correctly.
Flags: needinfo?(mail)
Attachment #8570430 - Flags: review?(jaws)
Comment on attachment 8570430 [details] [diff] [review]
bug782623_fixMetaTagPropertyAttribute_withLinebreak.diff

Review of attachment 8570430 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! I'll push this to tryserver where it will run the changes against the set of automated tests that this might affect. If all goes well (which I expect it to), we can land this on fx-team where it will then get merged to mozilla-central and appear in Firefox Nightly a day or two later.
Attachment #8570430 - Flags: review?(jaws) → review+
Attachment #8567565 - Attachment is obsolete: true
Attachment #8567565 - Attachment is obsolete: false
I pushed your changes to the fx-team repository. It should get merged to mozilla-central in about a day and then the following day the changes will appear in Firefox Nightly. Nice job! I'm looking forward to more of your work in bug 736572.

https://hg.mozilla.org/integration/fx-team/rev/75998342ca12
https://hg.mozilla.org/mozilla-central/rev/75998342ca12
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Reproduced with Firefox 14.0.1 (2012-07-13) with the instruction from comment 0 and on Windows 7 x64.

Verified as fixed with Firefox   39.0  (Build ID : 20150618135210)

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:39.0) Gecko/20100101 Firefox/39.0

Suggesting -> VERIFIED FIXED 

[Bugday-20150701]
Thanks!
Status: RESOLVED → VERIFIED
Reproduced this bug in Firefox (2012-17-13) with the instruction from comment 0 and on Linux x64

Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0.1

The bug is fixed on Latest beta 39.0

Build ID: 20150630154324
Mozilla/5.0 (X11; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
QA Whiteboard: [bugday-20150701]
Whiteboard: [lang=js] → [lang=js][bugday-20150701]
As this bug is now fixed in Linux and Windows as per comment 16 and Comment 18, changing the status flag as Verified!
status-firefox39: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: