Closed Bug 511625 Opened 10 years ago Closed 5 years ago

move other actions button in line with header buttons (chevron like)[polish]

Categories

(Thunderbird :: Message Reader UI, enhancement)

enhancement
Not set

Tracking

(thunderbird36 fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird36 --- fixed

People

(Reporter: ovidiu.grigorescu, Assigned: Paenglab)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ux-feature-wanted-38])

Attachments

(2 files, 6 obsolete files)

"other actions" header button should be inline with the other header buttons. Like a chevron or [+] or something 
It would free the space under for extensions or others (Bug 418024)
Would be closer to a header toolbar with customizable buttons idea.

See it in bug466025 as extension or respective captures

[This is more interesting in the vertical 3 pane.]
A more radical approach is that proposed by the bug #511491 (that I flag as [driver: WONTFIX?]).
I think that this is now invalid because bug #511491 is now assigned.

Feel free to reopen it if I'm wrong.
The fact that that bug is assigned means that a developer has agreed to work on the bug, but it doesn't say anything about when or if that bug is going to be fixed.  In that particular case, what happens with bug 511491 is not yet decided.

I'm intrigued by the proposal in this bug and will think about it as I'm working on this code in the upcoming days.
the dropdown should be a chevron. that would make it right.

this is a capture of an actual extension, part of the one in bug 466025.
I like the idea but I'm concerned about the added horizontal space being taken up by having the button up there.  The trash is just an icon because of concerns of using too much space.  I can easily agree that we could change the button as it is now into an icon plus chevron or even just a chevron however leaving it where it is.

Maybe I don't understand the merit for extension developers.  Perhaps we should make this an easier option to have the other actions in the button box or not.  If we make both buttons just a chevron it will be easier either way.

