Closed Bug 1546352 Opened 6 years ago Closed 6 years ago

[de-xbl] convert the menu-button-base + menubutton.xml#menu-button bindings to custom element

Categories

(Thunderbird :: General, task)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(3 files, 4 obsolete files)

Screenshot 2019-05-18 at 4.47.10 PM.png
(deleted), image/png
Details
bug1546352_menubuttton_dexbl.patch
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
1546352-fix-button-styling.patch
(deleted), patch
jorgk-bmo
: review+
khushil324
: review+
Details | Diff | Splinter Review

The menu-button binding https://searchfox.org/comm-central/rev/024094c2ebc05723d2b0da86086539d5b97044cb/common/bindings/toolbar.xml#577 is hooked up here: https://searchfox.org/comm-central/rev/024094c2ebc05723d2b0da86086539d5b97044cb/mail/base/content/bindings.css#49

Bug 1538983 is converting the button binding (and button[type="menu"] binding) to custom element. The button-base binding which menu-button inherits from is going away in that conversion. See https://phabricator.services.mozilla.com/D27742

Summary: [de-xbl] convert the menu-button binding to custom element (since it's parent binding button is going away in bug 1538983) → [de-xbl] convert the menu-button binding to custom element (since it's parent binding button-base is going away in bug 1538983)

button-base isn't going away in bug 1546352 after all.

No longer depends on: 1538983
Summary: [de-xbl] convert the menu-button binding to custom element (since it's parent binding button-base is going away in bug 1538983) → [de-xbl] convert the menu-button binding to custom element

(And menu-button-base inherited from button-base. )

Summary: [de-xbl] convert the menu-button binding to custom element → [de-xbl] convert the menu-button-base + menu-button bindings to custom element

Correction: button-base is still there, but will be removed "soon" in bug 1519577 after toolbarbutton is converted in bug 1519577.

Depends on: 1519577

To add confusion here: the searchfox permalinks above are wrong since we actually have two bindings named menu-button: toolbar.xml#menu-button for <toolbarbutton> and menubutton.xml#menu-button for <button>

Let's have this bug for the menubutton.xml#menu-button work:

Summary: [de-xbl] convert the menu-button-base + menu-button bindings to custom element → [de-xbl] convert the menu-button-base + menubutton.xml#menu-button bindings to custom element

The MozButtonBase base class already exists now.

Attached patch bug1546352_menubuttton_dexbl.patch (WIP v0) (obsolete) (deleted) — — Splinter Review

WIP

Assignee: nobody → mkmelin+mozilla
Blocks: 1547238
Depends on: 1551312
Attached patch bug1546352_menubuttton_dexbl.patch (obsolete) (deleted) — — Splinter Review
Attachment #9061111 - Attachment is obsolete: true
Attached patch bug1546352_menubuttton_dexbl.patch (obsolete) (deleted) — — Splinter Review

This should be it, if bug 1551312 gets review. Test adjustments in bug 1547238.

Status: NEW → ASSIGNED
Attachment #9064564 - Attachment is obsolete: true
Attached patch bug1546352_menubuttton_dexbl.patch (obsolete) (deleted) — — Splinter Review
Attachment #9064743 - Attachment is obsolete: true
Attachment #9065619 - Flags: review?(khushil324)
Comment on attachment 9065619 [details] [diff] [review] bug1546352_menubuttton_dexbl.patch Review of attachment 9065619 [details] [diff] [review]: ----------------------------------------------------------------- There is a linting error : 28:38 error 'MozXULElement' is not defined. no-undef (eslint) Also, On Mac, the button container has some padding and background color which is not similar to the trunk. I have attached the SS.
Attachment #9065619 - Flags: review?(khushil324) → review-

The "Folder Location" widget is also pretty much completely hosed. Will that be fixed here or do I need to fix another bug?

I thank that is a separate bug, please file me a bug.

For the visuals on mac, I can't really help. On linux it looks the same as it used to.
I wonder if adding the -moz-appearance: dualbutton; on mac could help: https://searchfox.org/comm-central/rev/95a184ce92612a12cb68ed2d7284a0c1c02cff30/mail/themes/linux/mail/messenger.css#810

I've fixed the linting error and unbitrotted.

Lint issue fixed and unbitrotted.
Since you're on mac, maybe you can have a look at the styling problem?

I wonder if some style changes are required on windows.

Attachment #9065619 - Attachment is obsolete: true
Attachment #9065910 - Flags: review?(khushil324)

Richard, can you help here if required.

Flags: needinfo?(richard.marti)

This fixes the buttons on Mac and Windows.

Flags: needinfo?(richard.marti)
Attachment #9065913 - Flags: review?(khushil324)
Attachment #9065913 - Flags: review?(jorgk)
Comment on attachment 9065910 [details] [diff] [review] bug1546352_menubuttton_dexbl.patch Review of attachment 9065910 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me with applying Richard's patch. r=khushil.
Attachment #9065910 - Flags: review?(khushil324) → review+
Comment on attachment 9065913 [details] [diff] [review] 1546352-fix-button-styling.patch Review of attachment 9065913 [details] [diff] [review]: ----------------------------------------------------------------- Looks good on Mac. r=khushil.
Attachment #9065913 - Flags: review?(khushil324) → review+
Comment on attachment 9065913 [details] [diff] [review] 1546352-fix-button-styling.patch Which buttons do I need to look at? The attachment reminder looks OK now, also the "New" button in the filter dialogue.
Attachment #9065913 - Flags: review?(jorgk) → review+

I think those are the only ones. We have a bunch of <toolbarbutton type="menu-button"> but that is still xbl.

Thanks Richard!

Keywords: checkin-needed

Also the "Get Map..." buttons in AB.

Right!

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9331b129db1c
[de-xbl] convert the menu-button-base + menubutton.xml#menu-button bindings to custom element. r=khushil
https://hg.mozilla.org/comm-central/rev/96c877c4aba1
Fix the button styling on Mac and Windows. r=jorgk,khushil

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

Attachment

General

Creator:
Created:
Updated:
Size: