[de-xbl] Remove facet-base binding and migrate facet-boolean and facet-boolean-filtered bindings to custom element.

RESOLVED FIXED in Thunderbird 66.0

Status

defect
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 66.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [bug 1516640 must land first])

Attachments

(1 attachment, 7 obsolete attachments)

Assignee

Description

5 months ago
No description provided.
Assignee

Comment 1

5 months ago
Posted patch facet-base-bindings.patch (obsolete) — Splinter Review
Assignee: nobody → arshdkhn1
Assignee

Updated

5 months ago
Assignee

Comment 2

5 months ago
Posted patch facet-base-bindings.patch (obsolete) — Splinter Review
Attachment #9032402 - Attachment is obsolete: true
Attachment #9032810 - Flags: review?(mkmelin+mozilla)
Assignee

Updated

5 months ago
Summary: [de-xbl] Migrate facet-base bindings to custom element. → [de-xbl] Remove facet-base binding and migrate facet-boolean and facet-boolean-filtered bindings to custom element.
Comment on attachment 9032810 [details] [diff] [review]
facet-base-bindings.patch

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

Seems to work

I do see some bugs (rendering issues such as overlapping texts and clicking around fairly easily something is null. But this happens on trunk too.

::: mail/base/content/glodaFacet.js
@@ +122,5 @@
> +
> +  set disabled(val) {
> +    if (val) {
> +      this.setAttribute("disabled", "true");
> +      this.checkbox.setAttribute("disabled", true);

"true"

@@ +141,5 @@
> +    this.checkbox.checked = val;
> +    if (val) {
> +      // the XBL inherits magic appears to fail if we explicitly check the
> +      //  box itself rather than via our click handler, presumably because
> +      //  we unshadow something.  So manually apply changes ourselves.

presumably this could go? please investigate and remove, or change the comment to reflect current state without xbl
Attachment #9032810 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 4

5 months ago
Posted patch facet-base-bindings.patch (obsolete) — Splinter Review
Attachment #9032810 - Attachment is obsolete: true
Attachment #9033388 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9033388 [details] [diff] [review]
facet-base-bindings.patch

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

Looks good to me, just a couple of style nits. r=mkmelin

::: mail/base/content/glodaFacetBindings.xml
@@ +122,2 @@
>  
> +        if(aType === "boolean") {

if (

@@ +123,5 @@
> +        if(aType === "boolean") {
> +          facet = document.createElement("facet-boolean");
> +          facet.setAttribute("name", aAttrDef.attributeName);
> +          facets.appendChild(facet);
> +        } else if(aType === "boolean-filtered") {

} else if (
Attachment #9033388 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 6

5 months ago
Posted patch facet-base-bindings.patch (obsolete) — Splinter Review
Attachment #9033388 - Attachment is obsolete: true
Attachment #9033535 - Flags: review?(philipp)
Attachment #9033535 - Flags: feedback+
Assignee

Updated

5 months ago
Depends on: 1516640
Assignee

Comment 7

5 months ago
Posted patch facet-base-bindings.patch (obsolete) — Splinter Review
Attachment #9033535 - Attachment is obsolete: true
Attachment #9033535 - Flags: review?(philipp)
Assignee

Comment 8

5 months ago
Posted patch facet-base-bindings.patch (obsolete) — Splinter Review
This patch now depends upon facets binding patch(Bug 1516640). Magnus I have made some changes, I dont think it needs a separate feedback request. You can take a look at the changes tho.
Attachment #9033539 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #9033540 - Flags: review?(philipp)
Attachment #9033540 - Flags: feedback+
Assignee

Updated

5 months ago
Blocks: 1516647
Assignee

Updated

5 months ago
Attachment #9033540 - Flags: review?(philipp) → review?(mkmelin+mozilla)
> This patch now depends upon facets binding patch(Bug 1516640). Magnus I have
> made some changes, I dont think it needs a separate feedback request. You
> can take a look at the changes tho.

Why? Seems like a bad idea to throw in that as a dependency just before landing. Please always focus on landing as small parts as possible. This will help reviews, and also regression hunting.

I tried it now with bug 1516640 also attached. Nothing works anymore. I just get

facet is undefined glodaFacetView.js:137 and then it's of course downhill from there.
Flags: needinfo?(mkmelin+mozilla)
Attachment #9033540 - Flags: review?(mkmelin+mozilla) → review-
Assignee

Comment 10

4 months ago
Posted patch facet-base-bindings.patch (obsolete) — Splinter Review
Attachment #9033540 - Attachment is obsolete: true
Attachment #9035597 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 11

4 months ago
Attachment #9035597 - Attachment is obsolete: true
Attachment #9035597 - Flags: review?(mkmelin+mozilla)
Attachment #9035598 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9035598 [details] [diff] [review]
facet-base-bindings.patch

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

Looks good to me, r=mkmelin
Attachment #9035598 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [bug 1516640 must land first]

Comment 13

4 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0a09c6d031d8
Migrate facet-base bindings to custom element. r=mkmelin

Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

4 months ago
Target Milestone: --- → Thunderbird 66.0
Depends on: 1520464
You need to log in before you can comment on or make changes to this bug.