Last modified: 2014-01-15 23:30:40 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T59394, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 57394 - Parsoid fails for OBJECT element
Parsoid fails for OBJECT element
Status: RESOLVED FIXED
Product: Parsoid
Classification: Unclassified
General (Other open bugs)
unspecified
All All
: Normal normal
: ---
Assigned To: Arlo Breault
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-22 02:24 UTC by Inez Korczyński
Modified: 2014-01-15 23:30 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Inez Korczyński 2013-11-22 02:24:38 UTC
For instance if some Parser tag extension returns OBJECT tag as a root element it will make Parsoid fail.
This is because Parsoid depends on ability to set and read property 'data' of elements, but for OBJECT it is not possible, because OBJECT always has data set, and it is string.

More info: http://www.w3schools.com/tags/att_object_data.asp

Also:
var element = document.createElement('object');
typeof element.data; // string
element.data = {a:1}
element.data.a // undefined
Comment 1 Gabriel Wicke 2013-11-22 07:41:49 UTC
This seems to be a bug in domino. It should not confuse the data attribute with the .data node object member.
Comment 2 Inez Korczyński 2013-11-22 16:17:05 UTC
(In reply to comment #1)
> This seems to be a bug in domino. It should not confuse the data attribute
> with
> the .data node object member.

I don't think it's a domino (or domino only) problem, try in a browser:
element = document.createElement('object');
element.setAttribute('data','123');
console.log(element.data);
Comment 3 Gabriel Wicke 2013-11-25 19:46:12 UTC
This is indeed correct for object elements according to http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-object-element
Comment 4 Inez Korczyński 2013-11-25 19:55:11 UTC
(In reply to comment #3)
> This is indeed correct for object elements according to
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-
> element.html#the-object-element

Gabriel: Do you have a suggestion what the proper fix should look like? Use in Parsoid property different than "data"? (That would be a massive search and replace.)
Comment 5 Gabriel Wicke 2013-11-25 21:30:59 UTC
I see two options here:

1) Rename data to some other non-conflicting 'expando' property. 

Example: .dataobject to rhyme with .dataset

This is used by jquery et al to store a string key for $.data.

2) Move all data out of the elements themselves and store them in an external data structure keyed on the id of the element. Jquery does this too, but uses an 'expando' property as in 1) instead of the id.
Comment 6 Gabriel Wicke 2013-11-26 17:53:23 UTC
/cc Arlo as he is working on external metadata and stable ids.
Comment 7 C. Scott Ananian 2013-12-04 20:02:38 UTC
I suggest we first do a refactor to use uniformly use:
  DU.data(node)
for getters, and maybe
  DU.data(node, new_data)
for setters?

Then we can easily experiment with/bikeshed different implementations of DU.data() under the hood -- first probably using a new 'parsoid-data' or 'dataobject' attribute, then later benchmarking against using an external map keyed on id, or an es6 weak map, or an es6 private field, etc.
Comment 8 Inez Korczyński 2013-12-04 21:13:23 UTC
(In reply to comment #7)
> I suggest we first do a refactor to use uniformly use:
>   DU.data(node)
> for getters, and maybe
>   DU.data(node, new_data)
> for setters?
> 
> Then we can easily experiment with/bikeshed different implementations of
> DU.data() under the hood -- first probably using a new 'parsoid-data' or
> 'dataobject' attribute, then later benchmarking against using an external map
> keyed on id, or an es6 weak map, or an es6 private field, etc.

This plan sounds good to me. I could work on that. Gabriel: Could you confirm that you are ok with that solution?
Comment 9 Gabriel Wicke 2013-12-06 20:31:19 UTC
I'm not a fan of giving DU internal state, so IMO it might make more sense to use either

DU.data(node, env)

or env.getNodeData(node)

I'd suggest both should return a reference to the actual object, which can be modified as needed. To completely replace the data entry, env.setNodeData(node, newData) could be used. Using separate getter/setters lets us add default values too.
Comment 10 Gabriel Wicke 2013-12-11 21:21:43 UTC
We discussed this more on IRC and figured that we could avoid giving DU internal state by attaching extra info to the document object in the longer term and to each node in the shorter term. That would leave us with DU.data(node) I guess.
Comment 11 Gerrit Notification Bot 2014-01-08 21:27:42 UTC
Change 106429 had a related patch set uploaded by Arlolra:
WIP: Move .data off DOM nodes

https://gerrit.wikimedia.org/r/106429
Comment 12 Gerrit Notification Bot 2014-01-15 23:29:01 UTC
Change 106429 merged by jenkins-bot:
Move .data off DOM nodes

https://gerrit.wikimedia.org/r/106429

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links