827524 - Display underlining of browser UI text links only as hover indication

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Frank Yan (:fryn)]

As we already implemented in arrow panels previously, we would prefer that browser UI text links not as underlining while not hovered to avoid being unnecessarily distracting. The hyperlink color alone is prominent enough to differentiate text links, and this styling has become commonplace on the internet, with many sites, including Wikipedia and Bugzilla, adopting it. Distinguishing blue and black text (the default colors) is almost never a problem, even with color blindness.

Retaining underlining as hover indication is fine. Hover indication is often good, while hover UI is generally not.

Comment 1

[Frank Yan (:fryn)]

Attachment: patch

Design change approved by Stephen.

Comment 2

[Frank Yan (:fryn)]

:Gavin Sharp (use [e-mail] for email) 2013-01-07 14:45:55 PST
Component: Theme → Themes
Product: Firefox → Toolkit

I was just about to fix that. Thanks! :)

Comment 3

[Mardeg]

Instead of adding, eg.
.inline-link:hover
rules, it would be easier to add
text-decoration: none
to the existing
.inline-link
rules, and just change that to
.inline-link:not(hover)

That should still allow the default underlining to happen on hover (unless that really is no longer default)

Comment 4

[Mardeg]

Sorry, meant
.inline-link:not(:hover)

Comment 5

[Frank Yan (:fryn)]

(In reply to Mardeg from comment #3)
> Instead of adding, eg.
> .inline-link:hover
> rules, it would be easier to add
> text-decoration: none
> to the existing
> .inline-link
> rules, and just change that to
> .inline-link:not(:hover)
> 
> That should still allow the default underlining to happen on hover (unless
> that really is no longer default)

No, that doesn't work, because the elements with the class "inline-link" are <xul:description/>s and <xul:label/>s, which do not have underlining by default.

Comment 6

[Frank Yan (:fryn)]

(In reply to Frank Yan (:fryn) from comment #5)
> (In reply to Mardeg from comment #3)

Oh, wait. I mentally parsed the XUL files incorrectly. :P You're right.

Comment 7

[Frank Yan (:fryn)]

Attachment: patch v2

(In reply to Frank Yan (:fryn) from comment #6)
> (In reply to Frank Yan (:fryn) from comment #5)
> > (In reply to Mardeg from comment #3)

Actually, there are xul:descriptions and xul:labels with the class inline-link, but they aren't in the preferences and the files don't have preferences.css applies to them, so I'm not sure why those classes are used there. There doesn't seem to be additional semantic meaning. That's probably what misled me earlier.

New patch fixes the rule. Thanks for catching that. :)

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 698920 [details] [diff] [review]
patch v2

> .inline-link:not(:focus) {
>   outline: 1px dotted transparent;
> }

I wonder what this code is trying to do...

Comment 9

[Frank Yan (:fryn)]

Attachment: patch v3

Actually, that was no good for inline-links in the preferences, because there is no rule that applies the underline by default to :-moz-any-link.
about:preferences (the in-content preferences) doesn't have this problem, because it pulls in a magical about:PreferenceStyleSheet via nsPresShell.cpp.
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1221
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1379
I don't know if this disparity is intended.


(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 698920 [details] [diff] [review]
> patch v2
> 
> > .inline-link:not(:focus) {
> >   outline: 1px dotted transparent;
> > }
> 
> I wonder what this code is trying to do...

Ehsan wrote and you reviewed the change that added that.
http://hg.mozilla.org/mozilla-central/rev/16336ebe1344
See bug 494887.
I think he was trying to remove the outline that came from ua.css, but ua.css only applies it to *|*:-moz-any-link:-moz-focusring now.
https://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#87

Do we still need that, or did we ever need it? It doesn't seem like it to me. If not, I can remove it.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 698959 [details] [diff] [review]
patch v3

(In reply to Frank Yan (:fryn) from comment #9)
> Do we still need that, or did we ever need it? It doesn't seem like it to
> me. If not, I can remove it.

Please remove it.

Comment 11

[Frank Yan (:fryn)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c59e6f8bb2d9

(In reply to Dão Gottwald [:dao] from comment #10)
> Please remove it.

Removed in pushed patch.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c59e6f8bb2d9