[de-xbl] get rid of splitmenu, and rework the appmenu

RESOLVED FIXED in Thunderbird 69.0

Status

task
--
blocker
RESOLVED FIXED
4 months ago
Last month

People

(Reporter: mkmelin, Assigned: pmorris)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 69.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird68+ fixed, thunderbird69 fixed)

Details

Attachments

(19 attachments, 27 obsolete attachments)

48.84 KB, image/png
Details
154.09 KB, image/jpeg
Details
113.88 KB, image/jpeg
Details
20.91 KB, image/png
Details
19.90 KB, image/png
Details
10.85 KB, image/png
Details
21.14 KB, image/png
Details
7.13 KB, patch
Details | Diff | Splinter Review
623.06 KB, patch
Details | Diff | Splinter Review
384.72 KB, patch
aceman
: review+
Details | Diff | Splinter Review
2.96 KB, patch
Paenglab
: review+
Details | Diff | Splinter Review
16.70 KB, patch
Details | Diff | Splinter Review
7.15 KB, patch
Details | Diff | Splinter Review
623.07 KB, patch
Details | Diff | Splinter Review
387.00 KB, patch
Details | Diff | Splinter Review
2.97 KB, patch
Details | Diff | Splinter Review
16.69 KB, patch
Details | Diff | Splinter Review
10.40 KB, patch
Details | Diff | Splinter Review
7.55 KB, patch
Details | Diff | Splinter Review

The menu-vertical binding used by the appmenu (hamburger menu) should be removed.

https://searchfox.org/comm-central/rev/024094c2ebc05723d2b0da86086539d5b97044cb/common/bindings/generalBindings.xml#13

I think this should not be converted as such, but instead we want to follow what Firefox did. We didn't adapt much here over the years, and what Firefox has now is way nicer.

Code should be around

Assignee: nobody → paul

When we follow FX here with the panelUI, then the splitmenu binding will be obsolete too.

Summary: [de-xbl] get rid of menu-vertical binding (appmenu) → [de-xbl] get rid of menu-vertical binding (appmenu), appmenu-vertical, splitmenu
Blocks: 1549634

This is coming along pretty well, but I've got a UI question to run by aleca.

In Firefox there is no "Edit" submenu, just cut, copy, and paste icons on an "Edit" line in the main panel.

In current Thunderbird there is an "Edit" submenu, and also cut, copy, and paste icons in the main menu on the "Edit" line. But doing both (on one line) does not really fit the new menu code. Here are some possibilities. (I tried to take a screenshot, but the panel was omitted from the image for some reason.)

1. | Edit                    > |  (no icons, opens submenu as indicated by right arrow ">" like other items)

2. | Edit [cut] [copy] [paste] |  (just icons, no edit submenu, like Firefox)

3. | Edit [cut] [copy] [paste] |  (icons and clicking "Edit" part opens submenu, but no ">" arrow so it's inconsistent!)

4. | [cut]   [copy]   [paste]  |  (just icons on first line, edit submenu line separate, maybe down under "File")
   |   ...(other lines)...     |
   | File                    > |
   | Edit                    > |

I'm liking #4 as the most straightforward thing that keeps all the functionality of the current TB menu. (Maybe also with text like this "[]Cut []Copy []Paste" on that line?)

Progress report: I've converted over all of the contents of the menu and you can navigate down into the submenus and back. Still to do: submenus that are dynamically populated, make sure the menu items work as expected (some do, but I've noticed some do not), and that items are (de-)activated correctly.

Turns out this basic conversion should not involve any string changes, so no problem with string freeze next week.

Flags: needinfo?(alessandro)
Posted image ff-menu.png

Wouldn't be possible to do what FF is doing on the account item, or the zoom item? Using a vertical separator to have the clickable chevron inline.

If the new menu doesn't allow having an inline chevron alongside other clickable elements (I guess is because the entire item must be clickable?), I'd say option #4 is the one that might look nicer.

Having [cut] [copy] [paste] easily accessible can be useful without the necessity of accessing the submenu, and it's definitely not a good idea to completely remove the Edit submenu.

I'm not sure about writing the command alongside the icon as it might feel a bit cluttered.

Flags: needinfo?(alessandro)
Posted image appmenu0.jpg

I experimented with a few things and came up with this. What do you think? (Apologies for crummy phone camera image...)

I like having the "Edit" text adjacent to the ">" rather than separated from it by the icons. That gives a larger click target with both "Edit" and '>' highlighted together on hover, and it's clearer what clicking it does. That feels more consistent with the other items where the whole row is highlighted (text and '>'). I like putting this row at the top of the menu (given its different horizontal spacing).

As you suggested, I thought about using a horizontal bar to separate the ">" from the icons, like in the Firefox menu, but then you run into the issue of either the ">" is a different 'weight' from the icons or it's not the same as the other ">"s on the other lines.

Agreed (on second thought) that adding the words in would be too crowded.

Posted image appmenu1.jpg

Showing highlight of the "Edit >" on hover.

I find having Edit on the right confusing. Edit is not an action in itself, but a description of the commands on that row (item). So, I'd prefer what Firefox does.
It's also perhaps not warranted to have it on top since the editing is basically only there for people who don't know about Ctrl+C/V. Those people surely exist, but I'm not so sure they find the hamburger menu.

We might also want to re-arrange/re-arrange the groups some, but that can be a separate discussion.

I've installed a more robust/full-featured screenshot app (Shutter).

This seems fine to me, but I'm still not 100% satisfied with it.

Note that in the Firefox appmenu the only lines that open a submenu are simple, like this:

[ Label       > ]

The lines that have several buttons don't open submenus. I like that simplicity and consistency.

I'm not a fan of how the current TB appmenu has lines where the arrow on the right is sometimes a separate button, but you can only see that when hovering over the line. (E.g. "New Message" and "Print...") I was hoping we could get away from that completely. With this design at least the arrow is next to other buttons and there's a vertical separator. Maybe I just have a bad taste in my mouth from the current TB appmenu...

For completeness, another option with separate lines for the icons and for the edit submenu. Spacing of icons would need to be improved, of course. Overall, I'm not sure this is any better than the previous one, maybe slightly worse, maybe, unless the icons were at the top... Anyway, nothing is perfect...

I really like the solution on appmenu-edit-left-arrow-right-1.png

I understand the fact that we're breaking the consistency with the edit submenu not being an entire label, but I don't think it's a big deal.
With the vertical separator, we're visually showing that there's a clear separation of intents, and having the Edit label to the left properly communicates the purpose and objective of the entire section, properly categorizing icons and chevron.
Even the fact that it makes the menu grow in width makes it visually more appealing.

Sounds good to me. I'll go ahead with that.

The Add-on pref submenu is to make the prefs of the old-style extensions accessible. In the Add-on manager you have no possibility to open the prefs of such extensions (the Add-on manager gives only the possibility of inline prefs in the info page of the extension).

Please don't ditch it.

