Closed Bug 1691297 Opened 2 years ago Closed 1 year ago

Mail Toolbar too high as of 78.3.1 when using "Icons and Text"

Categories

(Thunderbird :: Theme, defect)

defect

Tracking

(thunderbird_esr78 fixed, thunderbird89 fixed)

RESOLVED FIXED
90 Branch
Tracking Status
thunderbird_esr78 --- fixed
thunderbird89 --- fixed

People

(Reporter: dannyfox, Assigned: Paenglab)

References

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

Activate Mail Toolbar under VIEW TOOLBARS, and select "Icons & Text" so the labels display under the respective icons.
Compare main screen of current TB (78.3 and later) with 78.2.2 and earlier.

Actual results:

Following upgrade to 78.3.1 (standard user release, via HELP/ABOUT), the Mail Toolbar was little bit taller. Not such a big deal on my PC screen, but QUITE SIGNIFICANT on the laptop or other small screens!

Expected results:

Toolbar height should not have been changed on a whim -- I am not aware of any issues with previous height. (And it's not always desirable for users to implement the obvious work-around which is to use "Icons Only"...)

Component: Untriaged → Theme
Summary: Mail Toolbar too high → Mail Toolbar too high as of 78.3.1

Following upgrade to 78.3.1

Hi, that version is five months old and not supported anymore. Please upgrade to a supported version (currently: 78.7.1) and report back.

Flags: needinfo?(dannyfox)

Thanks Andre, but I did say "78.3.1 and later" in my initial report. Now on current user release 78.7.1 (prior to posting this bug) and the line is still the same -- a little higher than it needs to be. In fact, looking more closely at my notepad, the problem first showed up in 78.2.2 -- along with several other glitches which I reported at the same time as THIS report -- and all carried through to 78.3.1 and to today. None of these appear to be fixed so far, and I have a couple more to report which I noticed at the time and are still outstanding.

So for reporting back: The toolbar height is still a little high, even in current user release 78.7.1.
Steps to reproduce: Compare main screen of recent TBs (78.2.2 and later) against 78.2.1 and earlier.

Flags: needinfo?(dannyfox)
Severity: -- → S4

Please can you add screenshots of before and after?

Sorry, I don't have the old version. But the current toolbar is about 1/8" (3mm) higher on a screen that's 11 1/2" (29cm) high. So I lose about a line of text (one message) from my display window.

The laptop menu bar is set to "text beside icons", possible only because ours has fewer buttons. Otherwise the impact is significant because it would be at least one message line lost from a window pane that is already on the short side.

I didn't look at Windows. On Mac the toolbar of the newer version is maybe 1/8 higher. Not seeing a serious difference. And I don't see any toolbar or theme changes, intentional or otherwise, in Thunderbird checkins

Please post a screen shot of the full width your toolbar. And, does it reproduce in safe mode?

Flags: needinfo?(dannyfox)
Status: UNCONFIRMED → RESOLVED
Closed: 1 year ago
Flags: needinfo?(dannyfox)
Resolution: --- → INCOMPLETE

Full-screen Thunderbird UI image captured from TB 78.10.0 but representative of the height issue which started at 78.3.1. For scale: Physical screen is 11.25 inches high. Toolbar buttons are about 1/8" (or 3/32") taller than they were (and taller than they need to be). Costs about a line of email displays -- either a line of messages, or a line in the preview pane.

There is much more significant impact on a smaller laptop screen. We happen to use fewer buttons so we can afford to put "text beside icons" and have thereby shorter buttons.

I don't see anything obvious in Config Editor that controls the height of the toolbar or height of the buttons, nor do I see any options that control size or height of the buttons or their font, etc.

This happens when we set in customize "Icons and Text". Then the dropmarker of the "Tag" button is below the text.

This is a regression from de-XBL. The elements inside the button are vertically positioned. Before de-XBL there was a box that contained the icon and text and the dropmarker was alone and we was able to position it on the side of this box. Now without of this box we can't position only the dropmarker on the side.

Because of this I hide the dropmarker and use a background image that is positioned on the side of the icon/text.

Assignee: nobody → richard.marti
Status: RESOLVED → REOPENED
Ever confirmed: true
Attachment #9220343 - Flags: review?(alessandro)
Resolution: INCOMPLETE → ---
Status: REOPENED → ASSIGNED
Keywords: regression
Summary: Mail Toolbar too high as of 78.3.1 → Mail Toolbar too high as of 78.3.1 when using "Icons and Text"

Mhh...I'm not sure about this.
The patch indeed fixes it but it feels a bit hacky.

Shouldn't this be addressed in the toolkit? The fix would literally be a simple container wrapping the image and the label.
Then we can update our CSS to set -moz-box-orient: vertical; only to that container and ignore the dropmarker.

This could be fixed in toolkit when you are doing this. FX IMHO doesn't use such toolbarbuttons. And when they use them, then not like we with toolbar[mode="full"] but only in icon mode or without the dropmarker. So they have no need to fix this.

Comment on attachment 9220343 [details] [diff] [review]
1691297-menu-button-dropmarker.patch

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

I guess we can go with this solution.
FF doesn't have this type of stacked text+icon layout, so I doubt they will be cool with changing the toolkit for such a unique edge case.

