426732 - Implement -moz-nativehyperlinktext

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

Currently hyperlinks in Firefox, both in chrome and content, are hard coded to (0,0,255).  We use native platform colors for hyperlinks instead of full blue to give the product a softer and more integrated feel.

Comment 1

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

Attachment: Vista: hyperlinks in chrome

Example of (0,0,255) hyperlinks in chrome

Comment 2

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

Attachment: Vista: preferences for hyperlinks in content

Example of default visited and unvisited link colors for hyperlinks in the content area.

Comment 3

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

Attachment: Vista: native color of hyperlinks in content (IE)

Example of the softer blue used in vista for hyperlinks, this case in the content area.

Comment 4

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

Attachment: Vista: native color of hyperlinks in chrome (media player)

Example of the softer color for hyperlinks in the content area, from windows media player.

Comment 5

[Ian Spence]

In case added code is in the scope of this bug, in GTK 2.10, Widgets have style attributes "visited-link-color" and "link-color" which would be perfect for this

Comment 6

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

Attachment: OS X: example of the softer color for hyperlinks

Here is an example of hyperlinks in mail.app

Comment 7

[Ryan A. C.]

There is a HotTrack system color for windows.
http://msdn2.microsoft.com/en-us/library/system.drawing.systemcolors.hottrack.aspx

Comment 8

[Ryan A. C.]

Attachment: Implement COLOR_HOTLIGHT v1

After a little digging I found COLOR_HOTLIGHT: 
http://msdn2.microsoft.com/en-us/library/ms724371(VS.85).aspx
I'm not sure what it has to do with hotlights :P, but it does seem to return the right color on vista, and from what I've found this was implemented first in windows 98. This patch just sticks it in with the CSS user defined system colors, which is probably not quite the right way to do this. I'm not sure what winCE will do with this color either, so it might have to be #ifndefed out of that one.
The next step I suppose is to just make -moz-hyperlinktext default to COLOR_HOTLIGHT, but change when the user changes it.

Comment 9

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

Comment on attachment 314007 [details] [diff] [review]
Implement COLOR_HOTLIGHT v1

Neil, is this the right way to expose the property?

This isn't a blocker, but I don't think it's any worse for us to hard code the right colours if possible. Would take a minimally invasive patch, which might not be easy since I'm not sure that this is a theme thing as opposed to a widget thing :(

Comment 10

[Neil Deakin]

Comment on attachment 314007 [details] [diff] [review]
Implement COLOR_HOTLIGHT v1

Use '-moz-linktext' rather than 'hotlight' to be clearer and as it isn't a css defined value.

Also, add the color to nsXPLookAndFeel.cpp for preference support.

And of course, fix up the  alignment in nsILookAndFeel.h

Comment 11

[Ryan A. C.]

we already have a -moz-hyperlinktext, wouldn't -moz-linktext be a little redundant and confusing?  I was thinking maybe -moz-nativelinktext or something (which could be filled by link-color on linux for example) or just filling -moz-hyperlinktext with %nativecolor by default, but somehow changing it to user pref when the user changes it.
I was also wondering, since nsCSSProps is global, what would happen on linux/mac?

I found color_hotlight is also not implemented on NT4, which (as of firefox2 anyways) is still on the supported OS list.

Comment 12

[Ryan A. C.]

I think we have a couple options:

1) Default browser.anchor_color (and therefore -moz-hyperlinktext) to the appropriate value per-os. This is certainly the easiest way afaik, just a few #ifs. Personally I think this would be a decent solution, since it's user configable anyways. 

2) Implement native colors. We know windows provides us with one, which is editable using the Advanced Appearance dialog, apparently GTK gives us one (what does that do on kde?), and I can't find anything on OSX having such a property or not. It seems accessibility is brought up in every bug about theme/color, but windows at least does use different Hyperlink colors on high contrast themes, which is something to consider.

We might be able to do a bit of both though, since the hard coded default of -moz-hyperlinktext is defined right here:
http://mxr.mozilla.org/mozilla/source/layout/base/nsPresContext.cpp#199
we just need to set that to %nativecolor when applicable using #ifdef I think? (or does this get overridden by a default pref?)
We wouldn't even need to add a CSS keyword for that one.

Comment 13

[Ryan A. C.]

Attachment: Implement COLOR_HOTLIGHT v1.1

Addresses everything in Comment #10, but with a different CSS keyword due to my concerns in Comment #11

To set this as the default color of -moz-hyperlinktext you simply set browser.anchor_color to -moz-nativelinktext since colors in user prefs are able to take CSS colors. This is the easiest way I've found to set default native colors until the user specifies otherwise, albeit a strange and somewhat round-about way. I was going to make a patch for this too, but I didn't know if this should be done in firefox.js or all.js. Either way it's a simple #ifdef XP_WIN to set the pref for now anyways, could change if we do other OS native link colors.

Interesting tidbit since I'm sure everyone is now wondering: setting browser.anchor_color to -moz-hyperlinktext produces some strange seemingly random color, possibly some pointer being turned into a color?

Comment 14

[Ryan A. C.]

