Closed Bug 1703988 Opened 2 years ago Closed 2 years ago

[meta] UI issues with new Proton style

Categories

(Thunderbird :: Theme, defect, P3)

Tracking

(thunderbird_esr78 unaffected, thunderbird88 unaffected, thunderbird89 fixed)

RESOLVED FIXED
89 Branch
Tracking Status
thunderbird_esr78 --- unaffected
thunderbird88 --- unaffected
thunderbird89 --- fixed

People

(Reporter: aleca, Assigned: Paenglab)

References

Details

Attachments

(11 files, 10 obsolete files)

11.76 KB, patch
aleca
: review+
Details | Diff | Splinter Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
63.49 KB, image/png
Details
48 bytes, text/x-phabricator-request
Details | Review
18.71 KB, image/png
Details
Attached image proton-issues.png (obsolete) —

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.

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.

Attachment #9214741 - Flags: review?(alessandro)

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

Severity: -- → N/A
Status: NEW → ASSIGNED
Keywords: leave-open
Priority: -- → P3
Summary: UI issues with new Proton style → [meta] UI issues with new Proton style
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?
Attachment #9214741 - Flags: review?(alessandro) → feedback+
Attached image compose-issues.png (obsolete) —

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.

(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.

Attachment #9214741 - Attachment is obsolete: true
Attachment #9214798 - Flags: review?(alessandro)
Comment on attachment 9214798 [details] [diff] [review]
1703988-Proton-UI-issues-fix.patch

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

Perfect!
Attachment #9214798 - Flags: review?(alessandro) → review+
Target Milestone: --- → 89 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/94207006db0a
Fix UI issues with new Proton style. r=aleca

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d039101b4579
followup to fix invalid margin-inline. r=Paenglab
Attached image proton-chevron.gif (obsolete) —

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.

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

Attachment #9215484 - Flags: review?(alessandro)
Attachment #9215484 - Attachment is obsolete: true
Attachment #9215484 - Flags: review?(alessandro)

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/b665e2c71112
Set the correct dimensions for the AppMenu back button. r=aleca

Attached image Screenshot from 2021-04-15 10-00-22.png (obsolete) —

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

Attachment #9216109 - Attachment is obsolete: true

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

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

Pushed by geoff@darktrojan.net:
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

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

Regressions: 1705893
Attached image Screenshot from 2021-04-19 09-49-28.png (obsolete) —

Weird double focus ring in the compose addressing fields.

Backout by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/5c92cfbd485d
Backed out changeset 94b1d1b981b2 per request. r=aleca,rjl DONTBUILD
Attachment #9216205 - Attachment is obsolete: true

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/2f57b4686c0d
Revert the huge sizes of the prefs buttons, menulists and textboxes. r=aleca

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

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.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/bcf67013249f
Fix the focus outlines on composer. r=aleca
Regressions: 1706895
Attachment #9214553 - Attachment is obsolete: true
Attachment #9214773 - Attachment is obsolete: true
Attachment #9215271 - Attachment is obsolete: true
Attachment #9216149 - Attachment is obsolete: true
Attachment #9216844 - Attachment is obsolete: true
Attached image account-settings.png (obsolete) —

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

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

This is a regression from bug 1702228.

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

Fixed by bug 1707460.

Attachment #9218040 - Attachment is obsolete: true

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/0ccf7a766635
Make in the preferences the buttons/menulists better visible. r=aleca

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.

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.

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.

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED

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 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

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

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

(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 on attachment 9216907 [details]
Bug 1703988 - Fix the focus outlines on composer. r=aleca

[Triage Comment]
Approved for beta

Attachment #9216907 - Flags: approval-comm-beta? → approval-comm-beta+
Blocks: 1710848
Regressions: 1731934
You need to log in before you can comment on or make changes to this bug.