I'm not sure what the best compromise would be, perhaps I don't understand enough of what you are asking.  Your header extension already has the button up there.  Are you just trying to hide the existing 'other actions' button?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 513553
Providing a choice between icons or text is covered by bug 505169 and would
be resolved with bug 465138 making that a true toolbar. The combination of
bug 499410 with bug 514452 would distribute the header items better between
left and right boundaries as a vertical/horizontal trade-off. Putting the
"other actions" box onto the top row would not fit well with those efforts
and possibly truncate the sender information if text buttons are used and the window is not fully expanded (or a small monitor is used).
The CompactHeader add-on (https://addons.mozilla.org/de/thunderbird/addon/13564/) replaces the other actions menu by a button in the header pane toolbar.
herb, was that part of your fix for bug 519956?
(In reply to comment #8)
> herb, was that part of your fix for bug 519956?
No, the button is part of the CompactHeader add-on. Of course, the code for the button could be ported from the add-on into Thunderbird itself. 

Actually at the moment the menu is moved around using javascript, whereas it would be just xul code in Thunderbird itself:

    var target = "hdrOtherActionsButton";
    
    var newParent = document.getElementById(target) || 
        document.getElementById("header-view-toolbox").
                 palette.getElementsByAttribute("id", target)[0];
  
    if (newParent != null) {
      var myElement;
      myElement= document.getElementById("otherActionsPopup");
      if (myElement) {
        newParent.appendChild(myElement);
      }
    }
(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #3)

> I'm intrigued by the proposal in this bug

+1

After all these years, of course we've started to overlook this oddity somehow, but imho the "Other Actions" button is still an ugly alien and misplaced creature on the header, which should be tamed into a dropdown menu button on the right side of the other header buttons (we could even try using ≡ icon for that).

Also, every action from "Other actions" should be available for the customization palette of header bar, and probably items removed from header bar should automatically go into the "Other actions" menu. That would be intelligent customization, similar to how Australis customization works.
Blocks: 1074890
Attached patch Move it (obsolete) — Splinter Review
This patch moves the otherActionsButton to the header-view-toolbar. On the toolbar there is no icon shown, only the dropdown-marker and if not in icons only mode also the text.

This patch needs bug 1056649 applied first to allow the migration.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8500105 - Flags: ui-review?(josiah)
Attachment #8500105 - Flags: review?(josiah)
Comment on attachment 8500105 [details] [diff] [review]
Move it

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

So I like this in theory, but there are a few issues that need to be resolved before I accept this. First, the dropdown icon lacks a retina version, which looks quite poor on my machine. I don't think we have one yet, but we should get one. (It's a triangle, it can't be that hard).

Second, I'm not a fan of how small the dropmarker is compared to the other icons, it looks a little out of place.

Third, the triangle changes direction when you switch to customize mode, and that I am not too keen on that.
Attachment #8500105 - Flags: ui-review?(josiah)
Attachment #8500105 - Flags: review?(josiah)
Attached patch otherActionsMove.patch (obsolete) — Splinter Review
I've added a icon instead of the dropmarker to OS X only now. If this has f+ I add them to Linux (not sure if native or own icon, I have to check) and Windows.
Attachment #8500105 - Attachment is obsolete: true
Attachment #8507001 - Flags: feedback?(josiah)
Attached patch otherActionsMove.patch (obsolete) — Splinter Review
Like the previous patch adding a icon but now for all platforms. I renamed the button from "Other Actions" to "More". This is to make the button smaller and not to be the widest of all buttons (especially on other languages).
Attachment #8507001 - Attachment is obsolete: true
Attachment #8507001 - Flags: feedback?(josiah)
Attachment #8507371 - Flags: review?(josiah)
Whiteboard: [ux-feature-wanted-38]
Comment on attachment 8507371 [details] [diff] [review]
otherActionsMove.patch

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

This looks pretty good to me, except for the graphics. I think I would prefer the larger dropdown icon (In the More Options button) to be a normal dropmarker, and we use the v and ^ in the toolbar for next and previous. That way the dropmarkers all consistent across the application. Other than that it looks good.

::: mail/base/modules/mailMigrator.js
@@ +239,5 @@
>      } catch(e) {
>        Cu.reportError("Migrating from UI version " + currentUIVersion + " to "
>                       + UI_VERSION + " failed. Error message was: " + e + " -- "
>                       + "Will reattempt on next start.");
>      } 

Trailing whitespace, do you think you could remove that?
Attachment #8507371 - Flags: ui-review-
Attachment #8507371 - Flags: review?(josiah)
Attachment #8507371 - Flags: review+
Attached patch otherActionsMove.patch (obsolete) — Splinter Review
Now using again the normal dropmarker - also in customize window. Josiah, please can you test it on OS X? I haven't added a @2x image in customize as this is only a short time visible and not so important.

The v and ^ in the toolbar for next and previous should be done in a different bug. We need also to look what we want to do with the 'next unread' and 'previous unread' icons.
Attachment #8507371 - Attachment is obsolete: true
Attachment #8510905 - Flags: review?(josiah)
Comment on attachment 8510905 [details] [diff] [review]
otherActionsMove.patch

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

Okay, cool. Yes, the drop marker looks poor right now, but we'll change that in a followup bug.
Attachment #8510905 - Flags: review?(josiah) → review+
Attached patch otherActionsMove.patch (obsolete) — Splinter Review
Changed the variable name in mailMigrator.js to be consistent with the bug 1056649 changes.
Attachment #8510905 - Attachment is obsolete: true
Attachment #8511627 - Flags: review+
Keywords: checkin-needed
Attached patch otherActionsMove.patch (obsolete) — Splinter Review
Unbitrot after a followup fix for bug 1056649.
Attachment #8511627 - Attachment is obsolete: true
Attachment #8512124 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Could this change be the cause for the mozmill test failure?

> TEST-START | C:\slave\test\build\mozmill\message-header\test-header-toolbar.js | test_customize_header_toolbar_change_button_style
> Step Pass: {"function": "controller.click()"}
> Step Pass: {"function": "controller.click()"}
> Step Pass: {"function": "controller.click()"}
> Test Failure: a != b: 'none' != '-moz-box'.
> TEST-UNEXPECTED-FAIL | C:\slave\test\build\mozmill\message-header\test-header-toolbar.js | test-header-toolbar.js::test_customize_header_toolbar_change_button_style

https://tbpl.mozilla.org/php/getParsedLog.php?id=51718982&tree=Thunderbird-Trunk#error0
Yes this can be. The other actions button doesn't show a icon in "Icon besides Text" mode. I'm a beginner in JS. How could the other actions button excluded from the tests in subtest_buttons_style function?
https://tbpl.mozilla.org/php/getParsedLog.php?id=51775962&tree=Thunderbird-Trunk&full=1#error2

Backed out for orange - https://hg.mozilla.org/comm-central/rev/9bc5d462c145
I verified tests work locally with this backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I *think* this will work. Replace:

  for (let i = 0; i < currentSet.length; i++) {
    // XXX For the moment only consider normal toolbar buttons.
    // XXX Handling of toolbaritem buttons has to be added later,
    // XXX especially the smart reply button!
    if (mc.e(currentSet[i]).tagName == "toolbarbutton") {
      let icon = mc.a(currentSet[i], {class: "toolbarbutton-icon"});
      let label = mc.a(currentSet[i], {class: "toolbarbutton-text"});
      assert_equals(mc.window.getComputedStyle(icon).getPropertyValue("display"), aIconVisibility);
      assert_equals(mc.window.getComputedStyle(label).getPropertyValue("display"), aLabelVisibility);
    }
  }

with

  for (let i = 0; i < currentSet.length; i++) {
    // XXX For the moment only consider normal toolbar buttons.
    // XXX Handling of toolbaritem buttons has to be added later,
    // XXX especially the smart reply button!

    let currentElement = mc.e(currentSet[i]);

    if (currentElement.tagName == "toolbarbutton" && currentElement.id != "otherActionsButton") {
      let icon = mc.a(currentSet[i], {class: "toolbarbutton-icon"});
      let label = mc.a(currentSet[i], {class: "toolbarbutton-text"});
      assert_equals(mc.window.getComputedStyle(icon).getPropertyValue("display"), aIconVisibility);
      assert_equals(mc.window.getComputedStyle(label).getPropertyValue("display"), aLabelVisibility);
    }
  }

in subtest_buttons_style
Magnus, please could you only review the test.

It passed on try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=7b60928746d7

Josiah, thank you for the test code.
Attachment #8512124 - Attachment is obsolete: true
Attachment #8516977 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8516977 [details] [diff] [review]
otherActionsMove.patch with test fix

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

Yup, the test passes now. r=mkmelin
Attachment #8516977 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/bd598ed6d5d2
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 1113610
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.