Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement microdata #18528

Open
asajeffrey opened this issue Sep 15, 2017 · 21 comments
Open

Implement microdata #18528

asajeffrey opened this issue Sep 15, 2017 · 21 comments
Labels
A-content/dom Interacting with the DOM from web content

Comments

@asajeffrey
Copy link
Member

WHATWG HTML spec for microdata: https://html.spec.whatwg.org/multipage/microdata.html

@asajeffrey asajeffrey added the A-content/script Related to the script thread label Sep 15, 2017
@asajeffrey
Copy link
Member Author

cc @jdm

@nox nox added A-content/dom Interacting with the DOM from web content and removed A-content/script Related to the script thread labels Sep 18, 2017
@niravjain
Copy link

Posting here since IRC seems to be down. We're facing issues with html5ever, it gives error for xml5ever. However, we didn't make any changes to xml5ever. Can you guide what we can do move forward?

Pastebin: https://pastebin.com/pXfK5Fnx

@jdm
Copy link
Member

jdm commented Oct 27, 2017

What happens if you run ./mach cargo-update -p markup5ever?

@niravjain
Copy link

Thanks @jdm , servo build now uses xml5ever version 0.6.2 which no longer causes the issue we were facing.

@niravjain
Copy link

niravjain commented Oct 27, 2017

[Resolved]
pref.json had syntax error


Right now we have created test cases, but not able to run using test-wpt. While creating the tests using create-wpt, we're getting the following error:
https://pastebin.com/d8i99igf

Any input on why we are getting this issue?

@CJ8664
Copy link

CJ8664 commented Oct 28, 2017

All the issues have been resolved and the pull request is ready to be merged with only one problem, we are having difficulties in rebasing the code :(
The pull request is below
#19038

@CJ8664
Copy link

CJ8664 commented Nov 22, 2017

We are trying to compile servoshell with a local copy of servo(upto date with servo repository) and are getting this error unresolved import self::servo::msg::constellation_msg::SHIFT

I am thinking that current code in servoshell is not compatible with the changes that exist in latest servo code or is this some know dependency issue that can be fixed

@jdm
Copy link
Member

jdm commented Nov 22, 2017

That should be fixable by changing the import to self::servo::msg::constellation_msg::KeyModifiers, and then use KeyModifiers::SHIFT in the code that uses that value.

@CJ8664
Copy link

CJ8664 commented Nov 22, 2017

Ok one more dependency issue
missing screen_size, screen_avail_size in implementation

--> impl WindowMethods for ServoCallbacks

@jdm
Copy link
Member

jdm commented Nov 22, 2017

You should be able to return a Size2D value based off of the values in self.geometry.get().view_size for both of those.

@niravjain
Copy link

Thanks @jdm, that resolved the issue.

Now, for the next part, we are passing a hardcoded string from servo to servoshell which works, so we have identified the files where changes are required to establish the communication between servo and servoshell.

Now, as per our previous discussion, we are trying to serialize the tree dom as json in order to send it, using serde_json. However, we are facing issues while trying to use serde_json as extern crate.

We have pushed our changes in this branch: https://github.com/CJ8664/servo/tree/serde_try
Following is the error we're facing: https://pastebin.com/gFMX1fJp

Looks like there's some namespace clashing, as the error is on some previously working code.
Can you please take a look and suggest what the resolution might be?

@jdm
Copy link
Member

jdm commented Nov 25, 2017

That's super weird! I've filed an issue against the Rust compiler for that; you can work around the problem by replacing Default::default() with 0.

@CJ8664
Copy link

CJ8664 commented Nov 25, 2017

Thanks, @jdm, the code is now creating a JSON of nested Hashmap as of now by replacing Default::default() with 0.

@bluss
Copy link

bluss commented Nov 25, 2017

Style wise I'd always prefer T::default() when possible in favour of the very vague Default::default(). In this case that would be u64::default() and yes, for a numeric type, 0 is then even better :-).

@CJ8664
Copy link

CJ8664 commented Nov 26, 2017

We are trying to create the JSON for a Node of the DomObject by serializing it. For the same, we are trying to use serde_json.
However, the serialization fails with the following error, the trait serde::Serialize is not implemented for dom::node::Node.

We also found that there is a serializer implemented for Node in script/dom/html.rs but we are not able to use in out our module.

Can you please help us.

@niravjain
Copy link

niravjain commented Nov 26, 2017

Just to add to @CJ8664 's point, if we implement the trait for dom::node::Node by adding [derive(Serialize)]above the definition of struct Node, then we get the same error for all other structs defined in Node. Here's the pastebin for that: https://pastebin.com/79JYwJ3w

Are we expected to implement the serialization, because we found this issue (#16696) that seems like the logic for serialization would already be there. Can you help us figure out how to use the serialization logic?

@jdm
Copy link
Member

jdm commented Nov 26, 2017

The specification describes exactly what serialized output is expected; you need to implement that - an object containing an "items" property which is an array, where each is an object that matches this algorithm.

@jdm
Copy link
Member

jdm commented Nov 26, 2017

serde_json just provides building blocks for creating the object/array values, and serializing those values into a valid JSON string once it is complete.

@niravjain
Copy link

niravjain commented Nov 30, 2017

We're on it @jdm . Meanwhile, for vCard we have a small doubt. What will be the expected vCard output for the following?
HTML:
< h1 itemprop="fn" >
< span itemprop="n" itemscope >
< span itemprop="given-name" >Jack< /span >
< span itemprop="family-name" >Bauer< /span >
< /span >
< /h1 >

As per our understanding of the vCard algorithm on WHATWG HTML spec, it should be:
BEGIN:VCARD
PROFILE:VCARD
VERSION:4.0
SOURCE:file:///Users/nirav/workspaces/servo/sample.html
N:Bauer;Jack
END:VCARD

But as per some other online vCard examples, this seems to be the constructed vCard:
BEGIN:VCARD
PROFILE:VCARD
VERSION:4.0
SOURCE:file:///Users/nirav/workspaces/servo/sample.html
N:Bauer;Jack
FN:Jack Bauer
END:VCARD

@jdm
Copy link
Member

jdm commented Nov 30, 2017

I would stick with the algorithm in the specification for now.

@jdm
Copy link
Member

jdm commented Nov 30, 2017

Actually, the specification appears to agree with the online examples: for the h1 element with the fn property, the property's value is derived from the "otherwise" branch of this step. That should end up with "Jack Bauer", if I understand correctly.

@CJ8664 CJ8664 mentioned this issue Dec 12, 2017
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/dom Interacting with the DOM from web content
Projects
None yet
Development

No branches or pull requests

6 participants