Arshad Khan [:arshad] Assignee
	Description • 6 years ago

      No description provided.

	Arshad Khan [:arshad] 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?

	Arshad Khan [:arshad] 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

	Arshad Khan [:arshad] 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.

	Brian Grinstead [:bgrins]
	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?

	Arshad Khan [:arshad] 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.

	Magnus Melin [:mkmelin]
	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)

	Brian Grinstead [:bgrins]
	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?

	Arshad Khan [:arshad] 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.

	Arshad Khan [:arshad] 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.

	Arshad Khan [:arshad] Assignee
	Comment 16 • 6 years ago

rebased.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=082c1747747950683a17a9ffabf2b38384f53092

	Magnus Melin [:mkmelin]
	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

	Arshad Khan [:arshad] 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.

	Magnus Melin [:mkmelin]
	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

	Pulsebot
	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