Wait, setting browser.anchor_color to -moz-nativelinktext doesn't work. It works in the preferences > content > colors window properly, but it doesn't work right in the actual content.
That patch will give us a CSS color for native color hyperlinks on windows, but there's still no way to default to that native color.

Comment 15

[Ryan A. C.]

Attachment: Implement COLOR_HOTLIGHT v1.1.1

My code editors hate me :/
I think I've finally found the pref that's causing tabs to look like spaces.
Fixes the alignment, see comment #13 for the rest

Comment 16

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

Might as well get the ability to pull the colour, even if we're not going to use it immediately. Hurrah for third party themers.

Comment 17

[Reed Loden [:reed]]

Comment on attachment 314528 [details] [diff] [review]
Implement COLOR_HOTLIGHT v1.1.1

need sr for this...

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks OK to me but a style system person should sign off on it.

I would want a comment somewhere explaining how this differs from -moz-hyperlinktext ... it seems the answer is "this one is not affected by user preferences".

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In addition to what roc said, you should document what background color it goes against.

Comment 20

[Ryan A. C.]

Should that be documented in code or MDC? probably both?
I'm guessing a quick blurb in code like //extracted native hyperlink color from the system, not affected by user preferences
MDC should probably mention that it's good on Window, ThreeDFace and -moz-dialog
There is no official MSDN documentation on what background this color should be used on, I've just seen it personally on those in native chrome on windows.

I'm also thinking that we should take this alongside the gnome color if we can get it (I don't really know how to do that one), and if we can't find anything for OSX then we should probably just return the user pref or a hard coded color that matches the screenshot in attachment 313314 [details]

Comment 21

[Ryan A. C.]

So, does this make sense/work? I don't fully understand the workings of GTK or mozilla widget GTK code, but from what I can tell this appears to be correct. I would build & check this myself, but I don't have a linux machine that can build mozilla. 
file: http://mxr.mozilla.org/mozilla/source/widget/src/gtk2/nsLookAndFeel.cpp

  case eColor__moz_nativelinktext:
//"link-color" is implemented since GTK 2.10 
#if GTK_CHECK_VERSION(2,10,0)
     GdkColor colorvalue = NULL;
     gtk_widget_style_get(MOZ_GTK_WINDOW, "link-color", &colorvalue, NULL);
     if (colorvalue) {
        aColor = GDK_COLOR_TO_NS_RGB(colorvalue);
        break;
     }
// fall back to hard coded
#endif //GTK_CHECK_VERSION(2,10,0)
     aColor = NS_RGB(0x00,0x00,0xEE);
     break;

Specifically I'm not sure I'm calling gtk_widget_style_get correctly. I used MOZ_GTK_WINDOW because I think this is the GtkWidget for a window, and the link-color of a window is probably exactly what we want.
It defaults to #0000EE because that's the current value of most hyperlinks.

roc? I believe you're the guy to ask for Widget - GTK

Comment 22

[Ryan A. C.]