Huh, thanks for the info Richard. For context, on IRC I was discussing with aleca whether we needed the add-ons submenu or whether we could just link to the addons manager.

Seems to me the underlying issue is that you can't access the prefs for the old style add-ons from the add-ons manager. Can we fix that and then users can access everything about add-ons through the add-on manager? Do we have a bug for that yet?

The Add-ons manager is M-C territory. This makes it hard to inject special code. We do some injecting with https://searchfox.org/comm-central/source/mail/base/content/aboutAddonsExtra.js but if this is possible is out of my knowledge. Aceman created this dynamic menu, maybe because it wasn't possible to inject in Add-on manager.

Based on what we discussed during our meeting, here's a potential action plan for the Add-ons section

  • The Add-ons menu lists all the add-ons independently if they have or don't have a preferences panel
  • Clicking on those that have a preferences panel, will open it
  • Clicking on those that don't have a preferences panel, will open the Add-on manager tab

(In reply to Magnus Melin [:mkmelin] from bug 1134760 comment #16)

The app-menu ... Revival and some redesigning in bug 1546309.

Good stuff here. Are we also getting more iconagraphy in appmenu like firefox has? ;)
That would be a nice modernization.
Or is that a separate bug?

This can be done in a follow-up bug. I think this bug is complicated enough.

Depends on: 1542715

Fallen and aleca: the new appmenu is not designed to let addons (i.e. calendar) insert menu items (like with the current/old appmenu). How to handle this? Two most obvious options:

  1. Modify the new appmenu code to allow addons to insert menu items.
  2. Like in Firefox, let addons add their own buttons to the toolbar where they can have their own menus.

To me #2 seems like the better approach.

Flags: needinfo?(philipp)
Flags: needinfo?(alessandro)

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

To me #2 seems like the better approach.

Can't we have our cake and eat it as well. I can see where both options would be of benefit. There is nothing worse than clutter on a toolbar and an entry that has basically an on off function does not really have a purpose on a toolbar, a menu is where it belongs.

Lets not make Thunderbird a product where addons have to be shoe horned into a tiny forced UI corner. They are supposed to make your user experience better. 10 icons on the toolbar for the 10 add-ons you have installed is not going to deliver on that implied promise.

Following Firefox here is not really desirable. They have seriously started to loose the plot in locking does the user interface and with some of the new addon API.

I don't have a full background on this, so take my suggestion with a grain of salt.
I think we should modify the new AppMenu code to allow add-ons to insert menu items.
The reasons behind my suggestion are 3:

  1. Don't force users to relearn something. We're already introducing a new menu style with a different ordering, and it would be better to keep all the items and don't drastically change the paradigm to prevent a typical reaction like "Oh, new menu, cool...wait, where are my add-ons?"
  2. Don't "break" all the add-ons. Basically we will force all the add-ons maintainers to update their code in order to relocated a menuitem.
  3. Too much UI freedom is not good. We should try to keep a loose control of the narrative and guide 3rd party developers to use sections and locations we know they will work for the purpose they need. Allowing the addition of toolbar menu items for each add-on could cause some visual problems we can't control.

Would be possible to have a dedicated section in the new AppMenu, divided by separator, where devs can add their add-on menu item?
As I said, I'm sorry if my thought process is wrong and I don't have all the correct info regarding this issue.
Thoughts?

Flags: needinfo?(alessandro)

As long as we allow add-ons to do anything we'll be breaking all the add-ons with basically any UI change we do. I don't want to go deep into the merits of extensibility here, that is always a heated discussion.

What I can comment on is how this could fit into a WebExtensions API. The browser.menus API allows for different context areas, one of them is tools_menu. I could imagine having a similar context area for other parts of the menu, as appropriate. We should carefully consider where we allow extensibility because once we support it it will be a long term commitment.

I'd suggest going with #2 (which works now via browserAction), and then considering a follow-up if more menu extension points need to be added.

Flags: needinfo?(philipp)

Uploading current WIP patches, this is 1 of 3.

WIP patches: 2 of 3. In this one I just add all the "customizableui" files for now. Maybe not all are needed. Separate patch makes it clear what I've changed in the next patch.

WIP patches: 3 of 3. The main one.

I split off the actual app-menu button into bug 1553768 so we wouldn't get busted completely by toolbarbutton de-xbl landing.

Summary: [de-xbl] get rid of menu-vertical binding (appmenu), appmenu-vertical, splitmenu → [de-xbl] get rid of splitmenu, and rework the appmenu

Nearing review?
Jorg has noted this is a blocker to our building beta.

Flags: needinfo?(paul)

Getting there. Basic functionality is working, but still various things to do before it's ready to land. I should have it ready for review by the end of this week if not sooner.

There are so many edge cases that this looks like a land-it-then-refine kind of process. (Would be better to be doing this not right up against a release, but here we are...)

Folder submenus depend on bug 1542715. Review can start before that, but I don't know if having that working should block the beta build or not.

Flags: needinfo?(paul)

b1 doesn't need to be feature complete. SO perhaps the only reason we should delay beta on this bug is a) help menu is currently broken on beta (doesn't sound like it is) or b) string changes (not sure what you all have decided regarding string schedule)?

Flags: needinfo?(jorgk)

String changes are not needed for this. (Since the strings are the same as those in the current/old appmenu.) Future refinements will likely entail string changes, but that would come after the initial version that's in the TB-68 release, which is more or less a straight conversion of the current appmenu.

Any beta must be usable and free from major regressions. Some people operate using the Hamburger menu, a broken Hamburger menu is an absolute blocker.

Flags: needinfo?(jorgk)

Now that I've looked at bug 1549634 I understand. Which means that the severity should be set so it is clearly communicated this is blocker to release.

That said, this wouldn't be the the worst regression we've ever shipped by any stretch. So if it evolves that development is likely to drag into next week then this release team member's opinion is we should ship with the regression.

Severity: normal → blocker

An updated WIP patch. In case mkmelin has a chance to take a look and/or do a preliminary review.

Attachment #9066602 - Attachment is obsolete: true
Attachment #9068614 - Flags: feedback?(mkmelin+mozilla)

I've tagged things that are still WIP with a "TODO appmenu" comment. The mozmill tests need new infrastructure code to navigate the new appmenu -- currently WIP. I found a potential string addition, in PanelMultiView.jsm, the "aria-label" on the back buttons (which the current appmenu doesn't have):

backButton.setAttribute("aria-label", gBundle.GetStringFromName("panel.back"));

But perhaps we could use the existing mozilla-central string for that, either in place or by copying it over.

Attachment #9068614 - Attachment is obsolete: true
Attachment #9068614 - Flags: feedback?(mkmelin+mozilla)
Attachment #9069161 - Flags: feedback?(mkmelin+mozilla)

Seems to be working well enough. What's missing?

I'm planning on uploading a newer patch later today that fixes several regressions. For example, menu items being enabled/disabled correctly and radio/checkboxes getting updated correctly. There are a few outstanding things, probably not show-stoppers since the current appmenu is not fully-operational:

