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

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

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

Comment 1

[Magnus Melin [:mkmelin]]

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

Comment 2

[Magnus Melin [:mkmelin]]

But button-base was removed in bug 1538983. https://hg.mozilla.org/mozilla-central/rev/936c2d201405

Comment 3

[Magnus Melin [:mkmelin]]

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

Comment 4

[Magnus Melin [:mkmelin]]

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

Comment 5

[Magnus Melin [:mkmelin]]

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:

• binding: https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/common/bindings/menubutton.xml#117
• hooked up here: https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/mail/base/content/bindings.css#49

Comment 6

[Magnus Melin [:mkmelin]]

The MozButtonBase base class already exists now.

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: bug1546352_menubuttton_dexbl.patch (WIP v0)

WIP

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: bug1546352_menubuttton_dexbl.patch

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: bug1546352_menubuttton_dexbl.patch

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

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: bug1546352_menubuttton_dexbl.patch

Comment 11

[Magnus Melin [:mkmelin]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aa56a5093833f0385d05b19419149cd13a088c2d

Comment 12

[Khushil Mistry [: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.

Comment 13

[Khushil Mistry [:khushil324]]

Attachment: Screenshot 2019-05-18 at 4.47.10 PM.png

Comment 14

[Jorg K (CEST = GMT+2)]

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

Comment 15

[:aceman]

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

Comment 16

[Magnus Melin [:mkmelin]]

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.

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: bug1546352_menubuttton_dexbl.patch

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.

Comment 18

[Jorg K (CEST = GMT+2)]

Richard, can you help here if required.

Comment 19

[Richard Marti (:Paenglab)]

Attachment: 1546352-fix-button-styling.patch

This fixes the buttons on Mac and Windows.

Comment 20

[Khushil Mistry [:khushil324]]

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.

Comment 21

[Khushil Mistry [:khushil324]]

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.

Comment 22

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b9345e5f3c4697148f17d7148eb0d1ad263fa291

Comment 23

[Jorg K (CEST = GMT+2)]

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.

Comment 24

[Magnus Melin [:mkmelin]]

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

Thanks Richard!

Comment 25

[Richard Marti (:Paenglab)]

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

Comment 26

[Magnus Melin [:mkmelin]]

Right!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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