Closed Bug 57757 Opened 19 years ago Closed 18 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.2alpha

People

(Reporter: attinasi, Assigned: glazou)

References

Details

(Keywords: relnote, Whiteboard: EDITORBASE+, has r= and sr=)

Attachments

(2 files, 2 obsolete files)

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).
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.6
mozilla0.8 is the wrong target Milestone - it is for the N6.0 branch. Use
mozilla0.9, if you mean the next Mozilla milestone.
eh, mozilla0.6 is wrong. No such thing as mozilla0.8.
This depends on the work indicated by bug 58534, namely making the rules for
link colors and underline reference the pref colors.
Depends on: 58534
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.

Target Milestone ==> 1.0
Target Milestone: mozilla0.6 → mozilla1.0
*** Bug 96300 has been marked as a duplicate of this bug. ***
*** Bug 100844 has been marked as a duplicate of this bug. ***
*** Bug 100379 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.0 → mozilla1.2
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.
*** Bug 115822 has been marked as a duplicate of this bug. ***
See also my bug 122185.
*** Bug 125513 has been marked as a duplicate of this bug. ***
Daniel: Do you have any cycles to look at this problem?
Looks like Daniel would need to fix the depending bug first (which is assigned
to him anyway) before starting work on this one. :-)
nominating due to high number of DUPS.
Keywords: nsbeta1
Whiteboard: EDITORBASE
*** Bug 139117 has been marked as a duplicate of this bug. ***
This bug makes composer unusable for any page with a dark background, because
linked text becomes impossible to read.
nsbeta1-. Engineers are overloaded with higher priority bugs
Keywords: nsbeta1nsbeta1-
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.
Whiteboard: EDITORBASE → EDITORBASE-
Keywords: relnote
*** Bug 146510 has been marked as a duplicate of this bug. ***
*** Bug 156176 has been marked as a duplicate of this bug. ***
*** Bug 157437 has been marked as a duplicate of this bug. ***
seeking reconsideration due to the number of duplicates
Whiteboard: EDITORBASE- → EDITORBASE
Adding EDITORBASE+
Assignee: attinasi → glazman
Status: ASSIGNED → NEW
Keywords: nsbeta1-nsbeta1+
Whiteboard: EDITORBASE → EDITORBASE+
Syd: plussing this bug w/o plussing 58534 (the current one hevaily depends on it)
is a bit strange...
Attached patch patch v1.0, ready for reviews (obsolete) — Splinter Review
Summary: Link color and underline preferences should be honored in Composer and mail compose → [PATCH] Link color and underline preferences should be honored in Composer and mail compose
Whiteboard: EDITORBASE+ → EDITORBASE+, needs r=, needs sr=, needs a=
Attached patch patch v1.1, saw a bad typo (obsolete) — Splinter Review
Attached patch grrr, patch v1.2Splinter Review
Attachment #94031 - Attachment is obsolete: true
Attachment #94032 - Attachment is obsolete: true
Comment on attachment 94031 [details] [diff] [review]
patch v1.0, ready for reviews

r=brade
Attachment #94031 - Flags: review+
Attachment #94032 - Flags: review+
Keywords: review
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. ;)
Attachment #94033 - Flags: superreview+
Attachment #94033 - Flags: needs-work+
>>+ 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...
Status: NEW → ASSIGNED
Attached patch patch v1.3Splinter Review
removing dependency to bug 58534
No longer depends on: 58534
Whiteboard: EDITORBASE+, needs r=, needs sr=, needs a= → EDITORBASE+, has r= and sr=
checked in (trunk)
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
verified in 9/6 trunk.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.