Several mozmill tests will fail because the tests don't know how to navigate the new menu yet.

The four places where a folder submenu appears, it currently wraps an old/current appmenu style menu popup inside of a new appmenu panel. It's ugly but at least provides the functionality until the blocking bug for that lands.

There are a few menu items not working (e.g. view > messages > save view as folder).

There's some code integration and clean up still to work out, but I think it's close to regression-free at this point. So when I have some time later today I'll rebase and upload the current/latest version of the patch, and it will be ready for review, etc.

Up-to-date version of patches. Ready for review.

Attachment #9066600 - Attachment is obsolete: true
Attachment #9069198 - Flags: review?(mkmelin+mozilla)

Patch 2 of 3.

Attachment #9066601 - Attachment is obsolete: true
Attachment #9069200 - Flags: review?(mkmelin+mozilla)

Patch 3 of 3. This is the main one. Ready for review. My next step is to work on mozmill tests, but that need not hold up review, etc.

Attachment #9069161 - Attachment is obsolete: true
Attachment #9069161 - Flags: feedback?(mkmelin+mozilla)
Attachment #9069201 - Flags: review?(mkmelin+mozilla)
Posted patch 1546309-fix-edit-button.patch (obsolete) — Splinter Review

Thank you Paul for this huge work.

This works very well, except the issues you noted. I couldn't resist and fixed the appmenu-edit-button which on hover also included the separator.

Attachment #9069219 - Flags: review?(mkmelin+mozilla)
Attachment #9069198 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
Comment on attachment 9069200 [details] [diff] [review]
part2-add-customizable-ui-files-1.patch

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

In general seems to work fine, except for Go | Folder

Other stuff:
* Folder should really be under View
* location: why mail/base/components instead of mail/components to match browser/components/customizableui/CustomizableUI.jsm (applies to other files too)
* panelUI.inc.xul etc, can we match the browser location?
* PanelMultiView.jsm - should probably also sync up some with trunk, but more changes done here so hard to easily see what
 * panelUI.js - needs some updates for toolbarbutton de-xbl at least
Did some comparisons to trunk, but then also realized the comments are two-edged, since we want to take this for 68, but trunk moved on, and both needs to work :/

::: mail/base/content/customizableui/CustomizableUI.jsm
@@ +4437,5 @@
> +      let mainViewId = multiview.getAttribute("mainViewId");
> +      let mainView = doc.getElementById(mainViewId);
> +      let contextMenu = doc.getElementById(mainView.getAttribute("context"));
> +      gELS.addSystemEventListener(contextMenu, "command", this, true);
> +      let anchor = doc.getAnonymousElementByAttribute(this._chevron, "class", "toolbarbutton-icon");

let anchor = this._chevron.icon;

(toolbarbutton de-xbl changed this)

