Closed Bug 1554627 Opened 8 months ago Closed 6 months ago

[de-xbl] convert/rework the tabmail-alltabs-popup binding

Categories

(Thunderbird :: Toolbars and Tabs, task, P2)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

Attachments

(1 file, 4 obsolete files)

The parent-class popup is getting removed in bug 1555497.

Depends on: 1555497
Assignee: nobody → alessandro
Priority: -- → P2

First patch to simply de-xbl the all tabs popup.
This CE extends the menupopup element, which is a CE itself and doesn't depend on the soon to be removed popup parent.

Next step is to see if the Firefox approach can be integrated.

Attachment #9079154 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9079154 [details] [diff] [review]
1554627-tabmail-alltabs-popup.patch

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

Looks pretty good. All things considering I wouldn't spend too much time following Firefox here (our tabs impl. is anyway too different)

::: mail/base/content/tabmail.js
@@ +8,5 @@
> +
> +{
> +  /**
> +   * The MozTabmailAlltabsPopup widget is used as a menupopup
> +   * to list all the currently opened tabs.

please utilize the full 80 ch

@@ +11,5 @@
> +   * The MozTabmailAlltabsPopup widget is used as a menupopup
> +   * to list all the currently opened tabs.
> +   *
> +   * @extends {MozElements.MozMenuPopup}
> +   */

and @implements {EventListener}

@@ +12,5 @@
> +   * to list all the currently opened tabs.
> +   *
> +   * @extends {MozElements.MozMenuPopup}
> +   */
> +

no blank line before the class decl please

@@ +65,5 @@
> +      }
> +
> +      this.tabmail = document.getElementById("tabmail");
> +
> +      this._mutationObserver = null;

remove

@@ +67,5 @@
> +      this.tabmail = document.getElementById("tabmail");
> +
> +      this._mutationObserver = null;
> +
> +      this._mutationObserver = new MutationObserver((aRecords, aObserver) => {

let's also remove the a-namings in this class, while we're here

@@ +83,5 @@
> +    }
> +
> +    _tabOnTabClose(aEvent) {
> +      let menuItem = aEvent.target.mCorrespondingMenuitem;
> +      if (menuItem)

and add braces where missing even for one line ifs

@@ +128,5 @@
> +    }
> +
> +    _createTabMenuItem(aTab) {
> +      let menuItem = document.createElementNS(
> +        "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "menuitem");

createXULElement

@@ +176,5 @@
> +      }
> +    }
> +  }
> +
> +  customElements.define("tabmail-alltabs-popup", MozTabmailAlltabsPopup,

let's name it tabmail-alltabs-menupopup since it is actaully a menupopup (and update the class name accordingly)
Attachment #9079154 - Flags: feedback?(mkmelin+mozilla) → feedback+

Sounds good to me in going with our own and more simpler approach.
Ready for a full review.

Attachment #9079154 - Attachment is obsolete: true
Attachment #9079417 - Flags: review?(mkmelin+mozilla)
Attachment #9079417 - Flags: feedback+
Attachment #9079417 - Flags: review?(mkmelin+mozilla) → review?(geoff)
Comment on attachment 9079417 [details] [diff] [review]
1554627-tabmail-alltabs-popup.patch

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

I'd r+ this but I'd like some changes and want to see them before they land.

::: mail/base/content/tabmail.js
@@ +17,5 @@
> +  class MozTabmailAlltabsMenuPopup extends MozElements.MozMenuPopup {
> +    constructor() {
> +      super();
> +
> +      this.addEventListener("popupshowing", (event) => {

I think it makes more sense for these listeners to be added in connectedCallback.

@@ +114,5 @@
> +        return;
> +
> +      let tabstripBO = tabContainer.arrowScrollbox;
> +      for (let i = 0; i < this.childNodes.length; i++) {
> +        let curTabBO = this.childNodes[i].tab;

While we're here, let's rename these variables to tabstrip (tabStrip?) and currentTab. The "BO" tells of an implementation detail long gone that really doesn't matter any more.

@@ +117,5 @@
> +      for (let i = 0; i < this.childNodes.length; i++) {
> +        let curTabBO = this.childNodes[i].tab;
> +        if (curTabBO.screenX >= tabstripBO.screenX &&
> +            curTabBO.screenX + curTabBO.getBoundingClientRect().width <=
> +            tabstripBO.screenX + tabstripBO.getBoundingClientRect().width) {

Oh, yuck. I know you didn't write this but let's make it better. :-) Get the bounding rectangle of the strip and the tab and just compare the .left and .right properties.
Attachment #9079417 - Flags: review?(geoff) → feedback+

Thanks for the detailed review.
Here's the updated patch.

Attachment #9079417 - Attachment is obsolete: true
Attachment #9080074 - Flags: review?(geoff)
Comment on attachment 9080074 [details] [diff] [review]
1554627-tabmail-alltabs-popup.patch

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

Nice, just one minor change.

::: mail/base/content/tabmail.js
@@ +112,5 @@
> +      }
> +
> +      for (let i = 0; i < this.childNodes.length; i++) {
> +        let currentTabBox = this.childNodes[i].tab.getBoundingClientRect();
> +        let tabStripBox = tabStrip.getBoundingClientRect();

This line can go outside the loop so that it only runs once.
Attachment #9080074 - Flags: review?(geoff) → review+

Patch rebased from trunk and the new try push looks good, other than the intermittent X2
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=88654cc7058ec86844bc7592599b5374ef34c0c4

Attachment #9080480 - Attachment is obsolete: true
Attachment #9082308 - Flags: review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c04d43d17059
[de-xbl] convert/rework the tabmail-alltabs-popup binding. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.