Closed
Bug 1491758
Opened 6 years ago
Closed 6 years ago
[de-xbl] Convert facet-date to custom element.
Categories
(Thunderbird :: General, task)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 64.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Comment on attachment 9015249 [details] [diff] [review] facet-date.patch Review of attachment 9015249 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/glodaFacet.js @@ +4,5 @@ > + > +/* global MozXULElement, DateFacetVis, FacetContext */ > + > +class MozFacetDate extends MozXULElement { > + get a() { I don't know why I am not able to get a/build/brushItems/clearBrushedItems from the cuswtom element node.. any idea why?
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9015260 -
Attachment is obsolete: true
Attachment #9015274 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9015274 -
Attachment is obsolete: true
Attachment #9015274 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 6•6 years ago
|
||
Comment on attachment 9015276 [details] [diff] [review] facet-date_3.patch Review of attachment 9015276 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/glodaFacet.js @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* global HTMLElement, DateFacetVis, FacetContext */ > + > +class MozFacetDate extends HTMLElement { Using HTMLElement instead of MozXulElement because I think it makes sense to use that for custom element in xhtml file.. ALso if mozxulelment is used, you can't get `style` object using node.style. @@ +8,5 @@ > + get build() { > + return this.buildFunc; > + } > + > + get brushItems() { all the getters are function that are used by glodaFacetView.js
Assignee | ||
Updated•6 years ago
|
Attachment #9015276 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9015276 -
Flags: feedback?(bgrinstead)
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Arshad Khan [:arshad] from comment #2) > Comment on attachment 9015249 [details] [diff] [review] > facet-date.patch > > Review of attachment 9015249 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/base/content/glodaFacet.js > @@ +4,5 @@ > > + > > +/* global MozXULElement, DateFacetVis, FacetContext */ > > + > > +class MozFacetDate extends MozXULElement { > > + get a() { > > I don't know why I am not able to get a/build/brushItems/clearBrushedItems > from the cuswtom element node.. any idea why? the reason was the position of scripts in glodaView..xhtml. I have put all the scripts at the bottom of page, so that custom element is created first then the code of glodaFacetView.js is run, else it will throw error.
Comment 8•6 years ago
|
||
(In reply to Arshad Khan [:arshad] from comment #7) > (In reply to Arshad Khan [:arshad] from comment #2) > > Comment on attachment 9015249 [details] [diff] [review] > > facet-date.patch > > > > Review of attachment 9015249 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: mail/base/content/glodaFacet.js > > @@ +4,5 @@ > > > + > > > +/* global MozXULElement, DateFacetVis, FacetContext */ > > > + > > > +class MozFacetDate extends MozXULElement { > > > + get a() { > > > > I don't know why I am not able to get a/build/brushItems/clearBrushedItems > > from the cuswtom element node.. any idea why? > > the reason was the position of scripts in glodaView..xhtml. I have put all > the scripts at the bottom of page, so that custom element is created first > then the code of glodaFacetView.js is run, else it will throw error. Could that same script order be maintained but at the top of the file? Or is there some issue with having the CE defined and upgraded during parse?
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #8) > (In reply to Arshad Khan [:arshad] from comment #7) > > (In reply to Arshad Khan [:arshad] from comment #2) > > > Comment on attachment 9015249 [details] [diff] [review] > > > facet-date.patch > > > > > > Review of attachment 9015249 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: mail/base/content/glodaFacet.js > > > @@ +4,5 @@ > > > > + > > > > +/* global MozXULElement, DateFacetVis, FacetContext */ > > > > + > > > > +class MozFacetDate extends MozXULElement { > > > > + get a() { > > > > > > I don't know why I am not able to get a/build/brushItems/clearBrushedItems > > > from the cuswtom element node.. any idea why? > > > > the reason was the position of scripts in glodaView..xhtml. I have put all > > the scripts at the bottom of page, so that custom element is created first > > then the code of glodaFacetView.js is run, else it will throw error. > > Could that same script order be maintained but at the top of the file? Or is > there some issue with having the CE defined and upgraded during parse? yes,i guess facet-date custom element is not available when the code in glodaFacetView.js is run.. The errors occus before the conencted callback of the facet-date is called.. that's why i put all the script at the bottom.. I was aboutt to ask you the difference between the lifecicle of xbl element and ce.
Comment 10•6 years ago
|
||
Comment on attachment 9015276 [details] [diff] [review] facet-date_3.patch Review of attachment 9015276 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/glodaFacet.js @@ +9,5 @@ > + return this.buildFunc; > + } > + > + get brushItems() { > + return (aItems) => this.vis.hoverItems(aItems); can't be right. brushItems was not a getter ::: mail/base/content/glodaFacetView.xhtml @@ +93,5 @@ > + <script type="application/javascript" > + src="chrome://messenger/content/protovis-r2.6-modded.js"></script> > + <!-- Facet Binding Stuff that doesn't belong in XBL --> > + <script type="application/javascript" > + src="chrome://messenger/content/glodaFacetVis.js"></script> please put scripts in <head> where they belong (and these are local files so any advantage from doing that as a web page are not there)
Attachment #9015276 -
Flags: review?(mkmelin+mozilla) → review-
Comment 11•6 years ago
|
||
(In reply to Arshad Khan [:arshad] from comment #9) > (In reply to Brian Grinstead [:bgrins] from comment #8) > > (In reply to Arshad Khan [:arshad] from comment #7) > > > (In reply to Arshad Khan [:arshad] from comment #2) > > > > Comment on attachment 9015249 [details] [diff] [review] > > > > facet-date.patch > > > > > > > > Review of attachment 9015249 [details] [diff] [review]: > > > > ----------------------------------------------------------------- > > > > > > > > ::: mail/base/content/glodaFacet.js > > > > @@ +4,5 @@ > > > > > + > > > > > +/* global MozXULElement, DateFacetVis, FacetContext */ > > > > > + > > > > > +class MozFacetDate extends MozXULElement { > > > > > + get a() { > > > > > > > > I don't know why I am not able to get a/build/brushItems/clearBrushedItems > > > > from the cuswtom element node.. any idea why? > > > > > > the reason was the position of scripts in glodaView..xhtml. I have put all > > > the scripts at the bottom of page, so that custom element is created first > > > then the code of glodaFacetView.js is run, else it will throw error. > > > > Could that same script order be maintained but at the top of the file? Or is > > there some issue with having the CE defined and upgraded during parse? > > yes,i guess facet-date custom element is not available when the code in > glodaFacetView.js is run.. The errors occus before the conencted callback of > the facet-date is called.. that's why i put all the script at the bottom.. I > was aboutt to ask you the difference between the lifecicle of xbl element > and ce. As long as you put the script that registeres the CE above the script that runs the page scripts that rely on it, I don't expect there would be a problem with them being at the top of the page. What errors are you seeing?
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #10) > Comment on attachment 9015276 [details] [diff] [review] > facet-date_3.patch > > Review of attachment 9015276 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/base/content/glodaFacet.js > @@ +9,5 @@ > > + return this.buildFunc; > > + } > > + > > + get brushItems() { > > + return (aItems) => this.vis.hoverItems(aItems); > > can't be right. brushItems was not a getter I have done this intentionally. The variables/properties of custom element are private e.g., you can't call methods of custom element using the custome element node. Only way to have reference of ce inner variables and also have an exposed method is to set a getter.. If you see glodaFacetView.js, it uses brushItems, clearBrushedItems, build methods. I dont think this is something wrong syntax or styles wise. > ::: mail/base/content/glodaFacetView.xhtml > @@ +93,5 @@ > > + <script type="application/javascript" > > + src="chrome://messenger/content/protovis-r2.6-modded.js"></script> > > + <!-- Facet Binding Stuff that doesn't belong in XBL --> > > + <script type="application/javascript" > > + src="chrome://messenger/content/glodaFacetVis.js"></script> > > please put scripts in <head> where they belong > (and these are local files so any advantage from doing that as a web page > are not there) There are not their for any advantage. I have put down those files just to make things work. If put above, you ll see that the before the creation of custom element the code in glodaFacetView.js is called, which doesn't work obviously becuase customelement code is not executed till then.
Assignee | ||
Comment 13•6 years ago
|
||
Attachment #9015276 -
Attachment is obsolete: true
Attachment #9015276 -
Flags: feedback?(bgrinstead)
Attachment #9015324 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #9015324 -
Attachment is obsolete: true
Attachment #9015324 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9015325 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #11) > (In reply to Arshad Khan [:arshad] from comment #9) > > (In reply to Brian Grinstead [:bgrins] from comment #8) > > > (In reply to Arshad Khan [:arshad] from comment #7) > > > > (In reply to Arshad Khan [:arshad] from comment #2) > > > > > Comment on attachment 9015249 [details] [diff] [review] > > > > > facet-date.patch > > > > > > > > > > Review of attachment 9015249 [details] [diff] [review]: > > > > > ----------------------------------------------------------------- > > > > > > > > > > ::: mail/base/content/glodaFacet.js > > > > > @@ +4,5 @@ > > > > > > + > > > > > > +/* global MozXULElement, DateFacetVis, FacetContext */ > > > > > > + > > > > > > +class MozFacetDate extends MozXULElement { > > > > > > + get a() { > > > > > > > > > > I don't know why I am not able to get a/build/brushItems/clearBrushedItems > > > > > from the cuswtom element node.. any idea why? > > > > > > > > the reason was the position of scripts in glodaView..xhtml. I have put all > > > > the scripts at the bottom of page, so that custom element is created first > > > > then the code of glodaFacetView.js is run, else it will throw error. > > > > > > Could that same script order be maintained but at the top of the file? Or is > > > there some issue with having the CE defined and upgraded during parse? > > > > yes,i guess facet-date custom element is not available when the code in > > glodaFacetView.js is run.. The errors occus before the conencted callback of > > the facet-date is called.. that's why i put all the script at the bottom.. I > > was aboutt to ask you the difference between the lifecicle of xbl element > > and ce. > > As long as you put the script that registeres the CE above the script that > runs the page scripts that rely on it, I don't expect there would be a > problem with them being at the top of the page. What errors are you seeing? I get `this.mn is null. Can't access it weaklistener property` error - https://searchfox.org/comm-central/source/mozilla/toolkit/mozapps/extensions/amInstallTrigger.js#41 I extend MozXulElement then I get this error when I put scripts at top but when I put script at bottom then there is no error. Strange thing is if I use HTMLElement then I dont get any errors irrespective of the position of script. You can check https://www.youtube.com/watch?v=S5bYDqdAL8Y , the last section of the video shows the MozXULElement version.
Assignee | ||
Comment 16•6 years ago
|
||
rebased. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=082c1747747950683a17a9ffabf2b38384f53092
Attachment #9015325 -
Attachment is obsolete: true
Attachment #9015325 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9015357 -
Flags: review?(mkmelin+mozilla)
Comment 17•6 years ago
|
||
Comment on attachment 9015357 [details] [diff] [review] facet-date_6.patch Review of attachment 9015357 [details] [diff] [review]: ----------------------------------------------------------------- Clicking on the space next to "People" I get node is null; can't access its "parentNode" property glodaFacetBindings.xml:1204 showPopup chrome://messenger/content/glodaFacetBindings.xml:1204:13 onxblclick chrome://messenger/content/glodaFacetBindings.xml:1243:1 I don't get that on trunk. Other than that, seems to be working ::: mail/base/content/glodaFacetView.xhtml @@ +14,5 @@ > ]> > <html xmlns="http://www.w3.org/1999/xhtml" > + xmlns:html="http://www.w3.org/1999/xhtml" > + xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > + version="-//W3C//DTD XHTML 1.1//EN" xml:lang="en" since you're touching this, remove the xml:lang="en" - that would be incorrect for localized builds
Attachment #9015357 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 18•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #17) > Comment on attachment 9015357 [details] [diff] [review] > facet-date_6.patch > > Review of attachment 9015357 [details] [diff] [review]: > ----------------------------------------------------------------- > > Clicking on the space next to "People" I get > > node is null; can't access its "parentNode" property > glodaFacetBindings.xml:1204 > showPopup > chrome://messenger/content/glodaFacetBindings.xml:1204:13 > onxblclick > chrome://messenger/content/glodaFacetBindings.xml:1243:1 > > This is not a problem produced by this patch.. you can check it on default branch and see that it throws error even then.
Assignee | ||
Updated•6 years ago
|
Attachment #9015518 -
Flags: review?(mkmelin+mozilla)
Comment 20•6 years ago
|
||
Comment on attachment 9015518 [details] [diff] [review] facet-date_7.patch Review of attachment 9015518 [details] [diff] [review]: ----------------------------------------------------------------- Seems to be working. r=mkmelin
Attachment #9015518 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 21•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/1bffb3f6d7a9 Migrate facet-date binding to custom element. r=mkmelin
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 64.0
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•