Closed Bug 486653 Opened 11 years ago Closed 11 years ago

chrome links should never be red

Categories

(Toolkit :: Themes, defect, trivial)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: polish, Whiteboard: [polish-easy] [polish-visual][polish-p1])

Attachments

(1 file)

Attached patch patchSplinter Review
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.
Attachment #370835 - Flags: review?(gavin.sharp)
Blocks: 277717
Whiteboard: [polish-easy] [polish-visual]
Attachment #370835 - Flags: review?(gavin.sharp) → review+
http://hg.mozilla.org/mozilla-central/rev/305a59dbe437
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #370835 - Flags: approval1.9.1?
I don't think we should take this on 1.9.1 at this stage, my r+ was meant only for the trunk.
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?
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.
(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.
Depends on: 494887
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?
(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?
Basically yes, although I think outline is more suitable than border.
(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...
(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.
(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?
Yes, it does; without it, <a> isn't really a hyperlink. For example, I think :-moz-any-link won't apply.
(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.
(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.
Dao, you landed this on m-c without approval1.9.1.
(In reply to comment #15)
> Dao, you landed this on m-c without approval1.9.1.

I don't think that's a problem.
(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.
Attachment #370835 - Flags: approval1.9.1? → approval1.9.1-
Comment on attachment 370835 [details] [diff] [review]
patch

Yeah, I agree with Gavin, no need to churn here on the branch.
Why don't we have any focus indicator? It's nearly impossible to determine if such a text link has focus now.
(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.
(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?
bug 471838
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
Status: RESOLVED → VERIFIED
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
Whiteboard: [polish-easy] [polish-visual] → [polish-easy] [polish-visual][polish-p1]
You need to log in before you can comment on or make changes to this bug.