Also, we're so far into the 78 cycle and close tot he next ESR that I'd suggest to not uplift due to the multiple changes we did in the CSS.
It would be very tricky to fix on 78.

A little problem, when the button is clicked and hovered (:hover:active) this class takes over and the dropmarker image disappears: https://searchfox.org/comm-central/rev/b4ebcfcee86b24afb1706c45cb0705f31f37a5aa/mail/themes/shared/mail/messenger.css#441
Maybe that class in the messenger.css should only be affecting the `background-color`.

::: mail/themes/shared/mail/messenger.css
@@ +519,5 @@
> +toolbox:not([labelalign="end"]) > toolbar[mode="full"]
> +  .toolbarbutton-1:not(.button-appmenu)[type="menu"] {
> +  padding-inline-end: 12px !important;
> +  background-image: url("chrome://messenger/skin/icons/toolbarbutton-arrow.svg");
> +  background-size: 9px 9px;

remove the second 9px to let the height naturally adapt and keep the size ratio.

@@ +521,5 @@
> +  padding-inline-end: 12px !important;
> +  background-image: url("chrome://messenger/skin/icons/toolbarbutton-arrow.svg");
> +  background-size: 9px 9px;
> +  background-repeat: no-repeat;
> +  background-position: calc(100% - 4px) center;

would this work with rtl? You're the expert here :D
Attachment #9220343 - Flags: review?(alessandro) → feedback+

(In reply to Alessandro Castellani [:aleca] from comment #10)

Comment on attachment 9220343 [details] [diff] [review]
1691297-menu-button-dropmarker.patch

Review of attachment 9220343 [details] [diff] [review]:

A little problem, when the button is clicked and hovered (:hover:active)
this class takes over and the dropmarker image disappears:
https://searchfox.org/comm-central/rev/
b4ebcfcee86b24afb1706c45cb0705f31f37a5aa/mail/themes/shared/mail/messenger.
css#441
Maybe that class in the messenger.css should only be affecting the
background-color.

This was a remnant from when we used gradients. changed to background-color.

::: mail/themes/shared/mail/messenger.css
@@ +519,5 @@

+toolbox:not([labelalign="end"]) > toolbar[mode="full"]

  • .toolbarbutton-1:not(.button-appmenu)[type="menu"] {
  • padding-inline-end: 12px !important;
  • background-image: url("chrome://messenger/skin/icons/toolbarbutton-arrow.svg");
  • background-size: 9px 9px;

remove the second 9px to let the height naturally adapt and keep the size
ratio.

done

@@ +521,5 @@

  • padding-inline-end: 12px !important;
  • background-image: url("chrome://messenger/skin/icons/toolbarbutton-arrow.svg");
  • background-size: 9px 9px;
  • background-repeat: no-repeat;
  • background-position: calc(100% - 4px) center;

would this work with rtl? You're the expert here :D

No, and I had already a rule for this some lines below. Moved it up to follow directly to be more obvious. :-)

Attachment #9220343 - Attachment is obsolete: true
Attachment #9220539 - Flags: review?(alessandro)
Comment on attachment 9220539 [details] [diff] [review]
1691297-menu-button-dropmarker.patch

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

Looks good, thanks.

::: mail/themes/shared/mail/messenger.css
@@ +528,5 @@
> +}
> +
> +toolbox:not([labelalign="end"]) > toolbar[mode="full"]
> +  .toolbarbutton-1:not(.button-appmenu)[type="menu"]:-moz-locale-dir(rtl) {
> +  background-position: 4px center;

nit: maybe just background-position-x?
Attachment #9220539 - Flags: review?(alessandro) → review+

(In reply to Alessandro Castellani [:aleca] from comment #12)

Comment on attachment 9220539 [details] [diff] [review]
1691297-menu-button-dropmarker.patch

nit: maybe just background-position-x?

No, then the dropmarker would be on top right.

Target Milestone: --- → 90 Branch

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/b6cdba0a1e2f
Fix the dropmarker position .toolbarbutton-1[type="menu"] in "Icons and Text" toolbar mode. r=aleca

Status: ASSIGNED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED

Comment on attachment 9220539 [details] [diff] [review]
1691297-menu-button-dropmarker.patch

[Approval Request Comment]
User impact if declined: too high toolbar when "Icons and Text" is configured
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): should be low

Attachment #9220539 - Flags: approval-comm-beta?

[Approval Request Comment]
User impact if declined: too high toolbar when "Icons and Text" is configured
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): should be low. I built a try on all platforms and checked them.

Attachment #9220765 - Flags: approval-comm-esr78?
Regressions: 1710214

[Approval Request Comment]
User impact if declined: too high toolbar when "Icons and Text" is configured
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): should be low

Updated patch to fix a regression (bug1710214).

Attachment #9220765 - Attachment is obsolete: true
Attachment #9220765 - Flags: approval-comm-esr78?
Attachment #9220946 - Flags: approval-comm-esr78?

Comment on attachment 9220539 [details] [diff] [review]
1691297-menu-button-dropmarker.patch

[Triage Comment]
Approved for beta

Attachment #9220539 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9220946 [details] [diff] [review]
1691297-menu-button-dropmarker-ESR.patch

[Triage Comment]
Approved for esr78

Attachment #9220946 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.