486653 - chrome links should never be red

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

UL text-links are currently red in the active and focused states. As mentioned in bug 471838, HTML hyperlinks are by default red in the active state, but not in the focused state. Links in chrome shouldn't even be red in the active state, based on how native Windows and Gtk apps behave.

There's also styling for text-links with a "visited" attribute. That appears to be unused, and it's also not native. We should get rid of that, too.

Comment 1

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/305a59dbe437

Comment 2

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

I don't think we should take this on 1.9.1 at this stage, my r+ was meant only for the trunk.

Comment 3

[AKIHIRO Misaki (a.k.a Kuden)]

http://hg.mozilla.org/mozilla-central/annotate/38d50cc03a72/browser/themes/winstripe/browser/preferences/preferences.css#l114

browser/skin/preferences/preferences.css
> .inline-link:focus, .inline-link:hover:active {
>   color: red;
> }

Need not this change?

Comment 4

[Dão Gottwald [:dao]]

That has been added after I wrote this patch. Looks like it's missing a focus ring, so I can't just remove the red.

Comment 5

[AKIHIRO Misaki (a.k.a Kuden)]

(In reply to comment #4)
> Looks like it's missing a focus ring

I see.
Certainly, .inline-link cannot be selected with the keyboard. This is a problem of accessibility. I also think that this is another bug.

Comment 6

[Dão Gottwald [:dao]]

Oh yeah, that's even worse. I see you've filed bug 494887 on that.

(In reply to comment #2)
> I don't think we should take this on 1.9.1 at this stage

Are you seeing any particular risk?

Comment 7

[(no longer active)]

(In reply to comment #4)
> That has been added after I wrote this patch. Looks like it's missing a focus
> ring, so I can't just remove the red.

I guess we can just replace the color with a border for :focus rules, right?

Comment 8

[Dão Gottwald [:dao]]

Basically yes, although I think outline is more suitable than border.

Comment 9

[(no longer active)]

(In reply to comment #8)
> Basically yes, although I think outline is more suitable than border.

I remember now why I couldn't use borders in the first place.  The links in that dialog are inline elements, so the border/outline attribute can't be applied to them (unless I'm missing something.)

I asked around when writing that patch about the possibility of using HTML <a> links in that dialog, but couldn't find out any way to do that.  I think <a> provides both the correct focus ring and keyboard focus...

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> I remember now why I couldn't use borders in the first place.  The links in
> that dialog are inline elements, so the border/outline attribute can't be
> applied to them (unless I'm missing something.)

It's possible that there's something broken, but in general there's no such restriction.

> I asked around when writing that patch about the possibility of using HTML <a>
> links in that dialog, but couldn't find out any way to do that.  I think <a>
> provides both the correct focus ring and keyboard focus...

You're actually using html:a, but without the href attribute.

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #9)
> > I remember now why I couldn't use borders in the first place.  The links in
> > that dialog are inline elements, so the border/outline attribute can't be
> > applied to them (unless I'm missing something.)
> 
> It's possible that there's something broken, but in general there's no such
> restriction.

I'm certain that I was getting that behavior, but now that I tested it seems to work...  I'm not sure what was different back then... :/

> > I asked around when writing that patch about the possibility of using HTML <a>
> > links in that dialog, but couldn't find out any way to do that.  I think <a>
> > provides both the correct focus ring and keyboard focus...
> 
> You're actually using html:a, but without the href attribute.

Yes, but it doesn't seem work as it does in HTML.  Does the href attribute have an important role here?

Comment 12

[Dão Gottwald [:dao]]

Yes, it does; without it, <a> isn't really a hyperlink. For example, I think :-moz-any-link won't apply.

Comment 13

[(no longer active)]

(In reply to comment #12)
> Yes, it does; without it, <a> isn't really a hyperlink. For example, I think
> :-moz-any-link won't apply.

Yes, this is indeed the case.  Thanks for describing this.

Comment 14

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

(In reply to comment #6)
> Are you seeing any particular risk?

Some XUL users might want that styling, and changing it at the last second isn't very nice. Granted, the risks aren't that great, but neither are the benefits, and at this stage, the tree-churn costs of low-risk/low-benefit patches is significant.

Comment 15

[Joe Drew (not getting mail)]

Dao, you landed this on m-c without approval1.9.1.

Comment 16

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

(In reply to comment #15)
> Dao, you landed this on m-c without approval1.9.1.

I don't think that's a problem.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #15)
> Dao, you landed this on m-c without approval1.9.1.

Yes, because being fixed on trunk usually helps with getting 1.9.1 approval. And it looks like if you follow the rules on the m-c tinderbox strictly, you couldn't even land stuff with 1.9.1 approval.

(In reply to comment #14)
> Some XUL users might want that styling, and changing it at the last second
> isn't very nice.

Fair. That's a concrete trade-off that I think the drivers can approve or deny.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Yeah, I agree with Gavin, no need to churn here on the branch.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Why don't we have any focus indicator? It's nearly impossible to determine if such a text link has focus now.

Comment 20

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

(In reply to comment #19)
> Why don't we have any focus indicator? It's nearly impossible to determine if
> such a text link has focus now.

What do you mean by "focus indicator"? The patch only changed colors, it didn't affect focus outlines.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #20)
> What do you mean by "focus indicator"? The patch only changed colors, it didn't
> affect focus outlines.

See the about dialog. We don't use such an focus outline. Is there already an existing bug?

Comment 22

[Dão Gottwald [:dao]]

bug 471838

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Dao. I overread this part on bug 471838.

Marking verified fixed on trunk with builds on OS X and Windows like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre ID:20090527031500

Comment 24

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P1 - Polish issue that appears in the main window, or is something that the user may encounter several times a day.

general widget bugs are picking up P1s