Closed Bug 1716354 Opened 2 years ago Closed 1 year ago

Port bug 1699586: De-duplicate twisty/arrow icons

Categories

(Thunderbird :: Upstream Synchronization, task)

Tracking

(thunderbird_esr78 unaffected)

RESOLVED FIXED
91 Branch
Tracking Status
thunderbird_esr78 --- unaffected

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(2 files, 4 obsolete files)

Bug 1699586 cleans up the arrow icons inclusive the twisties.

Bug 1699586 is in the review and not yet on autoland. So no hurry.

I added on chat tab on the contacts twisty, on message header's message-id twisty and on new AB's twisty the turning animation we have already on other places. Unfortunately it's not easily possible to do this in a class because some twisties default is closed and on other open. Fortunately it's not much code.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9226850 - Flags: review?(alessandro)
Comment on attachment 9226850 [details] [diff] [review]
1716354-de-duplicate-arrow-icons.patch

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

Thanks for doing this.

A couple of problems I noticed.
In the new Address Book the arrow rotation is a bit jarring when you close a menulist. It looks like there's a small shift in position where the icon moves by 1px.
The arrows are gone from the folder pane list.

::: mail/components/im/messages/mail/main.css
@@ +151,4 @@
>  }
> +
> +.hide-children .eventToggle:-moz-locale-dir(rtl) {
> +  background: url("chrome://global/skin/icons/arrow-left-12.svg") no-repeat left center;

Could we rotate this item 180deg, or maybe use `scaleX(-1)`?
I'm thinking that we should try to have 1 single 12px arrow that we adapt with CSS based on the item and situation is used.
Instead of having a right, left, and down variation.
Not sure if it's possible.
Attachment #9226850 - Flags: review?(alessandro) → feedback+

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

The arrows are gone from the folder pane list.

Have you tested together with the M-C patches? If not it's clear because the arrows/twisty are renamed and without the M-C patches applied they point to no more existing icons.

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

Have you tested together with the M-C patches? If not it's clear because the arrows/twisty are renamed and without the M-C patches applied they point to no more existing icons.

You got me there :D
Sorry.

Same patch without the new-AB twisty animation.

Attachment #9226850 - Attachment is obsolete: true
Attachment #9226914 - Flags: review?(alessandro)
Comment on attachment 9226914 [details] [diff] [review]
1716354-de-duplicate-arrow-icons.patch

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

Looks good, thanks.
Let's not mark it for check in before the parent m-c patch has landed otherwise we'll have some visual issues.
Attachment #9226914 - Flags: review?(alessandro) → review+

Updated after landing of bug 1716209.

Attachment #9226914 - Attachment is obsolete: true
Attachment #9227679 - Flags: review+

They changed in bug 1699586 the menu-arrow.svg to arrow-right.svg -> followed.

Attachment #9227679 - Attachment is obsolete: true
Attachment #9228433 - Flags: review+

Hopefully the last update which includes bug 1704669.

Attachment #9228433 - Attachment is obsolete: true
Attachment #9228606 - Flags: review+

Bug 1699586 is now on autoland.

Whiteboard: Land after bug 1699586
Target Milestone: --- → 91 Branch

Backed-out again.

And again on autoland.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ce89a4f46247
Port bug 1699586: De-duplicate twisty/arrow icons. r=aleca

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

Looks like you missed some which are now causing test failures:

https://searchfox.org/comm-central/search?q=arrow-dropdown-12.svg&path=&case=false&regexp=false
TEST-UNEXPECTED-FAIL | comm/mail/test/static/browser_parsable_css.js | missing chrome://global/skin/icons/arrow-dropdown-12.svg referenced from chrome://messenger/skin/shared/EditorDialog.css

https://searchfox.org/comm-central/search?q=twisty-expanded.svg&path=&case=false&regexp=false
TEST-UNEXPECTED-FAIL | comm/mail/test/static/browser_parsable_css.js | missing chrome://global/skin/icons/twisty-expanded.svg referenced from chrome://messenger/skin/shared/messageHeader.css

Flags: needinfo?(richard.marti)
Status: RESOLVED → REOPENED
Flags: needinfo?(richard.marti)
Resolution: FIXED → ---

Thank you José for the heads up.

Attachment #9229093 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9229093 [details] [diff] [review]
1716354-fup.patch

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

Thx! r=mkmelin
Attachment #9229093 - Flags: review?(mkmelin+mozilla) → review+
Whiteboard: Land after bug 1699586

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/bcd3d9f6ac2a
Follow-up: Fix missed changes. r=mkmelin

Status: REOPENED → RESOLVED
Closed: 2 years ago1 year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.