Closed
Bug 1515308
Opened 6 years ago
Closed 6 years ago
[de-xbl] Remove facet-base binding and migrate facet-boolean and facet-boolean-filtered bindings to custom element.
Categories
(Thunderbird :: Mail Window Front End, task)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 66.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
(Whiteboard: [bug 1516640 must land first])
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee: nobody → arshdkhn1
Assignee | ||
Updated•6 years ago
|
Blocks: tb-war-on-xbl
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #9032402 -
Attachment is obsolete: true
Attachment #9032810 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years 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 3•6 years ago
|
||
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
Updated•6 years ago
|
Attachment #9032810 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9032810 -
Attachment is obsolete: true
Attachment #9033388 -
Flags: review?(mkmelin+mozilla)
Comment 5•6 years ago
|
||
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•6 years ago
|
||
Attachment #9033388 -
Attachment is obsolete: true
Attachment #9033535 -
Flags: review?(philipp)
Attachment #9033535 -
Flags: feedback+
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #9033535 -
Attachment is obsolete: true
Attachment #9033535 -
Flags: review?(philipp)
Assignee | ||
Comment 8•6 years ago
|
||
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•6 years ago
|
Attachment #9033540 -
Flags: review?(philipp) → review?(mkmelin+mozilla)
Comment 9•6 years ago
|
||
> 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)
Updated•6 years ago
|
Attachment #9033540 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #9033540 -
Attachment is obsolete: true
Attachment #9035597 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #9035597 -
Attachment is obsolete: true
Attachment #9035597 -
Flags: review?(mkmelin+mozilla)
Attachment #9035598 -
Flags: review?(mkmelin+mozilla)
Comment 12•6 years ago
|
||
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+
Updated•6 years ago
|
Status: NEW → ASSIGNED
Updated•6 years ago
|
Keywords: checkin-needed
Whiteboard: [bug 1516640 must land first]
Comment 13•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0a09c6d031d8
Migrate facet-base bindings to custom element. r=mkmelin
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 66.0
Updated•5 years ago
|
Type: defect → task
You need to log in
before you can comment on or make changes to this bug.
Description
•