::: mail/base/content/customizableui/CustomizeMode.jsm
@@ +176,5 @@
> +  async _updateThemeButtonIcon() {
> +    let lwthemeButton = this.$("customization-lwtheme-button");
> +    let lwthemeIcon = lwthemeButton.icon;
> +    let theme = (await AddonManager.getAddonsByTypes(["theme"])).find(addon => addon.isActive);
> +    lwthemeIcon.style.backgroundImage = theme ? "url(" + theme.iconURL + ")" : "";

lwthemeIcon.style.backgroundImage = (theme && theme.iconURL) ? "url(" + theme.iconURL + ")" : "";

also some other changes on trunk in this file that should be picked up

@@ +1340,5 @@
> +    function buildToolbarButton(aTheme) {
> +      let tbb = doc.createXULElement("toolbarbutton");
> +      tbb.theme = aTheme;
> +      tbb.setAttribute("label", aTheme.name);
> +      tbb.setAttribute("image", aTheme.iconURL);

tbb.setAttribute("image", aTheme.iconURL || "chrome://mozapps/skin/extensions/themeGeneric.svg");

like ff trunk
Comment on attachment 9069201 [details] [diff] [review]
part3-WIP-appmenu-redo-2.patch

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

Some small nits. It's of course a huge patch and difficult to review in detail, but it looks good in general!

::: mail/base/content/customizableui/CustomizableUI.jsm
@@ +7,5 @@
>  "use strict";
>  
>  var EXPORTED_SYMBOLS = ["CustomizableUI"];
>  
> +// TODO appmenu - We're not using this file as a module, so these cause "redeclaration" errors.

sounds like trouble!

::: mail/base/content/customizableui/content/panelUI.inc.xul
@@ +565,5 @@
> +      <vbox class="panel-subview-body">
> +        <toolbarbutton label="&addons.label;"
> +                       class="subviewbutton subviewbutton-iconic"
> +                       oncommand="openAddonsMgr(); event.stopPropagation();"/>
> +        <toolbarseparator class="appmenu-menuseparator"/> 

some trailing spaces here (see and in the next lines)

@@ +681,5 @@
> +    <panelview id="appMenu-fileView"
> +               title="&fileMenu.label;"
> +               class="PanelUI-subView">
> +      <vbox class="panel-subview-body">
> +        <toolbarbutton id="appmenu_openMessageFileMenuitem"  

trailing space

@@ +737,5 @@
> +    <panelview id="appMenu-fileGetNewMsgForView"
> +               title="&getNewMsgForCmd.label;"
> +               class="PanelUI-subView">
> +      <vbox class="panel-subview-body">
> +        <!-- <toolbarbutton id="appmenu_getnewmsgs_all_accounts"

what about this one? a TODO?

@@ +1388,5 @@
> +    <!-- TODO appmenu - what about this 'observes' bit?
> +        <menu id="appmenu_goRecentlyClosedTabs"
> +                  label="&goRecentlyClosedTabs.label;"
> +                  observes="cmd_undoCloseTab"> 
> +    -->

trailing space

@@ +1722,5 @@
> +                       class="subviewbutton subviewbutton-nav"
> +                       label="&imAccountsStatus.label;"
> +                       closemenu="none"
> +                       oncommand="PanelUI.showSubView('appMenu-toolsChatStatusView', this)"/>
> +   

trailing whitespace

::: mail/base/content/messageWindow.xul
@@ +113,5 @@
>    <script src="chrome://communicator/content/utilityOverlay.js"/>
>    <script src="chrome://messenger/content/mailCore.js"/>
>    <script src="chrome://messenger/content/quickFilterBar.js"/>
> +  <!-- Code for the appmenus. -->
> +  <script src="chrome://messenger/content/CustomizableUI.jsm"/>

type="module"

::: mail/extensions/mailviews/content/msgViewPickerOverlay.js
@@ +246,5 @@
>  
> +  // Create tag menu items.
> +  const currentTagKey = gFolderDisplay.view.mailViewIndex == kViewItemTags
> +    ? gFolderDisplay.view.mailViewData
> +    : "";

I think we prefer ? at the end of the line, and then the next line would be 
FolderDisplay.view.mailViewData : "";
Attachment #9069219 - Flags: review?(mkmelin+mozilla) → review+

I guess the main thing I'd like to have sorted out before landing these is making the file locations match browser/.
We probably also need to get at least the main part of test failures sorted out.

Comment on attachment 9069219 [details] [diff] [review]
1546309-fix-edit-button.patch

Paul, don't hesitate to include this patch in a next version of your patch.

Regarding basing on beta vs. trunk: perhaps it makes most sense to do a patch for beta and add a separate patch with trunk updates. Not sure which exact version the files were copied over from.

(In reply to Richard Marti (:Paenglab) from comment #40)

Created attachment 9069219 [details] [diff] [review]
1546309-fix-edit-button.patch

Thank you Paul for this huge work.

This works very well, except the issues you noted. I couldn't resist and fixed the appmenu-edit-button which on hover also included the separator.

Thank you Richard for improving this! (It was on my to-do list but I hadn't gotten to it yet.) I'll incorporate these changes in the next revisions.

Part2 revised: now moves files into the correct places. Does not yet sync up with m-c trunk, see next comment.

Attachment #9069200 - Attachment is obsolete: true
Attachment #9069200 - Flags: review?(mkmelin+mozilla)
Posted patch part3-appmenu-redo-3.patch (obsolete) — Splinter Review

Part3 revised, now:

  • with files in correct places, in parall with m-c
  • with moz.build files and JS modules as in m-c. (A bonus: using the modules as modules fixed a few TODOs.)
  • incorporating Richard's CSS changes for the edit menu button
  • addressing most review comments
  • fixes for some small things I noticed while testing

Still to do:

  • move Folders to View/Folders
  • mozmill tests
  • sync up with changes in m-c and handling needing to work on beta & trunk

For syncing in the m-c changes (now that the files are in the right place), I think I can just back up and recreate the part2 patch with the current versions of those m-c files from beta branch, then rebase the part3 patch on top of that, and fix any merge conflicts.

Then I can do another patch to catch up with changes on trunk. Hopefully that will do the trick without too much trouble.

Attachment #9069201 - Attachment is obsolete: true
Attachment #9069219 - Attachment is obsolete: true
Attachment #9069201 - Flags: review?(mkmelin+mozilla)

OK, here are patches for the 68 beta release.

The 'part1' patch applied fine to 68 beta, so no new patch for it. Here's the 'part2' patch for 68 beta, which copies over the files from FF 68 beta branch. I'll upload the 'part3' patch next. It:

  • moves Folders item from top level to under View
  • lets the mozmill tests navigate the new appmenu
    -- that fixes 4/6 test files that use the appmenu
    -- the test-mode-switching test is excluded from linux platform, but should work?
    -- the test-message-filters test depends on folder-menupopup, so it fails
Attachment #9069465 - Attachment is obsolete: true

Part 3 of 3. For 68 beta.

aleca: BTW, due to time constraints, for now, I've kept the add-ons submenu showing only addons with preferences as before. But I added a label above the list that says "Add-on Preferences". That should help prevent confusion. (That string was already there for that item under Tools menu in the main menu bar, so no string changes involved.)

High on the list of follow-ups: determine how addons interact with new appmenu, then adapt Calendar accordingly.

Attachment #9069471 - Attachment is obsolete: true
Posted image addons-menu.png

:aleca - here's a screenshot of the add-ons menu showing the "prefs" heading.

Part3 patch, with fixes for some eslint issues the try server caught, and rebased on current comm-beta. Try run for this version:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8a259f601602694cc527641554aa243f32971b07

Requesting review of the mozmill changes. (As jorgk says "first reviewer wins".)

Unfortunately, the new appmenu works differently from the other menus and the previous appmenu, so I had to add some code for navigating it. The new appmenu needed an async / event listener approach to testing it. (More details in the comments at the top of this file: https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm)

Attachment #9070329 - Attachment is obsolete: true
Attachment #9070402 - Flags: review?(geoff)
Attachment #9070402 - Flags: review?(acelists)
Comment on attachment 9070402 [details] [diff] [review]
part3-appmenu-redo-beta-6.patch

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

Drive-by comment.

::: mail/components/customizableui/content/panelUI.js
@@ +95,5 @@
> +    "ISO-8859-7",
> +    // Hebrew
> +    "windows-1255",
> +    "ISO-8859-8",
> +    // Japanese

See https://hg.mozilla.org/mozilla-central/rev/52b6ea9ab31e#l16.43.

Do we really need to copy this?

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

Do we really need to copy this?

Good question. (Already gone stale...) I copied this set of kEncodings because it is inaccessible from outside of the mozilla-central toolkit/modules/CharsetMenu.jsm module. If we could access it from there we wouldn't need our own copy of it.

Maybe we could propose adding a getter that would provide a copy of it (so the original still could not be modified)? But that would be a follow up effort since I don't think we want to delay the 68 beta.

At the least it we should make that change to the Japaneses entry, but that's a post-68 change.

I'm really not keen on the idea of introducing async stuff in Mozmill. Mozmill is so ancient it has no concept of a Promise, let alone an async function. So in each place you have async function testFoo, the test runner will start the test but not hang around waiting for a result.

To do things the Mozmill way (horrible as it is), set a variable to false and wait for it to become true, which you make it do when you're ready.

Ah, good call. I'll revise it to use the Mozmill way. I know about utils.waitFor, open to other suggestions if there's a better way. (Looking forward to using Marionette instead once that's set up.)

Part 3, now avoiding promises/async/await in mozmill tests, and doing it the mozmill way. Re-requesting review of mozmill changes.

One mozmill test is skipped because of further work needed now that the folder-menupopup de-xbl bug is done (the one this bug 'depends on').

Try server run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6d2791eeeb544b2368627800ad2e0e352ce456a7

Attachment #9070402 - Attachment is obsolete: true
Attachment #9070402 - Flags: review?(geoff)
Attachment #9070402 - Flags: review?(acelists)
Attachment #9070632 - Flags: review?(geoff)
Attachment #9070632 - Flags: review?(acelists)

While working on the folder-menupopup follow-up for this, I found a problem. In the part 3 patch I used the setBooleanAttribute helper in several places, but it's in calendar code, so when calendar is not installed, things break. Like showing and hiding the main menu bar. I didn't catch it before because I was testing with calendar installed. This should be fixed before landing.

I remember something like checking or enabling a menuitem was strangely not working but using setBooleanAttribute made it work. And it's just nice to have, so my inclination is to fix this by adding that helper to the Thunderbird utility functions.

Please don't add usage of setBooleanAttribute. It's a real pain to have these helpers IMO. I spent a bunch of time just the other day trying to track down a problem which would have been directly obvious if just written out.

OK, per mkmelin's request, this revision just removes the usages of setBooleanAttribute. Re-requesting review of the mozmill parts.

Try server run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=011645a64c9a16680d470e8eb3761dcd31e24fcb&selectedJob=250798343

Attachment #9070632 - Attachment is obsolete: true
Attachment #9070632 - Flags: review?(geoff)
Attachment #9070632 - Flags: review?(acelists)
Attachment #9070814 - Flags: review?(geoff)
Attachment #9070814 - Flags: review?(acelists)
Comment on attachment 9070814 [details] [diff] [review]
part3-appmenu-redo-beta-8.patch

R+ on just the Mozmill files. Assuming they pass, I haven't tried to run them. :-)
Attachment #9070814 - Flags: review?(geoff)
Attachment #9070814 - Flags: review?(acelists)
Attachment #9070814 - Flags: review+
Comment on attachment 9070814 [details] [diff] [review]
part3-appmenu-redo-beta-8.patch

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

::: mail/base/content/mailCore.js
@@ +326,2 @@
>  
> +  for (const toolboxId of toolboxIds) {

It is interesting that this works, as the value of the variable changes through its lifetime so how can it be declared 'const'.

::: mail/test/mozmill/folder-pane/test-folder-pane-consumers.js
@@ -32,5 @@
>  function test_virtual_folder_selection_tree() {
>    plan_for_modal_dialog("mailnews:virtualFolderProperties",
>                          subtest_create_virtual_folder);
> -  mc.click(mc.eid("button-appmenu"));
> -  mc.click_menus_in_sequence(mc.e("appmenu-popup"),

I'm not sure I like the new click_appmenu_in_sequence, behaves differently here, that it also clicks the first popup, contrary to the click_menus_in_sequence.

::: mail/test/mozmill/folder-tree-modes/test-mode-switching.js
@@ +52,5 @@
>    toggle_menu = mc.e("menu_compactFolderView");
>    toggle_appmenu = mc.e("appmenu_compactFolderView");
>  
>    modeList_menu = mc.e("menu_FolderViewsPopup");
> +  modeList_appmenu = mc.e("appMenu-foldersView");

Why is this renamed? And e.g. appmenu_compactFolderView not?

@@ +99,4 @@
>      assert_true(modeList_menu.querySelector('[value="' + baseMode + '"]').hasAttribute("checked"));
>      mc.close_popup_sequence(popuplist);
>    }
> +  // TODO appmenu

What is missing?

@@ +111,3 @@
>   */
> +function select_mode_in_menu(mode) {
> +  // TODO appmenu

What is missing?

::: mail/test/mozmill/folder-widget/test-message-filters.js
@@ +90,5 @@
>     * Get the getAllNewMessages menu and check the number of items.
>     */
>    function check_getAllNewMsgMenu() {
>      wait_for_window_focused(mc.window);
> +    // TODO appmenu - depends on folder-menupopup

What needs to be done here? folder-menupopup is already converted.

@@ -100,3 @@
>                    "Incorrect number of items for GetNewMessages before customization");
> -
> -    mc.close_popup_sequence(popups);

Will the children exist when the menu is already closed? Is that safe to assume?

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +1066,5 @@
> +     *                                 to identify a final menu item to click.
> +     * @return {Element}  The <vbox class="panel-subview-body"> element inside
> +     *                    the last shown <panelview>.
> +     */
> +    click_appmenu_in_sequence: function _click_appmenus(appmenuButton, navTargets, nonNavTarget) {

You do not need to add the second function name "_click_appmenus" anymore.

@@ +1091,5 @@
> +          // Some views are dynamically populated after ViewShown, so we wait.
> +          utils.waitFor(
> +            () => kids.find(findFunction),
> +            () => "Waited but did not find matching menu item for target: " +
> +                  JSON.stringify(clickTarget));

The second argument to waitFor shouldn't be a function. What is this doing?

@@ +1124,5 @@
> +                      () => ("Popup never opened! id=" + rootPopup.id +
> +                            ", state=" + rootPopup.state));
> +      }
> +
> +      if (!done) {

You do not need this check.

(In reply to :aceman from comment #62)

  • for (const toolboxId of toolboxIds) {

It is interesting that this works, as the value of the variable changes
through its lifetime so how can it be declared 'const'.

It's my understanding that with for...of the variable's scope and lifetime is the {...} block that is one iteration of the loop. So const works. (I suppose with let the variable might be re-assigned rather than re-declared with each iteration, as an implementation detail?)

I'm not sure I like the new click_appmenu_in_sequence, behaves differently
here, that it also clicks the first popup, contrary to the
click_menus_in_sequence.

Hm, this way it makes for a cleaner implementation because you can attach the same event listener for the initial click. The other way that click has already happened so you have to manually call the event listener function with a faked event object {target: theClickTarget} to start the series of event listeners. Also it requires less code at the call sites.

But, on second thought, I you're probably right that more consistency in calling these two functions is worth more than having a more elegant implementation. I'll change it.

::: mail/test/mozmill/folder-tree-modes/test-mode-switching.js
@@ +52,5 @@

toggle_menu = mc.e("menu_compactFolderView");
toggle_appmenu = mc.e("appmenu_compactFolderView");

modeList_menu = mc.e("menu_FolderViewsPopup");

  • modeList_appmenu = mc.e("appMenu-foldersView");

Why is this renamed? And e.g. appmenu_compactFolderView not?

A long story short, the new appmenu is implemented differently than the old one. So I made the ids of <panelview> elements xxxxView rather than xxxxPopup to match the elements they are used on. (A panelview is not a popup.) modeList_appmenu / #appMenu-foldersView is now a <panelview> so it now has a xxxxView id.

toggle_appmenu / #appmenu_compactFolderView is not a popup/panelview but a menu item, or now a <toolbarbutton>. Changing these menu item/button ids is not a good idea because they are used by other code to enable/disable, check/uncheck etc. these items, so that code would have to change too and this huge patch would have to get even bigger. (Unfortunately, this id was already using a xxxxView name for a menu item / button, so that just adds to the confusion.)

In an ideal world...

@@ +99,4 @@

 assert_true(modeList_menu.querySelector('[value="' + baseMode + '"]').hasAttribute("checked"));
 mc.close_popup_sequence(popuplist);

}

  • // TODO appmenu

What is missing?

Thanks for the catch. I'll remove these reminder comments. I couldn't run this test on linux because the test is set to be skipped on linux. These were a reminder to me to see if they passed on the try server run. But it seems this test is an intermittent fail on other platforms:
https://bugzilla.mozilla.org/show_bug.cgi?id=1490639

@@ +111,3 @@

*/
+function select_mode_in_menu(mode) {

  • // TODO appmenu

What is missing?

Same as above, removed.

::: mail/test/mozmill/folder-widget/test-message-filters.js
@@ +90,5 @@

* Get the getAllNewMessages menu and check the number of items.
*/

function check_getAllNewMsgMenu() {
wait_for_window_focused(mc.window);

  • // TODO appmenu - depends on folder-menupopup

What needs to be done here? folder-menupopup is already converted.

Now that it's converted, the folder-menupopup code needs to be adapted for use with the new appmenu. That's the first follow up to do after this bug. I'll expand the comment to explain that.

@@ -100,3 @@

               "Incorrect number of items for GetNewMessages before customization");
  • mc.close_popup_sequence(popups);

Will the children exist when the menu is already closed? Is that safe to
assume?

I did not work on getting this test to run and pass yet because of the pending folder-menupopup work that it depends on (to work at all). Looking at it now, we would do something like this: (I've just changed the code to this.)

  function check_getAllNewMsgMenu() {
    wait_for_window_focused(mc.window);
    const subview = mc.click_appmenu_in_sequence(mc.eid("button-appmenu"),
                                                 [ {id: "appmenu_File"},
                                                   {id: "appmenu_getNewMsgFor"} ]);
    assert_equals(subview.children.length, 5,
                  "Incorrect number of items for GetNewMessages before customization");

    // TODO appmenu - Now click somewhere that causes the appmenu to close.
  }

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +1066,5 @@

  • *                                 to identify a final menu item to click.
    
  • * @return {Element}  The <vbox class="panel-subview-body"> element inside
    
  • *                    the last shown <panelview>.
    
  • */
    
  • click_appmenu_in_sequence: function _click_appmenus(appmenuButton, navTargets, nonNavTarget) {

You do not need to add the second function name "_click_appmenus" anymore.

OK, removed.

@@ +1091,5 @@

  •      // Some views are dynamically populated after ViewShown, so we wait.
    
  •      utils.waitFor(
    
  •        () => kids.find(findFunction),
    
  •        () => "Waited but did not find matching menu item for target: " +
    
  •              JSON.stringify(clickTarget));
    

The second argument to waitFor shouldn't be a function. What is this doing?

This tells you what target it was looking for when it timed out. The second argument to waitFor can be a function that returns a string, allowing you to do this kind of thing. Like here:

    click_menus_in_sequence: function _click_menus(aRootPopup, aActions, aKeepOpen) {
      if (aRootPopup.state != "open") { // handle "showing"
        utils.waitFor(() => aRootPopup.state == "open",
                      () => ("Popup never opened! id=" + aRootPopup.id +
                             ", state=" + aRootPopup.state));
      }

@@ +1124,5 @@

  •                  () => ("Popup never opened! id=" + rootPopup.id +
    
  •                        ", state=" + rootPopup.state));
    
  •  }
    
  •  if (!done) {
    

You do not need this check.

Yeah, probably an over-optimization. Removed.

New version of the patch on the way soon.

This revision addresses aceman's review comments, as described above. Affected mozmill tests pass locally (one omitted for linux platform and one skipped pending follow-up work). New try server run coming up.

Attachment #9070814 - Attachment is obsolete: true
Attachment #9071015 - Flags: review?(acelists)

mozmill/folder-tree-modes/test-mode-switching.js | test-mode-switching.js::test_toggling_modes failed everywhere now.

EXCEPTION: Waited but did not find matching menu item for target: {"id":"appmenu_FolderViews"}
Looks like a simple oversight (I hope).

I know nothing about this, but it looks like appmenu_FolderViews being on appMenu-viewView and not appMenu-mainView where the test is looking for it. I may be wrong ...

By request, an up-to-date screenshot of the new appmenu.

Thanks Jorg K. You're were right. I forgot to update the test when I moved the 'Folders' button/item from the top level to under the 'View' menu (when addressing code review). And because the test was omitted on Linux I didn't catch it earlier.

However, when I fixed that, the try run had different errors on that same test (Z3):
"EXCEPTION: Waited for the appmenu to open, but it never opened."
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=89e7b68d7730ed69bbdcfc55c15ab1139df9ad7a

I tried again with some changes, but got the same result:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=33483decc1050b32a554561437aea065f15a1276&selectedJob=251083453

Just now I tried running it locally on Linux and see that the appmenu does indeed open. I'll keep at it...

Meanwhile wsmwk asked me if we could go ahead and land this for the beta. I think we should mark the 'test-mode-switching' mozmill test as disabled/skipped and fix it in a follow up, in order to move forward with the beta. I'll prepare a patch that disables that test.

There's also a review of the mozmill code pending from aceman, but he's reviewed it once and I've made the changes he suggested, and it's only about tests, not the menu itself. So any further changes needed could be done in a follow-up.

I'll prepare the patches for landing.

Rebased on current comm-beta. Here's part 1 of 3, ready to check in.

Attachment #9069198 - Attachment is obsolete: true

Rebased on current comm-beta. Part 2 of 3 ready for checkin.

Attachment #9070327 - Attachment is obsolete: true

Rebased on current comm-beta. Part 3 of 3 ready for checkin.

Fixes one part of the test-mode-switching mozmill test (see above), but skips the test for now since it's still failing.

Attachment #9071015 - Attachment is obsolete: true
Attachment #9071015 - Flags: review?(acelists)
Comment on attachment 9071276 [details] [diff] [review]
part3-appmenu-redo-beta-10.patch

I accidentally removed the review request (aceman).  Adding it back.
Attachment #9071276 - Flags: review?(acelists)
Keywords: checkin-needed

I've now gotten test-mode-switching working on Linux locally. Lets see how try server run goes, and I'll make another patch for checkin if all's well:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9f9376ed56ae5b18a7b6a5f58f6252d6c99b90da

Also, if all goes well we can stop excluding this test on Linux. I've included it on Linux for the try run. (Was excluded recently here: https://bugzilla.mozilla.org/show_bug.cgi?id=1533085)

Thanks, Paul. The reason the "mode switching" test was disabled on Linux is that it frequently failed for no apparent reason, see bug 1490639. Bug 1533085 just had some noise that switched it back from totally disabled to disabled on Linux:
https://hg.mozilla.org/comm-central/rev/b644445a3bf97c3b2b8bd86bb17c3a00a5f758b8#l5.28
... which was the state after bug 1490639.

If it works locally, there is a chance it will still fail in automation, but only on Linux.

You have a winner!

Thanks jorgk for explanation. The test is passing on try, so here's a patch with the fix for the test. Ready to go.

Attachment #9071276 - Attachment is obsolete: true
Attachment #9071276 - Flags: review?(acelists)
Attachment #9071325 - Flags: review?(acelists)

Checkin-needed is for trunk landings only.

Keywords: checkin-needed

One more revision. This just uses the new de-xbl / custom element syntax for the 'legacy' menupopups that are in there temporarily for the folder/account submenus. Instead of type="folder" use is="folder-menupopup". (It's not pretty but this way the functionality is still there.)

Attachment #9071325 - Attachment is obsolete: true
Attachment #9071325 - Flags: review?(acelists)
Attachment #9071337 - Flags: review?(acelists)
Blocks: 1558565
Blocks: 1558572
Blocks: 1558599
Comment on attachment 9071337 [details] [diff] [review]
part3-appmenu-redo-beta-12.patch

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

Thanks for all the menu rework. I'm not a fan of the direction Firefox is going, but we can't reject their changes at this time.

::: mail/base/content/mailCore.js
@@ +372,1 @@
>            Services.xulStore.persist(toolbar, hidingAttribute);

Are you sure this will persist anything if the attribute is removed above? Wasn't it intentionally set to "true"/"false" explicitly?

::: mail/base/content/mailWindowOverlay.js
@@ +1087,1 @@
>                                             .getString("restoreAllTabs"));

Indent?

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +1084,5 @@
> +     *     attribute->value pairs. We pick the menu item whose DOM node matches
> +     *     all the attribute->value pairs. We click whatever we find. We throw
> +     *     if the element being asked for is not found.
> +     * @param {Object} [nonNavTarget]  Contains attribute->value pairs used
> +     *                                 to identify a final menu item to click.

Why is this final element as a separate argument? Is it for the case where the menu should stay open so you can inspect the children, e.g. at test-addons-mgr.js ?

@@ +1089,5 @@
> +     * @return {Element}  The <vbox class="panel-subview-body"> element inside
> +     *                    the last shown <panelview>.
> +     */
> +    click_appmenu_in_sequence(mainView, navTargets, nonNavTarget) {
> +      const rootPopup = this.e("appMenu-popup");

Are there different 'mainView' elements, that can be passed in? But then you get a hardcoded "appMenu-popup". If it always can only be "appMenu-mainView", the argument could be dropped.

@@ +1137,5 @@
> +      utils.waitFor(() => rootPopup.getAttribute("panelopen") == "true",
> +        "Waited for the appmenu to open, but it never opened.");
> +
> +      // Because the appmenu button has already been clicked in the calling
> +      // code (to match click_menus_in_sequence), we have to call the first

Thanks. I wanted to have the possibility in case we can open the appmenu with a hotkey or other key presses, instead of a click of the button.

But I am not opposed to a wrapper function too (that calls mc.click(mc.eid("button-appmenu")); and then click_appmenu_in_sequence() with the passed in arguments, if that is the most common thing we do in the tests. We supposedly should increase the use of appmenu in tests so any helpers and code-line reducers would be handy.

::: mail/themes/shared/customizableui/panelUI.css
@@ +50,5 @@
>      transparent calc(100% - 4px)
>    );
>  }
>  
> +/* TODO appmenu - added, more needed? */

Why the TODOs? Are these copied from m-c?

::: mail/themes/windows/mail/primaryToolbar.css
@@ +210,5 @@
>  }
>  
>  /* AppMenu styling */
>  
> +/* TODO appmenu - remove? needed? */

Do we know an answer to this?
Attachment #9071337 - Flags: review?(acelists) → review+
Comment on attachment 9071337 [details] [diff] [review]
part3-appmenu-redo-beta-12.patch

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

::: mail/components/customizableui/content/panelUI.inc.xul
@@ +699,5 @@
> +              label="&getNewMsgForCmd.label;"
> +              oncommand="MsgGetMessagesForAccount();">
> +          <menupopup is="folder-menupopup"
> +                      mode="getMail"
> +                      id="appmenu_getAllNewMsgPopup"

You need to put 'id' on the same line as 'is' attribute, as it was in the original file.

Thanks, Aceman. Looks like all/most(?) comments won't affect the functioning and we can ship "as is", right?

Yes, all the proposed changes except the "Services.xulStore.persist(toolbar, hidingAttribute);" question will not change behaviour.

My only comment is as a user. There are some things which need to be easily and quickly accessible for frequent use. Then, there are the items which are not used much, but which should be able to be easily found. These need to somehow "live" together; or not. In Firefox, there are more things which are just set and left alone, but in Thunderbird/Calendar, there is more interaction. Good luck! See you when some test versions come out.

(In reply to :aceman from comment #82)

Comment on attachment 9071337 [details] [diff] [review]
part3-appmenu-redo-beta-12.patch

::: mail/base/content/mailCore.js
@@ +372,1 @@

       Services.xulStore.persist(toolbar, hidingAttribute);

Are you sure this will persist anything if the attribute is removed above?
Wasn't it intentionally set to "true"/"false" explicitly?

I hadn't looked into this before since this line was already there and not part of the code I was changing. I just checked the source for the persist method and it looks like it will do what is needed here:
https://searchfox.org/mozilla-central/source/toolkit/components/xulstore/new/XULStore.jsm#49

A toolbar's hidden attribute should either be present and set to "true" or not be present (if the toolbar is not hidden). And the code will effectively persist the equivalent of either of these two states: hidden=true or no hidden attribute persisted.

The backstory is that when I was testing the appmenu, the toolbar wasn't being hidden when the code was setting hidden="false". I had to change it to remove the hidden attribute to get it to work.

(This is why Calendar uses a setBooleanAttribute helper which converts a false value into a removal of the attribute. I was going to use such a helper here, but mkmelin asked me not to do that. That means we are stuck with working directly with the DOM and its APIs for better or worse.)

::: mail/base/content/mailWindowOverlay.js
@@ +1087,1 @@
Indent?

Fixed.

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +1084,5 @@
Why is this final element as a separate argument? Is it for the case where
the menu should stay open so you can inspect the children, e.g. at
test-addons-mgr.js ?

Yes, that's it. It's explained in the function's doc string. You supply a series of click targets to navigate the menu, then if you want to click an item that does a command you supply that as a separate optional argument. If you don't, then the menu just stays open.

For navigation clicks we have to attach an event listener (to know when the navigation is done), but we don't want to do that for a final non-navigation click.

@@ +1089,5 @@
Are there different 'mainView' elements, that can be passed in? But then you
get a hardcoded "appMenu-popup". If it always can only be
"appMenu-mainView", the argument could be dropped.

Hm, yeah, it's always the same 'mainView' element, so I'll just drop that argument.

@@ +1137,5 @@
Thanks. I wanted to have the possibility in case we can open the appmenu
with a hotkey or other key presses, instead of a click of the button.

Good idea. One advantage of the new appmenu is better support for keyboard navigation, so we'll eventually want a way to test that throughout as well as the initial activation.

But I am not opposed to a wrapper function too (that calls
mc.click(mc.eid("button-appmenu")); and then click_appmenu_in_sequence()
with the passed in arguments, if that is the most common thing we do in the
tests. We supposedly should increase the use of appmenu in tests so any
helpers and code-line reducers would be handy.

I like the wrapper function idea. I'll add one and use it. (So far we're only testing with the main mail appmenu button, but there are others -- chat tab, calendar, etc.)

::: mail/themes/shared/customizableui/panelUI.css
@@ +50,5 @@

+/* TODO appmenu - added, more needed? */

Why the TODOs? Are these copied from m-c?

Not copied. I had added these as reminders to myself, but this was holding up the release timeline and so there wasn't time to address all of them. So I left them in, to deal with in a follow-up. (Not ideal, I know.) This one could probably just be dropped.

::: mail/themes/windows/mail/primaryToolbar.css
@@ +210,5 @@

}

/* AppMenu styling */

+/* TODO appmenu - remove? needed? */

Do we know an answer to this?

I haven't had time to look at exactly which CSS we're using and which we aren't. Magnus suggested erring on the side of leaving things in rather than deleting them, since we're copying this over from FF and will probably want to incorporate future upstream changes. (Again, not ideal.) So something else that has been deferred to follow-up. (Would be nice to have a compiler or some static analysis tool that could identify unused CSS.)

A couple more patches for beta (part 4 and 5). This part 4 patch fixes an oversight I caught while working on the patches to get this to work on trunk. I think these must have slipped back in when I was rebasing on beta.

Changes shouldn't affect functionality or appearance. It's just adapting the FF CSS file for TB use where we don't use a pre-processor step.

Attachment #9071704 - Flags: review?(richard.marti)

Part 5 patch addresses aceman's second code review, as described above. I did a separate patch for the changes since the one reviewed is already checked in for the 68 beta.

Try server run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bbe39cf06492f3c89afb5278b47ccc181ccb9bfa

These two patches are for beta. For trunk, I've rebased the beta patches (parts 1-5) and made some additional changes (parts 6 and 7). I'll upload all of those next.

Attachment #9071705 - Flags: review?(acelists)

Part 1 for trunk.

Part 2 for trunk.

Part 3 for trunk.

Part 4 for trunk.

Part 5 for trunk.

Part 6 for trunk. Simple removal of the toolbarbutton-appmenu custom element which isn't needed with the new appmenu.

Attachment #9071714 - Flags: review?(mkmelin+mozilla)

Part 7 for trunk. Copies over the recent (post-beta) changes from FF code.

Attachment #9071715 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9071704 [details] [diff] [review]
part4-fix-css-preprocessor-entries-0.patch

Thanks.
Attachment #9071704 - Flags: review?(richard.marti) → review+
Comment on attachment 9071705 [details] [diff] [review]
part5-aceman-code-review-0.patch

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

Thanks!

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +1147,5 @@
>        return subviewToReturn;
>      },
>  
>      /**
> +     * Utility wrapper function for click_appmenu_in_sequence.

More descriptive comment please.
Attachment #9071705 - Flags: review?(acelists) → review+
Comment on attachment 9071714 [details] [diff] [review]
trunk-part6-remove-toolbarbutton-appmenu-0.patch

Looks reasonable, thanks.
Attachment #9071714 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9071715 [details] [diff] [review]
trunk-part7-copy-new-changes-from-ff-0.patch

If that's a straight copy, we just take as it is, assuming that it works.
Attachment #9071715 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9071711 [details] [diff] [review]
trunk-part3-appmenu-redo-0.patch

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

::: mail/components/customizableui/content/panelUI.js
@@ +94,5 @@
> +    "ISO-8859-7",
> +    // Hebrew
> +    "windows-1255",
> +    "ISO-8859-8",
> +    // Japanese

Sorry, but that's not right for trunk as discussed previously.

OK, let's bring this to a close.

  1. Add the comment Aceman wants for the beta patch, then I'll land the two patches.
  2. Fix trunk part 3 for Japanese, also add the comment from 1.
  3. Do a try run with those seven patches, if green, I'll land.

Now with better doc for the wrapper function. Ready to land on beta. More to follow for trunk.

Attachment #9071705 - Attachment is obsolete: true

Thanks for the quick reviews. Here come the trunk patches, rebased on current trunk, ready to land, if try server run is green enough.

  • Added the improved function doc string to part 5 to match beta.

  • Put the Japanese encoding change in part 7 so that part 3 would remain just a rebase of the same patch from beta. (Thanks for the reminder about this.)

Try server run looks good to me, one failing test without a bug (Z2 on Linux debug), but it seems unrelated.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b42f748fea828f702806baa4e0aa0371a5e1077d

Attachment #9071709 - Attachment is obsolete: true
Attachment #9071710 - Attachment is obsolete: true
Attachment #9071711 - Attachment is obsolete: true
Attachment #9071713 - Attachment is obsolete: true
Attachment #9071715 - Attachment is obsolete: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/534295374182
Remove extra unused arguments from refresh functions. r=mkmelin
https://hg.mozilla.org/comm-central/rev/fe4c6acd6880
Add customizable UI files. rs=jorgk
https://hg.mozilla.org/comm-central/rev/bc8ef4b819df
[de-xbl] Get rid of splitmenu, and rework the appmenu. r=darktrojan,aceman (in parts), rs=jorgk
https://hg.mozilla.org/comm-central/rev/c8cac88bbe30
Fix some pre-processor entries in panelUI.css. r=Paenglab
https://hg.mozilla.org/comm-central/rev/4ad0ea172c05
appmenu: follow-up on aceman code review. r=aceman
https://hg.mozilla.org/comm-central/rev/b9426afd27bf
appmenu: remove the toolbarbutton-appmenu custom element. r=jorgk
https://hg.mozilla.org/comm-central/rev/c358a56e279b
appmenu: copy some customizableui related changes from Firefox trunk. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED

So we're finally done here, or are we? Looks like part 3, "[de-xbl] Get rid of splitmenu, and rework the appmenu" hasn't been fully reviewed. If I see it correctly, only the Mozmill parts were looked at thoroughly. There are many TODOs left, and some chunks are commented out, like here:
https://hg.mozilla.org/comm-central/rev/bc8ef4b819df#l14.584

We should address all this in a follow-up bug.

Target Milestone: --- → Thunderbird 69.0
Blocks: 1559127

Indeed, there is more to do, as you described. Bug 1558565, then bug 1558599, then this follow-up I just created for the remaining TODOs etc., bug 1559127.

Just an opinion for what it's worth....

It maybe just me but the hamburger seems to be only activated when you left click. I've not found out how to access it via the keyboard eg: tab key or menukey or any other shortcut.
For some people, when Thunderbird is initially setup, this makes TB difficult to use because there is no apparantly visible method of access to menus. So if there is a method of access to the 'Menu icon/hamburger' eg: tab or shortcut key can someone please enlighten me, but if there is no access then could be be considered and implemented before next version is rolled out.

I would like to see Thunderbird developments in design, move towards inclusivity for those with limited abilities eg: need to use keyboard to access all tools etc and less need to perform more clicks (reducing repetitive stress issues). The new ideas do not seem to be taking these issues into consideration.
Ideally the Menu Bar should be, by default, enabled.

Will the menu icon/hamburger be able to be selected using keyboard 'tab' key and then opened to see contents using the keyboard 'menukey' ?

Hovering as used in current design is quicker to reveal various options especially when you are not familiar or do not exactly know where something is located and less uses less clicks. The new design appears to use more clicks (It certainly does in FF) which can contribute to repetitive stress disorder. This issue is more likely to be of importance in an email client than in a browser. So a design working in a browser does not mean it would work as well in an email client.

In what way does the new design improve work throughput or not make it worse for RSD issues, when additional manual processes are involved?

You need to log in before you can comment on or make changes to this bug.