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

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 64.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Posted patch facet-date.patch (obsolete) — Splinter Review
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?
Posted patch facet-date_1.patch (obsolete) — Splinter Review
Attachment #9015249 - Attachment is obsolete: true
Posted patch facet-date_2.patch (obsolete) — Splinter Review
Attachment #9015260 - Attachment is obsolete: true
Attachment #9015274 - Flags: review?(mkmelin+mozilla)
Posted patch facet-date_3.patch (obsolete) — Splinter Review
Attachment #9015274 - Attachment is obsolete: true
Attachment #9015274 - Flags: review?(mkmelin+mozilla)
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
Attachment #9015276 - Flags: review?(mkmelin+mozilla)
Attachment #9015276 - Flags: feedback?(bgrinstead)
(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.
(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?
(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 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-
(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?
(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.
Posted patch facet-date_4.patch (obsolete) — Splinter Review
Attachment #9015276 - Attachment is obsolete: true
Attachment #9015276 - Flags: feedback?(bgrinstead)
Attachment #9015324 - Flags: review?(mkmelin+mozilla)
Posted patch facet-date_5.patch (obsolete) — Splinter Review
Attachment #9015324 - Attachment is obsolete: true
Attachment #9015324 - Flags: review?(mkmelin+mozilla)
Attachment #9015325 - Flags: review?(mkmelin+mozilla)
(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.
Attachment #9015357 - Flags: review?(mkmelin+mozilla)
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)
(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.
Attachment #9015357 - Attachment is obsolete: true
Attachment #9015518 - Flags: review?(mkmelin+mozilla)
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+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1bffb3f6d7a9
Migrate facet-date binding to custom element. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.