Closed Bug 1178484 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Iteration:
42.3 - Aug 10
Tracking Status
firefox43 --- fixed

People

(Reporter: benfrancis, Assigned: tedders1)

References

Details

(Keywords: feature)

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 files, 5 obsolete files)

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: nobody → tclancy
Attached patch WIP (obsolete) — Splinter Review
Attached 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.
Attachment #8647021 - Attachment is obsolete: true
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)
Attachment #8649320 - Attachment description: bug-1178484-part-1.patch → Bug 1178484 - Part 1: Add attribute 'property' to parser.
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)
This code fires a metachange event whenever a <meta> tag has a |name| or |property| attribute that starts with |og:| .
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?
(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)
Attached patch bug-1178484-part-4.patch (obsolete) — Splinter Review
Attachment #8649324 - Attachment is obsolete: true
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-
Attachment #8649323 - Attachment is obsolete: true
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+
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
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.