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

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

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.

Comment 1

[Ted Clancy [:tedders1]]

Attachment: WIP

Comment 2

[Ted Clancy [:tedders1]]

Attachment: bug-1178484-fix.patch

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

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.

Comment 4

[Ted Clancy [:tedders1]]

Attachment: Bug 1178484 - Part 1: Add attribute 'property' to parser.

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?

Comment 5

[Ted Clancy [:tedders1]]

Attachment: Bug 1178484 - Part 2: Regenerate gecko's in-tree parser.

Comment 6

[Ted Clancy [:tedders1]]

Attachment: Bug 1178484 - Part 3: Add support for OpenGraph's property attribute to DOM classes.

Due to a name collision, I refer to this attribute as |OGProperty| in the C++ source code (instead of just |Property|).

Comment 7

[Ted Clancy [:tedders1]]

Attachment: Bug 1178484 - Part 4: Fire metachange event when a <meta> tag has Open Graph data.

Comment 8

[Ted Clancy [:tedders1]]

This code fires a metachange event whenever a <meta> tag has a |name| or |property| attribute that starts with |og:| .

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 11

[Ben Francis [:benfrancis]]

(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:*" ?

Comment 12

[Michael Henretty [:mikehenrty][: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.

Comment 13

[Ted Clancy [:tedders1]]

Attachment: bug-1178484-part-4.patch

Comment 14

[Ted Clancy [:tedders1]]

Attachment: bug-1178484-test.patch

Comment 15

[Henri Sivonen (:hsivonen)]

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.

Comment 16

[Henri Sivonen (:hsivonen)]

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.)

Comment 17

[Ted Clancy [:tedders1]]

Attachment: Bug 1178484 - Part 4: Fire metachange event when a <meta> tag has Open Graph data.

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.

Comment 18

[Ted Clancy [:tedders1]]

Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e98964e4c7f

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76bf75baf504
https://hg.mozilla.org/integration/mozilla-inbound/rev/904469674599
https://hg.mozilla.org/integration/mozilla-inbound/rev/d475df7f9c82

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76bf75baf504
https://hg.mozilla.org/mozilla-central/rev/904469674599
https://hg.mozilla.org/mozilla-central/rev/d475df7f9c82

Comment 21

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/projects/htmlparser/rev/fe5060de0e6c