57757 - [PATCH] Link color and underline preferences should be honored in Composer and mail compose

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Marc Attinasi]

link colors and link underlining are 'hard coded' in EditorOverride.css.  They 
should be using the preferences, but to do that, the way the pref stylesheet is 
disabled on the presShell has to be changed to allow finer granularity (i.e. 
caller decides what prefs gets enabled/disabled).

Comment 1

[Ben Bucksch (:BenB)]

mozilla0.8 is the wrong target Milestone - it is for the N6.0 branch. Use
mozilla0.9, if you mean the next Mozilla milestone.

Comment 2

[Ben Bucksch (:BenB)]

eh, mozilla0.6 is wrong. No such thing as mozilla0.8.

Comment 3

[Marc Attinasi]

This depends on the work indicated by bug 58534, namely making the rules for
link colors and underline reference the pref colors.

Comment 4

[amshey]

Ben Bucksch wrote on 2000-10-23:
> eh, mozilla0.6 is wrong. No such thing as mozilla0.8.

But, there is now a mozilla0.8 milestone; unless this is urgently needed,
perhaps the milestone should be changed now.

Comment 5

[Marc Attinasi]

Target Milestone ==> 1.0

Comment 6

[Alfonso Martinez]

*** Bug 96300 has been marked as a duplicate of this bug. ***

Comment 7

[Akkana Peck]

*** Bug 100844 has been marked as a duplicate of this bug. ***

Comment 8

[Alfonso Martinez]

*** Bug 100379 has been marked as a duplicate of this bug. ***

Comment 9

[TucsonTester1]

I am seeing this (or something very similar) in Composer rev. 2001112603 on Win 98

Steps to reproduce:
1. Launch Composer
2. Type in text then select it
3. Assign a URL to it thus making it a hyperlink

Expected behavior:
The hyperlinked text should display as blue and underlined.

Actual behavior:
The hyperlinked text is displayed with no visible changes. 
A url doesn't show up as blue and underlined unless it is inserted directly. It
will show up once the page is saved and reopened or the text is copied and
pasted back onto the page.

Comment 10

[Charles Manske]

*** Bug 115822 has been marked as a duplicate of this bug. ***

Comment 11

[Greg K.]

See also my bug 122185.

Comment 12

[Charles Manske]

*** Bug 125513 has been marked as a duplicate of this bug. ***

Comment 13

[Charles Manske]

Daniel: Do you have any cycles to look at this problem?

Comment 14

[Håkan Waara]

Looks like Daniel would need to fix the depending bug first (which is assigned
to him anyway) before starting work on this one. :-)

Comment 15

[sujay]

nominating due to high number of DUPS.

Comment 16

[sujay]

*** Bug 139117 has been marked as a duplicate of this bug. ***

Comment 17

[Akkana Peck]

This bug makes composer unusable for any page with a dark background, because
linked text becomes impossible to read.

Comment 18

[Kevin McCluskey (gone)]

nsbeta1-. Engineers are overloaded with higher priority bugs

Comment 19

[Syd Logan]

Let's revisit this after more pressing issues are resolved, maybe post beta.
Assigning the cmanske since some amount of UI is probably needed in composer app
to support this.

Comment 20

[sujay]

*** Bug 146510 has been marked as a duplicate of this bug. ***

Comment 21

[Boris Zbarsky [:bzbarsky]]

*** Bug 156176 has been marked as a duplicate of this bug. ***

Comment 22

[Alfonso Martinez]

*** Bug 157437 has been marked as a duplicate of this bug. ***

Comment 23

[Kathleen :Brade]

seeking reconsideration due to the number of duplicates

Comment 24

[Syd Logan]

Adding EDITORBASE+

Comment 25

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Syd: plussing this bug w/o plussing 58534 (the current one hevaily depends on it)
is a bit strange...

Comment 26

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.0, ready for reviews

Comment 27

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.1, saw a bad typo

Comment 28

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: grrr, patch v1.2

Comment 29

[Kathleen :Brade]

Comment on attachment 94031 [details] [diff] [review]
patch v1.0, ready for reviews

r=brade

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 94033 [details] [diff] [review]
grrr, patch v1.2

>+        nsILookAndFeel::nsColorID colorID = (nsILookAndFeel::nsColorID)aValue.GetIntValue();

Why GetIntValue() again?  You already have |intValue}....

