Closed Bug 1572106 Opened 4 months ago Closed 4 months ago

Fix attachment item .text-link decoration on win

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(thunderbird_esr6868+ fixed, thunderbird69 fixed, thunderbird70 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
thunderbird_esr68 68+ fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: alta88, Assigned: Paenglab)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1571663 +++

(In reply to alta88 from comment #2)

In addition to this, there is a regression on win only (maybe mac too), where a detached/external attachment does not appear like a link, the .text-link color is overridden. I think it's due to .text-link no longer being added to the child node post de-xbl. This is an important visual.

(In reply to Jorg K (GMT+2) from comment #6)

Created attachment 9083483 [details]
link-when-hovered.png

Well, there is an underline shown for a link when hovered. If not hovered, it looks like the others.

No longer depends on: 1571663

This fixes the issue and also removes a unneeded colour for the size text which makes it unreadable with dark theme.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9083717 - Flags: review?(alta88)
Comment on attachment 9083717 [details] [diff] [review]
1572106-attachmentItem-color.patch

The original code set .text-link on the .attachmentcell-name element. It was done to prevent exactly this kind of css hackery and divergence into platform specific workarounds. Several bugs later (boxobject, de-xbl) this was lost.
```
+      let name = attachmentitem.boxObject.firstChild
+                               .getElementsByClassName("attachmentcell-name");
+      name[0].classList.add("text-link");
```
I think for the sanity of anyone maintaining the css (you :)) it would be better to do a querySelector to get .attachmentcell-name element and set .text-link there rather than the attachmentitem ancestor.
Attachment #9083717 - Flags: review?(alta88)

Is this what you had in mind?

Attachment #9083717 - Attachment is obsolete: true
Attachment #9083786 - Flags: review?(alta88)
Comment on attachment 9083786 [details] [diff] [review]
1572106-attachmentItem-color.patch

Yes, thanks! r+ on inspection.
Attachment #9083786 - Flags: review?(alta88) → review+
Keywords: checkin-needed
Comment on attachment 9083786 [details] [diff] [review]
1572106-attachmentItem-color.patch

The other attachment bugs (bug 1562200 and bug 1563793) have ESR68 approval too.
Attachment #9083786 - Flags: approval-comm-esr68?
Attachment #9083786 - Flags: approval-comm-beta?

So the colour is some link colour (blue-ish) but the underline only comes on when hovered. That's intentional?

Yes, the original patch was for CVE level spoofing issues, so the intent was to make external attachments different from internal attachments, and the very common behavior is blue for a link, underline on hover and show the url.

Attachment #9083786 - Flags: approval-comm-esr68?
Attachment #9083786 - Flags: approval-comm-esr68+
Attachment #9083786 - Flags: approval-comm-beta?
Attachment #9083786 - Flags: approval-comm-beta+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/45b5c5bb3fe6
On Windows show the detached attachment text with the hyperlinktext color. r=alta88

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.