1491758 - [de-xbl] Convert facet-date to custom element.

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Arshad Khan [:arshad]]

Attachment: facet-date.patch

Comment 2

[Arshad Khan [:arshad]]

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?

Comment 3

[Arshad Khan [:arshad]]

Attachment: facet-date_1.patch

Comment 4

[Arshad Khan [:arshad]]

Attachment: facet-date_2.patch

Comment 5

[Arshad Khan [:arshad]]

Attachment: facet-date_3.patch

Comment 6

[Arshad Khan [:arshad]]

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

Comment 7

[Arshad Khan [:arshad]]

(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

[Brian Grinstead [:bgrins]]

(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?

Comment 9

[Arshad Khan [:arshad]]

(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

[Magnus Melin [:mkmelin]]

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)

Comment 11

[Brian Grinstead [:bgrins]]

(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?

Comment 12

[Arshad Khan [:arshad]]

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

Comment 13

[Arshad Khan [:arshad]]

Attachment: facet-date_4.patch

Comment 14

[Arshad Khan [:arshad]]

Attachment: facet-date_5.patch

Comment 15

[Arshad Khan [:arshad]]

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

Comment 16

[Arshad Khan [:arshad]]

Attachment: facet-date_6.patch

rebased.

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

Comment 17

[Magnus Melin [:mkmelin]]

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

Comment 18

[Arshad Khan [:arshad]]

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

Comment 19

[Arshad Khan [:arshad]]

Attachment: facet-date_7.patch

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9015518 [details] [diff] [review]
facet-date_7.patch

Review of attachment 9015518 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to be working. r=mkmelin

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1bffb3f6d7a9
Migrate facet-date binding to custom element. r=mkmelin