Also, could we add a comment to the nsILookAndFeel.h code saying that all the
values MUST be nonnegative or else weird shit would come down?

>+        default:
>+          break;

how about a warning or assertion or something?	I'd prefer an assertion.  Like
NS_NOTREACHED?

>+        if (underlineLinks) {
>+          text->mTextDecoration |= NS_STYLE_TEXT_DECORATION_UNDERLINE;
>+        }
>+        else {
>+          text->mTextDecoration &= ~NS_STYLE_TEXT_DECORATION_UNDERLINE;
>+        }

This seems very wrong... so if I do:

a { text-decoration: -moz-underlineanchors; }
.foo { text-decoration: underline }

then a.foo will have no text-decoration?  That's _very_ wrong.

Could we name this "anchor-decoration" instead of "underlineanchors"?  Just in
case we ever get "visited-decoration" (which there is a request for, by the
way).  Fix the NS_STYLE_TEXT_DECORATION_PREF_UNDERLINE_ANCHORS to be 
NS_STYLE_TEXT_DECORATION_PREF_ANCHORS or something?

>+  eCSSKeyword__moz_hyperlinktext, NS_COLOR_MOZ_HYPERLINKTEXT,

__moz_hyperlink_color, maybe?

>+  eCSSKeyword__moz_visitedhyperlinktext, NS_COLOR_MOZ_VISITEDHYPERLINKTEXT,

__moz_visited_hyperlink_color?

>+  eCSSKeyword__moz_underlineanchors, NS_STYLE_TEXT_DECORATION_PREF_UNDERLINE_ANCHORS,

__moz_link_decoration?

>+CSS_KEY(-moz-hyperlinktext, _moz_hyperlinktext)
>+CSS_KEY(-moz-underlineanchors, _moz_underlineanchors)
>+CSS_KEY(-moz-visitedhyperlinktext, _moz_visitedhyperlinktext)

And rename these accordingly?  More dashes for greater redability. ;)

Comment 31

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

>>+ nsILookAndFeel::nsColorID colorID =
nsILookAndFeel::nsColorID)aValue.GetIntValue();
> 
> Why GetIntValue() again?  You already have |intValue}....

Oops.

> Also, could we add a comment to the nsILookAndFeel.h code saying that all the
> values MUST be nonnegative or else weird shit would come down?

Sure.

>>+        default:
>>+          break;
> 
> how about a warning or assertion or something?	I'd prefer an assertion.  Like
> NS_NOTREACHED?

Ok.

>>+        if (underlineLinks) {
>>+          text->mTextDecoration |= NS_STYLE_TEXT_DECORATION_UNDERLINE;
>>+        }
>>+        else {
>>+          text->mTextDecoration &= ~NS_STYLE_TEXT_DECORATION_UNDERLINE;
>>+        }
> 
> This seems very wrong... so if I do:
> 
> a { text-decoration: -moz-underlineanchors; }
> .foo { text-decoration: underline }
> 
> then a.foo will have no text-decoration?  That's _very_ wrong.

No, that's not. text->mTextDecoration is assigned by the line above the
chunk of code you copied. I showed you during an IRC chat and you agreed with
that.

> Could we name this "anchor-decoration" instead of "underlineanchors"?  Just in
> case we ever get "visited-decoration" (which there is a request for, by the
> way).  Fix the NS_STYLE_TEXT_DECORATION_PREF_UNDERLINE_ANCHORS to be 
> NS_STYLE_TEXT_DECORATION_PREF_ANCHORS or something?

Good proposal.

>>+  eCSSKeyword__moz_hyperlinktext, NS_COLOR_MOZ_HYPERLINKTEXT,
>>+  eCSSKeyword__moz_visitedhyperlinktext, NS_COLOR_MOZ_VISITEDHYPERLINKTEXT,
> 
> __moz_hyperlink_color, maybe?
> __moz_visited_hyperlink_color?

No, the names come from CSS 3 Color module and we have no lattitude here.

>>+  eCSSKeyword__moz_underlineanchors,
NS_STYLE_TEXT_DECORATION_PREF_UNDERLINE_ANCHORS,
> 
> __moz_link_decoration?

Hmmm... Will try to find something you could like...

Comment 32

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.3

Comment 33

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

removing dependency to bug 58534

Comment 34

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked in (trunk)

Comment 35

[sujay]

verified in 9/6 trunk.