[Browser API] Add support for Open Graph meta tags to metachange event

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
8 days ago

People

(Reporter: benfrancis, Assigned: tedders1)

Tracking

({feature})

unspecified
mozilla43
feature
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

User Story

As a Gaia developer I would like to listen for events which tell me when an Open Graph meta tag is detected in the DOM of an embedded frame so that I can build rich "cards" in the UI to represent key metadata from web pages.

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
As a Gaia developer I would like to listen for events which tell me when an Open Graph meta tag is detected in the DOM of an embedded frame so that I can build rich "cards" in the UI to represent key metadata from web pages.

I suggest firing events for all meta tags with a "property" attribute, which is only used by Open Graph. The payload of the event should include both the property name and the content.
(Assignee)

Updated

4 years ago
Assignee: nobody → tclancy
(Assignee)

Comment 1

4 years ago
Posted patch WIP (obsolete) — Splinter Review
(Assignee)

Comment 2

4 years ago
Posted patch bug-1178484-fix.patch (obsolete) — Splinter Review
Attachment #8646373 - Attachment is obsolete: true
Just to add my two cents:

We should support both the property="og:xxx" and the name="og:xxx" attributes since some sites use one or the other.
(Assignee)

Updated

4 years ago
Attachment #8647021 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
This patch is for the repo at https://hg.mozilla.org/projects/htmlparser, not the Gecko repo.

Or, do I need to make a separate bugzilla ticket for that?
Attachment #8649320 - Flags: review?(hsivonen)
(Assignee)

Updated

4 years ago
Attachment #8649320 - Attachment description: bug-1178484-part-1.patch → Bug 1178484 - Part 1: Add attribute 'property' to parser.
(Assignee)

Comment 6

4 years ago
Due to a name collision, I refer to this attribute as |OGProperty| in the C++ source code (instead of just |Property|).
Attachment #8649323 - Flags: review?(hsivonen)
(Assignee)

Comment 8

4 years ago
This code fires a metachange event whenever a <meta> tag has a |name| or |property| attribute that starts with |og:| .
(Assignee)

Updated

4 years ago
Iteration: --- → 42.3 - Aug 10
Comment on attachment 8649324 [details] [diff] [review]
Bug 1178484 - Part 4: Fire metachange event when a <meta> tag has Open Graph data.

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

LGTM. Test needed. You could reuse dom/browser-element/mochitest/browserElement_Metachange.js

::: dom/browser-element/BrowserElementChildPreload.js
@@ +588,5 @@
>  
> +    if ((property || name).match(/^og:/)) {
> +      name = property || name;
> +      handler = _genericMetaHandler;
> +    }

this._genericMetaHandler
Attachment #8649324 - Flags: review?(kchen) → review+
Comment on attachment 8649323 [details] [diff] [review]
Bug 1178484 - Part 3: Add support for OpenGraph's property attribute to DOM classes.

>-[uuid(2a3f789e-0667-464f-a8d7-6f58513443d9)]
>+[uuid(8ab47e1b-ab48-4669-afb6-b25f6fff13e2)]
> interface nsIDOMHTMLMetaElement : nsISupports
> {
>            attribute DOMString        content;
>            attribute DOMString        httpEquiv;
>            attribute DOMString        name;
>            attribute DOMString        scheme;
>+
>+           [binaryname(OGProperty)]
>+           attribute DOMString        property;  // For OpenGraph
Why this? We try to not add new stuff to nsIDOM*.idl


>+
>+  // This is not in the WHATWG spec, but is needed for Open Graph support
>+  [SetterThrows, Pure, BinaryName=OGProperty]
>+           attribute DOMString property;
Why element.getAttribute("property"); isn't enough?
(Reporter)

Comment 11

4 years ago
(In reply to Michael Henretty [:mhenretty] from comment #3)
> We should support both the property="og:xxx" and the name="og:xxx"
> attributes since some sites use one or the other.

Can you point towards some examples of websites that do this so we can try to figure out why? I know that Twitter uses the name attribute for "twitter:" prefixed meta tags, but they support RDFa-style meta tags with property="og:*" for the Open Graph vocabularly. I wonder why people are doing name="og:*" ?
Flags: needinfo?(mhenretty)
IIRC, I believe a big video site did it, but now it seems like they all use the property correctly. Pinterest does both name and property for some reason. Maybe I was mistaken, or whoever it was fixed it.
Flags: needinfo?(mhenretty)
(Assignee)

Comment 13

4 years ago
Posted patch bug-1178484-part-4.patch (obsolete) — Splinter Review
Attachment #8649324 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Attachment #8651734 - Flags: review?(kchen)
Comment on attachment 8649320 [details] [diff] [review]
Bug 1178484 - Part 1: Add attribute 'property' to parser.

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

It's sad that Facebook's success at getting Web authors to provide metadata this way makes it looks like we are supporting RDFa even though neither Facebook nor we really are. If the topic comes up at the W3C or similar, we should be clear that we don't have RDFa support but support for scraping existing OGP data without processing it as RDFa.
Attachment #8649320 - Flags: review?(hsivonen) → review+
Attachment #8649322 - Flags: review?(hsivonen) → review+
Comment on attachment 8649323 [details] [diff] [review]
Bug 1178484 - Part 3: Add support for OpenGraph's property attribute to DOM classes.

(In reply to Olli Pettay [:smaug] (high review load) from comment #10)
> >+  // This is not in the WHATWG spec, but is needed for Open Graph support
> >+  [SetterThrows, Pure, BinaryName=OGProperty]
> >+           attribute DOMString property;
> Why element.getAttribute("property"); isn't enough?

Reiterating what smaug said.

We shouldn't add Web-exposed API surface like this without it being in the spec. Please use element.getAttributeNS(null, "property") until/unless a convenience property is added to the WHATWG spec. (Since we aren't actually implementing RDFa, it doesn't count even if there exists some RDFa spec somewhere claiming the existence for this DOM property.)
Attachment #8649323 - Flags: review?(hsivonen) → review-
(Assignee)

Updated

4 years ago
Attachment #8649323 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Thanks for your review, Henri. 

You're right, it's probably not a good idea to put the "property" attribute in the IDL interface. We don't want to support RDFa any further than what is necessary for Open Graph, and we also don't want to give the impression that we're supporting RDFa.
Attachment #8651596 - Attachment is obsolete: true
Attachment #8651734 - Flags: review?(kchen) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76bf75baf504
https://hg.mozilla.org/mozilla-central/rev/904469674599
https://hg.mozilla.org/mozilla-central/rev/d475df7f9c82
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.