1703988 - [meta] UI issues with new Proton style

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect, P3)

Description

[Alessandro Castellani [:aleca]]

Attachment: proton-issues.png

Some random UI issues now that Proton is enabled by default in Thunderbird.

1. The icons in the calendar sidebar have wrong colors.
2. The input fields in the editable menulist in the Calendar event dialog have wrong padding/margin.
3. The tabs in the manage identities subdialog have a border around visually focused text.

I'm sure I missed other issues, but these are the most noticeable.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1703988-Proton-UI-issues-fix.patch

This fixes the issues.
The tab focus-ring is always shown when a dialog is opened. Then the tab has automatically the :focus-visible, also on non-proton. Probably a change they did. I removed the focus-ring and show instead a button-background-color. This looks more natural like a selected tab.

Comment 2

[Alessandro Castellani [:aleca]]

Turning this into a meta bug to keep open since more and more tiny issues will pop out from now and then.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9214741 [details] [diff] [review]
1703988-Proton-UI-issues-fix.patch

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

For the tabs, in the edit identity subdialog, it seems we're importing the common.css twice.
We should set the border-bottom transparent for this `xul|tab:hover`.
Also, we should remove this
```
tab + tab {
    margin-inline-start: -2px;
}
```
as it makes the tabs weirdly overlap. I'm not sure if it is needed somewhere else.

::: calendar/base/themes/common/widgets/calendar-widgets.css
@@ +237,5 @@
>    appearance: none;
>    list-style-image: url("chrome://messenger/skin/icons/hidden.svg");
> +  color: inherit;
> +  background-color: transparent;
> +  border-color: transparent;

Since m-c seem to be slowly stopping using `list-style-image` we should follow that approach and use the more common background for images.
Let's use `background: url("chrome://messenger/skin/icons/hidden.svg") transparent no-repeat center;`

@@ +250,5 @@
>    opacity: 0.9 !important;
>  }
>  
>  #calendar-list > richlistitem > checkbox[checked="true"] > .checkbox-check {
>    list-style-image: url("chrome://messenger/skin/icons/visible.svg");

Replace this with background-image

::: mail/themes/linux/mail/themeableDialog.css
@@ +61,5 @@
> +menulist[is="menulist-editable"][editable="true"]::part(text-input) {
> +  padding: 5px 4px;
> +  margin-block: -2px;
> +  margin-inline: -7px 7px;
> +}

This looks like there's an extra pixel at the bottom and is not properly aligned with the menulist.
using margin-block: -1px; and margin-inline: -6px 7px; and then removing the border tot he text-input makes it look good for me on Linux.
What do you think?

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: compose-issues.png

Found another couple of issues in the compose.
The identity menulist is not aligned anymore with the other fields, and the separator before the other recipient buttons lost its proper spacing.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: 1703988-Proton-UI-issues-fix.patch

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

Comment on attachment 9214741 [details] [diff] [review]
1703988-Proton-UI-issues-fix.patch

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

For the tabs, in the edit identity subdialog, it seems we're importing the
common.css twice.

It seems the subdialog loader does no more check if the stylesheet is already loaded and adds it a second time. This would need removing the common.css stylesheet from accountManage.css and add it to all XHTML files that aren't loaded in a dialog. Something for a different bug.

We should set the border-bottom transparent for this xul|tab:hover.
Also, we should remove this

tab + tab {
    margin-inline-start: -2px;
}

as it makes the tabs weirdly overlap. I'm not sure if it is needed somewhere
else.

Linux only. Added the fixing rule to preferences.css.

::: calendar/base/themes/common/widgets/calendar-widgets.css
@@ +237,5 @@

appearance: none;
list-style-image: url("chrome://messenger/skin/icons/hidden.svg");

• color: inherit;
• background-color: transparent;
• border-color: transparent;

Since m-c seem to be slowly stopping using list-style-image we should
follow that approach and use the more common background for images.
Let's use background: url("chrome://messenger/skin/icons/hidden.svg") transparent no-repeat center;

Done

@@ +250,5 @@

opacity: 0.9 !important;
}

#calendar-list > richlistitem > checkbox[checked="true"] > .checkbox-check {
list-style-image: url("chrome://messenger/skin/icons/visible.svg");

Replace this with background-image

Also done

Also fixed the messageIdentity menulist margins.

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9214798 [details] [diff] [review]
1703988-Proton-UI-issues-fix.patch

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

Perfect!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/94207006db0a
Fix UI issues with new Proton style. r=aleca

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: Bug 1703988 - followup to fix invalid margin-inline. r=Paenglab

Was this the intention? It's breaking comm/mail/test/static/browser_parsable_css.js

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d039101b4579
followup to fix invalid margin-inline. r=Paenglab

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: proton-chevron.gif

Another little annoying thing here.
In the main AppMenu the chevron togo back to a previous panel view is larger then it should, and also comes with an extra margin to the left that makes it unaligned compared to other elements.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: 1703988-appMenu-back-button.patch

I missed this part. I took the code from FX.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Set the correct dimensions for the AppMenu back button. r=aleca

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b665e2c71112
Set the correct dimensions for the AppMenu back button. r=aleca

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Fix the composer's address fields and set the correct colors on focus and hover. r=aleca

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2021-04-15 10-00-22.png

There's an extra border in the message pane when no message is currently visible, making the vertical splitter looking thicker than normal.
This extra border comes from the border bottom added to the #mail-notification-top element.
https://searchfox.org/comm-central/rev/fcb2f819cde716b9ed6596168f5a0498819b88a0/mail/themes/shared/mail/messageHeader.css#11-19

