Closed Bug 1491758 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 7 obsolete files)

No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Attached 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?
Attached patch facet-date_1.patch (obsolete) — Splinter Review
Attachment #9015249 - Attachment is obsolete: true
Attached patch facet-date_2.patch (obsolete) — Splinter Review
Attachment #9015260 - Attachment is obsolete: true
Attachment #9015274 - Flags: review?(mkmelin+mozilla)
Attached 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.
Attached 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)
Attached 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: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.