1559127 - Follow up on implementing the new appmenu in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Paul Morris [:pmorris]]

Now that patches from bug 1546309 have landed, allowing beta 68 releases to proceed, this is for follow-up work to finish integrating the new appmenu code. For example, address a number of "TODO appmenu" comments, and do a more thorough round of review of the part 3 patch from bug 1546309.

The most pressing things are fixing the folder submenus (bug 1558565) and updating Calendar UI (bug 1558599). Once those are done, then comes this bug.

Comment 1

[Paul Morris [:pmorris]]

A little bit of this is handled in bug 1563789.

Comment 2

[Paul Morris [:pmorris]]

Attachment: part1-back-button-aria-label-0.patch

Fixes adding an "aria-label" to the back buttons in the appmenu. There is more to do for this bug that is in progress, but I wanted to go ahead and get this patch up for review, because...

This involves adding a string, so we'll need to decide its fate for TB 68, since it is already after the string freeze. Adding needinfo requests on that.

Comment 3

[Paul Morris [:pmorris]]

Attachment: part2-disable-recently-closed-tabs-0.patch

Another little piece of this that should make it into TB 68 since it is a user visible change. More work on this bug is in progress, but I wanted to go ahead and get this patch moving.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Paul Morris [:pmorris] from comment #2)

This involves adding a string, so we'll need to decide its fate for TB 68, since it is already after the string freeze. Adding needinfo requests on that.

Can't you use the "Back" string from elsewhere?
browser/locales/en-US/chrome/browser/browser.dtd
197 <!ENTITY backCmd.label "Back">

mail/locales/en-US/chrome/messenger/viewSource.dtd
76 <!ENTITY backCmd.label "Back">