Comment 16

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Fix the composer's address fields and set the correct colors on focus and hover. r=aleca

Comment 17

[Magnus Melin [:mkmelin]]

The From and To fields in the invite attendees dialog are huge.

Comment 18

[Richard Marti (:Paenglab)]

I see no From or To fields in invite attendees dialog. Can you attach a screenshot?

Comment 19

[Magnus Melin [:mkmelin]]

Here's a screenshot: https://pasteboard.co/JXtLpSg.png

Comment 20

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Fix the menulists in invite attendees dialog. r=mkmelin

Comment 21

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Hide the #mail-notification-top when we hide the #msgHeaderView. r=aleca

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b645de0a2ebe
Fix the composer's address fields and set the correct colors on focus and hover. r=aleca
https://hg.mozilla.org/comm-central/rev/3e3bdcf7ad87
Fix the menulists in invite attendees dialog. r=mkmelin

Comment 23

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Revert the huge sizes of the prefs buttons, menulists and textboxes. r=aleca

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/94b1d1b981b2
Hide the #mail-notification-top when we hide the #msgHeaderView. r=aleca

Comment 25

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2021-04-19 09-49-28.png

Weird double focus ring in the compose addressing fields.

Comment 27

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5c92cfbd485d
Backed out changeset 94b1d1b981b2 per request. r=aleca,rjl DONTBUILD

Comment 28

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Hide and show the #mail-notification-top together with #msgHeaderView. r=aleca

Comment 29

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Fix the focus outlines on composer. r=aleca

Depends on D112641

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b1d272365e52
Hide and show the #mail-notification-top together with #msgHeaderView. r=aleca

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2f57b4686c0d
Revert the huge sizes of the prefs buttons, menulists and textboxes. r=aleca

Comment 32

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2021-04-20 17-03-17.png

Exterior outline for focused elements.
This happens for all the regular buttons, checkbox, radio, etc., in our dialogs.

Comment 33

[Richard Marti (:Paenglab)]

Comment on attachment 9217288 [details]
Screenshot from 2021-04-20 17-03-17.png

This is done by the new Proton --focus-outline-offset variable. Actually set to 2px. I could reduce to 1px. 0px seems not so optimal as the focus would connect the selected checkmark which makes almost not noticeable because both have the same colour.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bcf67013249f
Fix the focus outlines on composer. r=aleca

Comment 35

[Alessandro Castellani [:aleca]]

Attachment: account-settings.png

The account settings sidebar doesn't have the darker grey background anymore, removing the necessary visual separation between the 2 columns.

Comment 36

[Richard Marti (:Paenglab)]

Comment on attachment 9218040 [details]
account-settings.png

This is a regression from bug 1702228.

Comment 37

[Richard Marti (:Paenglab)]

Attachment: Bug 1703988 - Make in the preferences the buttons/menulists better visible. r=aleca

Comment 38

[Richard Marti (:Paenglab)]

Comment on attachment 9218040 [details]
account-settings.png

Fixed by bug 1707460.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0ccf7a766635
Make in the preferences the buttons/menulists better visible. r=aleca

Comment 40

[José M. Muñoz]

Do you plan to backport any of this the TB 89 beta? The strange focus ring from attachment 9216844 [details] is still present in TB 89 beta 2.

Comment 41

[Alessandro Castellani [:aleca]]

I'm not sure we will. The majority of these issues are visually annoying but not high risk or full on breakages, so we prefer to let these run the train.

Comment 42

[Richard Marti (:Paenglab)]

I close now this bug as we have probably fixed all issues introduced by Proton. If we find new ones we can do them in follow-up bugs.

Comment 43

[Thomas D. (:thomas8)]

Attachment: Double focus ring confusion in composer - fixed by https://phabricator.services.mozilla.com/D112652

First of all, thanks to Richard for all the awesome patches in here!

I understand that most of these are cosmetic and hence technically inconsequential, so that we may not want to uplift each and every patch here as Alex said.

However, I'd like to point out one itching issue on Beta which I'd like to suggest for uplift, the double focus ring in composition's address rows when there's at least one pill in the row.
Double focus ring...

• ...covers the address row above (as seen in the screenshot)
• ...is extremely irritating when testing beta and a pain in the eye
• ...looks very broken and amateurish, i.e. makes us look bad - if it looks this broken on the outside, what about inside!?

The patch for the focus outline is very small and self-contained, so it should be easy to uplift and no-risk.

Comment 44

[Thomas D. (:thomas8)]

Comment on attachment 9216907 [details]
Bug 1703988 - Fix the focus outlines on composer. r=aleca

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Double focus ring on address rows in composition, covers other rows, looks broken, weird and irritating every time pills are added. See my screenshot and details in Comment 43.
Testing completed (on c-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Low - small, self-contained styling change

Comment 45

[Alessandro Castellani [:aleca]]

I'm not sure this will apply cleanly as that patch depends on previous patches that are not on beta.
Thomas, can you check?

Comment 46

[Richard Marti (:Paenglab)]

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

I'm not sure this will apply cleanly as that patch depends on previous patches that are not on beta.
Thomas, can you check?

Tried it, it applies.

Comment 47

[Wayne Mery (:wsmwk)]

Comment on attachment 9216907 [details]
Bug 1703988 - Fix the focus outlines on composer. r=aleca

[Triage Comment]
Approved for beta

Comment 48

[Rob Lemley [:rjl]]

Thunderbird 89.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/6f1413ea267b