Closed Bug 954937 Opened 8 years ago Closed 8 years ago

[Bubbles] Change link color in system messages

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(8 files, 4 obsolete files)

*** Original post on bio 1505 at 2012-06-12 11:56:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1505 as attmnt 1591 at 2012-06-12 11:56:00 UTC ***

Extracted from Bubbles-Light, as requested.
Attachment #8353348 - Flags: review?(clokep)
Attached image Screenshot (obsolete) —
*** Original post on bio 1505 as attmnt 1592 at 2012-06-12 12:01:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Comment on attachment 8353348 [details] [diff] [review]
Patch

*** Original change on bio 1505 attmnt 1591 at 2012-06-12 12:21:11 UTC ***

r+ with the 50% changed to 30% (as Mic noted on IRC: these colors are difficult to tell apart, especially if the underline is removed).
Attachment #8353348 - Flags: review?(clokep) → review+
Attached patch PatchSplinter Review
*** Original post on bio 1505 as attmnt 1593 at 2012-06-12 12:35:00 UTC ***

Changed to 30%, since more contrast seems wanted. Carrying the review forward.
Attachment #8353350 - Flags: review+
Comment on attachment 8353348 [details] [diff] [review]
Patch

*** Original change on bio 1505 attmnt 1591 at 2012-06-12 12:35:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353348 - Attachment is obsolete: true
Assignee: nobody → aleth
Whiteboard: [checkin-needed]
*** Original post on bio 1505 at 2012-06-12 22:29:02 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/f73b70977541

We can tweak this to be a bit lighter after trying it on the nightlies for a bit, if need be.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1505 as attmnt 1595 at 2012-06-13 13:16:00 UTC ***

Based on the feedback, I am attaching two new suggestions, which hopefully should be a better compromise. They avoid the need to style topic system messages differently (this seems overkill to me, we already have a special place to display the topic, and there is a bug on file to improve that and make links clickable there too).

Both variants have no underline on links in system messages. The underline reappears on hover unless the user has disabled it overall, along with a darker link color.

Patch v2: Links appear slightly darker than the surrounding text.

Patch v3: Links appear slightly paler than the surrounding text.

I think v3 makes more sense (the surrounding text is more, not less, important than the URL.)

An intermediate possibility would be to have the link colour be the same as the surrounding text until the link is hovered over.
Attachment #8353352 - Flags: review?(clokep)
Attached image Screenshot for v2
*** Original post on bio 1505 as attmnt 1596 at 2012-06-13 13:16:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 1505 as attmnt 1597 at 2012-06-13 13:17:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attached image Screenshot for v3
*** Original post on bio 1505 as attmnt 1598 at 2012-06-13 13:18:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Comment on attachment 8353354 [details] [diff] [review]
Patch v3

*** Original change on bio 1505 attmnt 1597 at 2012-06-13 13:18:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353354 - Flags: review?(clokep)
Attached image Screenshot v4
*** Original post on bio 1505 as attmnt 1599 at 2012-06-13 13:28:00 UTC ***

For comparison - no colour difference between text and link until hover.
*** Original post on bio 1505 as attmnt 1600 at 2012-06-13 13:31:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attached patch Patch v4Splinter Review
*** Original post on bio 1505 as attmnt 1601 at 2012-06-13 13:32:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353358 - Flags: review?(clokep)
*** Original post on bio 1505 as attmnt 1602 at 2012-06-13 13:41:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 1505 at 2012-06-13 13:46:15 UTC ***

03:41:13 PM - flo: [...] just one comment about the patch: my first reaction when I read it was that |:not(:hover)| seemed like it could be removed if we added a text-decoration: underline; in the :hover case.
03:41:23 PM - flo: you may want to add in the code the comment that explains it in the bug

text-decoration in the :not(:hover) case removes the underline on the link if it is present already, which is the default setting for links. We don't want to add underline if the user has disabled underlined links overall.
Comment on attachment 8353349 [details]
Screenshot

*** Original change on bio 1505 attmnt 1592 at 2012-06-13 16:05:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353349 - Attachment is obsolete: true
Comment on attachment 8353358 [details] [diff] [review]
Patch v4

*** Original change on bio 1505 attmnt 1601 at 2012-06-14 13:04:14 UTC ***

Let's try this on nightlies! :)
Attachment #8353358 - Flags: review?(clokep) → review+
*** Original post on bio 1505 at 2012-06-14 13:31:55 UTC ***

(In reply to comment #14)
> Comment on attachment 8353358 [details] [diff] [review] (bio-attmnt 1601) [details]
> Patch v4
> 
> Let's try this on nightlies! :)

Committed as http://hg.instantbird.org/instantbird/rev/e209b56ace45
Comment on attachment 8353354 [details] [diff] [review]
Patch v3

*** Original change on bio 1505 attmnt 1597 at 2012-06-14 14:54:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353354 - Attachment is obsolete: true
Attachment #8353354 - Flags: review?(clokep)
Comment on attachment 8353352 [details] [diff] [review]
Patch v2

*** Original change on bio 1505 attmnt 1595 at 2012-06-14 14:54:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353352 - Attachment is obsolete: true
Attachment #8353352 - Flags: review?(clokep)
*** Original post on bio 1505 as attmnt 1614 at 2012-06-15 11:27:00 UTC ***

We don't want any <a> tag to be highlighted on hover, in particular not those whose href is stripped out by http://lxr.instantbird.org/instantbird/source/chat/modules/imContentSink.jsm#45. Also increase contrast a little bit.
Attachment #8353371 - Flags: review?(florian)
Whiteboard: [checkin-needed]
Comment on attachment 8353371 [details] [diff] [review]
Change to -moz-any-link

*** Original change on bio 1505 attmnt 1614 at 2012-06-21 00:35:18 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353371 - Flags: review?(florian) → review+
*** Original post on bio 1505 at 2012-06-21 01:12:25 UTC ***

(In reply to comment #16)
> Created attachment 8353371 [details] [diff] [review] (bio-attmnt 1614) [details]
> Change to -moz-any-link
> 
> We don't want any <a> tag to be highlighted on hover, in particular not those
> whose href is stripped out by
> http://lxr.instantbird.org/instantbird/source/chat/modules/imContentSink.jsm#45.
> Also increase contrast a little bit.

Checked in as http://hg.instantbird.org/instantbird/rev/a0957ec93c4f

Hopefully this is the last tweak. :-D
Whiteboard: [checkin-needed]
You need to log in before you can comment on or make changes to this bug.