(In reply to comment #21)
bah, I forgot the * for the pointer in the definition and aColor assignment

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I meant document in code comments, and this latest patch doesn't do that yet.

We do need Mac and GTK2 code for this. We should also see the code to actually use this in Firefox.

Michael Ventnor can probably test/finish the GTK2 path.

Comment 24

[Ryan A. C.]

Attachment: Implement COLOR_HOTLIGHT v1.2

Adds comments, and hard codes the mac color (assuming the underline is the non-aliased color)
I searched and can not find any reference to a link, hyperlink or anchor text color anywhere in cocoa or as a kThemeTextColor*. There might be something if we hooked in to webkit, they have a -webkit-link, but I'm guessing we don't want to do that.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks good, but still needs GTK love.

Comment 26

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

>I searched and can not find any reference to a link, hyperlink or anchor text
>color anywhere in cocoa or as a kThemeTextColor*. There might be something if
>we hooked in to webkit, they have a -webkit-link

I couldn't find this information either.  As far as I can tell Safari doesn't use platform native hyperlink colors, and instead defaults to (0,0,255).

Comment 27

[Ryan A. C.]

I've gotten this far with the GTK version:
http://pastebin.mozilla.org/404229
This still doesn't work (link goes red, colorValuePtr remains null) and I can't figure out why. I'm attempting this on Kubuntu in KDE, but GTK should be all the same.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Michael V, can you help here?

Comment 29

[Michael Ventnor]

Ryan, looking at your pastebin, that code doesn't belong there. It must go in InitLookAndFeel() and the resulting color saved to a static variable.

I think the problem is he hasn't realized the window; this will be fixed (and a LOT of code removed) if he moves the code to InitLookAndFeel() and reuses the widgets and local variables there. I could be wrong though, but he should still move the code regardless.

Also, I think colorValuePtr needs to be freed like what was done with the odd-row-color code.

We also don't need the 2.10 check, we require 2.10 now.

Comment 30

[Ryan A. C.]

Attachment: Implement COLOR_HOTLIGHT v1.5

It works! And As far as I can tell, it has been working for some time now :/
Even the version on that pastebin works (which I meant to mention, was written that way for testing purposes, I was going to do it this way once I actually got something working.)
Finding a gtk2 theme that actually specifies a link-color has proven more difficult than pulling it out. (I ended up just editing the gtkrc file of Raleigh)

Interesting note: as far as I can tell any GtkWidget will work, we could use an already defined widget rather than creating the gtklinkbutton, but it is possible AFAIK to specify link-color per widget, so this is probably the best way to do it.

Comment 31

[Michael Ventnor]

Why don't you move the gdk_color_free call to within the first check for colorValuePtr?

Comment 32

[Ryan A. C.]

Attachment: Implement -moz-nativelinktext v1.6

Good point, I thought of that as I was going over the diff, but left it like that because that's how the treeview was done. Now that I look over the treeview again though I see there's 2 instances where it could be used, which is obviously why it was done that way there.

Comment 33

[Ryan A. C.]

Attachment: -moz-nativelinktext in content by default v1

This patch is separate, but obviously depends on the other one.
I had to add a special case in the part where it reads the pref, but I don't know if there's a better way to do this. It might be worth changing MakeColorPref to recognize named colors, but this is so far the first time we've wanted to do this.

Comment 34

[Ryan A. C.]

Attachment: -moz-nativelinktext v1.6 unbitrot

I suppose I never did document which background colors this goes against, but since that isn't done on any of the other native colors I think it's something best left for MDC. On there we can note that there is no official documentation specifying a safe background color, but that it's used on dialogs and windows in native OS apps.

Comment 35

[Ryan A. C.]

Attachment: -moz-nativelinktext v1.6 unbitrot 2

the old one still works with fuzz, but might as well attach this

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What about comment 19?  (I didn't go through the other comments in detail to check that they were addressed; I just found one that wasn't addressed.)

Comment 37

[Ryan A. C.]

(In reply to comment #36)
> What about comment 19?

I kind of addressed it in comment 34. If it should be documented in code; is nsILookAndFeel.h the best place for that with the other comments?

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 319518 [details] [diff] [review]
-moz-nativelinktext v1.6 unbitrot 2

>+    eColor__moz_nativelinktext,				//hyperlink color extracted from the system, not affected by user pref

Could you call this value (and the corresponding CSS value) nativehyperlinktext rather than nativelinktext?  I think that's more consistent with existing names.  (Or was there a reason you chose what you did?)


>     // Colors which will hopefully become CSS3

Could you add your new color to the end of the section below this comment, rather than above it?

And could you add an item to layout/style/test/property_database.js testing that the new value is parsed?

r=dbaron with those changes (I don't seem to have permission to grant the review flag you requested, though I can grant the other review flag)

I'm presuming that roc's superreview+ above means he reviewed the platform widget implementations; if not, somebody with some platform knowledge should review that.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, the platform widget implementations are reviewed.

Comment 40

[Ryan A. C.]

(In reply to comment #38)
> And could you add an item to layout/style/test/property_database.js testing
> that the new value is parsed?

I've looked at this file, and tbh I'm not entirely certain on how to add this property to it. If I understand it correctly the Initial value would be entirely dependent on your system settings, so I could only guess on the initial values. For other_values the only thing that it wouldn't really be is transparent/semi-transparent. And the only invalid thing is non-longhand colors, since it should always return six character colors. With that, based on other items in the file I have this, but I don't know if it's correct:

"-moz-nativehyperlinktext": {
domProp: "MozNativeHyperLinkText",
inherited: false,
type: CSS_TYPE_LONGHAND,
initial_values: [ "#0000ee", "#144fae", "#0066cc", "#000080", "#0000ff" ],
other_values: [ "transparent", "rgba(255,128,0,0.5)" ],
invalid_values: [ "#0", "#00", "#0000", "#00000", "#0000000", "#00000000", "#000000000" ]


Other than that I've made the other changes mentioned, though I will wait for feedback on this before posting a patch. It was named that way mostly due to the suggestion in comment #10, but nativehyperlinktext makes more sense.

Comment 41

[Dão Gottwald [:dao]]

David, comment 40 is for you.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

-moz-nativehyperlinktext isn't a property; it's a value for existing properties.  You should add two items to the "other_values" line for the "color" property and then check that the mochitests in layout/style/test still show no failures.

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #37)
> I kind of addressed it in comment 34. If it should be documented in code; is
> nsILookAndFeel.h the best place for that with the other comments?

Yes.

Comment 44

[Ryan A. C.]

Attachment: -moz-nativehyperlinktext v1.7

All comments should be addressed, I rearranged stuff to make a little more sense (these changes are at the bottom of the appropriate list unless they should be elsewhere), added a comment on the mac color in-line with other comments in the file, and clarified exactly which user pref it is not affected by.

Comment 45

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/dd2a7349ee20

Comment 46

[Wesley Johnston (:wesj)]

Added to:

http://developer.mozilla.org/en/docs/CSS_Reference:Mozilla_Extensions

Hope that's right.

Comment 47

[Dão Gottwald [:dao]]

Bug 371870 and bug 437358 are about using the native color in chrome. Please file a new bug for using it in content.