See:
https://groups.google.com/d/msg/mozilla.dev.l10n/xgjiOmwuNgk/LbXDSfbtAwAJ
:-(

Comment 5

[Paul Morris [:pmorris]]

Attachment: beta-part1-back-button-aria-label-0.patch

(In reply to Jorg K (GMT+2) from comment #4)

Can't you use the "Back" string from elsewhere?

Here's a patch that takes this approach, that we could use for 68/beta since the string freeze has already happened.

Unfortunately, the existing "Back" strings are in ".dtd" files, (rather than in a ".properties" file like this code expects). I didn't find a way to directly access the strings in a .dtd file from JS code. So this patch uses an ugly temporary workaround. I'd be glad to know if there's a better way to access the strings in .dtd files.

We'd still want to use the approach in the other patch for trunk/TB 69, to be rid of the ugly workaround.

(It's unfortunate that this slipped through the cracks and didn't make it in before the string freeze. It would have been better if I had made this back button string issue a separate bug. It slipped my mind, as it was lumped in with the other follow-up work on the appmenu, and I've been focusing on fixing problems that are more visible and disruptive to users.)

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9076361 [details] [diff] [review]
beta-part1-back-button-aria-label-0.patch

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

This is a good approach (actually it's a terrible hack but it works), but rather than adding an element just for this job, just set the string as an attribute on an existing element. I guess appmenu-popup?

Comment 7

[Jorg K (CEST = GMT+2)]

So the (modified) aria patch will go to beta as well as the other one? You should request beta approval so I can see it.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/56dea140befc
Fix aria-label for back buttons in the appmenu. r=darktrojan
https://hg.mozilla.org/comm-central/rev/c14bd4ff44b0
Disable the appmenu's "Go > Recently Closed Tabs" when needed. r=darktrojan

Comment 9

[Paul Morris [:pmorris]]

Hi Jorgk,

"part1-back-button-aria-label-0.patch" is for trunk.
"beta-part1-back-button-aria-label-0.patch" is for beta/68.

Also, darktrojan gave me r+ on the "beta-part1-" patch but asked me to change something, so it wasn't ready to push yet.

The "beta-part1-" patch is now pushed to trunk. Let me know how you want to resolve that, by backing it out or by me creating a new patch to go on top of it.

The "part2" patch will need to go to beta as well, as I mentioned above. I can make a "beta-" version of it, but I think the non-beta version of it should apply cleanly, so that may not be needed. Thanks for the tip about using the approval-comm-beta flag.

Also, there is more work to do for this bug (I mentioned this in a comment above) so it is not resolved/fixed yet. Probably would be clearer to have created separate bugs for these patches. I'm setting the bug to reopened.

Comment 10

[Jorg K (CEST = GMT+2)]

What? The "beta-part1-" patch is now pushed to trunk? How so? Scrolling back my window I see:

$ hg out
comparing with https://hg.mozilla.org/comm-central
searching for changes
changeset: 27189:56dea140befc
tag: part1-back-button-aria-label-0.patch
tag: qbase
user: Paul Morris <[e-mail]>
date: Fri Jul 05 17:21:53 2019 -0400
summary: Bug 1559127 - Fix aria-label for back buttons in the appmenu. r=darktrojan

changeset: 27190:c14bd4ff44b0
tag: part2-disable-recently-closed-tabs-0.patch
tag: qtip
tag: tip
user: Paul Morris <[e-mail]>
date: Wed Jul 03 16:59:28 2019 -0400
summary: Bug 1559127 - Disable the appmenu's "Go > Recently Closed Tabs" when needed. r=darktrojan

https://hg.mozilla.org/comm-central/rev/56dea140befc is part 1 for trunk with the string change here:
https://hg.mozilla.org/comm-central/rev/56dea140befc#l2.1

Who is confused? You or me?

Comment 11

[Paul Morris [:pmorris]]

sigh Aha, thank you for pointing that out. Glad everything is as it should be and that I had just gotten confused. (I should probably wait until I'm fully awake...)

Although, as I mentioned, there is more to do with this bug (more patches to finish and upload), so that's why I had set it to REOPENED, since it is not RESOLVED/FIXED yet.

Comment 12

[Jorg K (CEST = GMT+2)]

Well, we mark it resolved, if trunk is resolved. If there are more parts to come, we'd set "leave open". Is that the case? Then set that flag and set it back to ASSIGNED (via REOPENED).

Comment 13

[Paul Morris [:pmorris]]

Okay, good to know, thanks. There are more patches to come, so I'm setting the "leave-open" flag, and back to ASSIGNED.

Comment 14

[Paul Morris [:pmorris]]

Attachment: beta-part1-back-button-aria-label-0.patch

1 of 2 for beta. Makes the change from darktrojan's review.

Comment 15

[Paul Morris [:pmorris]]

Attachment: beta-part1-back-button-aria-label-1.patch

Argh, I uploaded the wrong patch file. This is the right one. (Today is not my day.)

Comment 16

[Paul Morris [:pmorris]]

Attachment: beta-part2-disable-recently-closed-tabs-0.patch

2 of 2 for beta.

Comment 17

[Paul Morris [:pmorris]]

Here is a try run for the part1 and part2 trunk patches:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b401459f01dd0cfdc6fd298d3fa20b538333d9d1

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076632 [details] [diff] [review]
beta-part2-disable-recently-closed-tabs-0.patch

If it's the same as the trunk patch, I'll just transplant that.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076631 [details] [diff] [review]
beta-part1-back-button-aria-label-1.patch

OK, thanks.

Comment 20

[Paul Morris [:pmorris]]

Attachment: part3-remove-unused-code-0.patch

This part3 patch is the last one needed to finish out this bug. It mostly removes unused code, some left over from the old appmenu, and some that came in with the new appmenu, but for things we're not using.

Comment 21

[Geoff Lankow (:darktrojan)]

Comment on attachment 9076642 [details] [diff] [review]
part3-remove-unused-code-0.patch

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

This is good, but there's still large chunks of Firefox Account stuff here. I suspect any rule with "fxa" in it can be removed.

::: mail/components/customizableui/content/panelUI.js
@@ +53,1 @@
>        // overflowPanel: "widget-overflow",

Take this all away if we're not using it, unless you have plans to use it again soon.

::: mail/themes/shared/customizableui/panelUI.css
@@ +54,2 @@
>  panelview:not([visible]) {
> +  /* This style was added for TB use. */

Is it not all for TB use? ;-)

@@ +1237,5 @@
>    color: inherit;
>  }
>  
>  #zoom-controls[cui-areatype="toolbar"] > #zoom-reset-button > .toolbarbutton-text {
>  

This white-space and the one just below can go.

Comment 22

[Jorg K (CEST = GMT+2)]

Geoff, Paul is away as I just saw on the calendar. Can you please make the necessary adjustments. Or do we do yet another follow-up?

Comment 23

[Paul Morris [:pmorris]]

Attachment: part3-remove-unused-code-1.patch

This patch addresses the review comments. It removes all the CSS rules pertaining to Firefox accounts (the ones containing "fxa").

Comment 24

[Paul Morris [:pmorris]]

Here's a try server run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1db5aafd0ad017ea3d9eba3e085be3c08c083fbb

Comment 25

[Geoff Lankow (:darktrojan)]

The sendTabToDevice and remotetabs is also part of FxA. There's probably a lot more stuff that doesn't apply to Thunderbird, but as you say, at some point it's time to stop digging.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076994 [details] [diff] [review]
part3-remove-unused-code-1.patch

Does this patch apply to beta cleanly? I haven't tried.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/29d01501d31c
Remove unused code and other new appmenu follow-ups. r=darktrojan

Comment 28

[Jorg K (CEST = GMT+2)]

TB 69 beta (TB 68 beta coming):
https://hg.mozilla.org/releases/comm-beta/rev/3477bbfe61fb4c9a5b06841f22cb3b0c11f5b87b

Comment 29

[Jorg K (CEST = GMT+2)]

TB 68 beta 5:
https://hg.mozilla.org/releases/comm-beta/rev/feabcfb77b987338e3a28a8b7d6440ff9e0e2e23
https://hg.mozilla.org/releases/comm-beta/rev/658bff9d5cc55ec3bc9c0c4280127d78773ee064
https://hg.mozilla.org/releases/comm-beta/rev/bd356636aab529d2137c9a0a2f1c06244962d780