824938 - Remove custom styling for text-links in arrow panels on OS X

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

bug 771284 did a couple of things:

* It changed "#notification-popup .text-link" to "#notification-popup .text-link, .panel-arrowcontent .text-link", where the second selector makes the first one redundant.

* It changed the color from #fff to #0073e6 since the popup background isn't black anymore. However we shouldn't have to specify any color, the native link color should work just fine here.

* It removed the underline. I don't see why we would want this for arrow panels specifically. If really desired, we should do it for all text-links in toolkit.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 696012 [details] [diff] [review]
patch

This patch seems reasonable to me - I don't see a reason to remove the underline, the two selectors do seem redundant. Frank would be more familiar with the reasoning behind the original changes than I would, though.

Is it safe to assume that the default text color will always be dark?

Comment 2

[Frank Yan (:fryn)]

Comment on attachment 696012 [details] [diff] [review]
patch

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

(In reply to Dão Gottwald [:dao] from comment #0)
> * It removed the underline. I don't see why we would want this for arrow
> panels specifically. If really desired, we should do it for all text-links
> in toolkit.

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> This patch seems reasonable to me - I don't see a reason to remove the
> underline, the two selectors do seem redundant. Frank would be more familiar
> with the reasoning behind the original changes than I would, though.

The selector redundancy was likely just a mistake.
The removal of the underlining was part of the intended design by Stephen to make things cleaner. He is okay with this inconsistency, but I prefer the consistency, so I am approving this on both technical and design grounds. He explicitly stated his approval of my decision. We both prefer removing underlining from all text links (probably with underlining as hover indication), and I will file a bug shortly for that.


> Is it safe to assume that the default text color will always be dark?

As I understand it, if the default text color is changed, the default hyperlink color should also change as needed. We shouldn't have to worry about that styling at this level.

Comment 3

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/141a154d9ca3

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to Frank Yan (:fryn) from comment #2)
> As I understand it, if the default text color is changed, the default
> hyperlink color should also change as needed. We shouldn't have to worry
> about that styling at this level.

The concern is whether the text-link's default color (-moz-nativehyperlinktext) will conflict with the hardcoded background color in the default styling for arrow panels' panel-arrowcontent (http://hg.mozilla.org/mozilla-central/annotate/5bb309998e70/toolkit/themes/pinstripe/global/popup.css#l69).

If that is a problem, it should be fixed in toolkit panel styling, not browser window styling, but the current rule may be papering over the problem, so we should move it rather than removing it.

Comment 5

[Frank Yan (:fryn)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)

I see. Thank you for explaining. The colors do not conflict in practice, and it seems unlikely that they would in the future. I'm not concerned about it, but if you are, I'm okay with moving the rule, which I agree would be the correct fix.

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/141a154d9ca3