Closed Bug 89977 Opened 24 years ago Closed 24 years ago

Color popup sets bgcolor on <body> but other attributes aren't set

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.4

People

(Reporter: Brade, Assigned: cmanske)

Details

Attachments

(3 files)

fix
1.63 KB, patch
Details | Diff | Splinter Review
Updated patch: Need to write text colors only when colorpicker type is "Page"
1.66 KB, patch
Details | Diff | Splinter Review
Updated patch: Added missing try {} catch {} around pref code
2.67 KB, patch
Details | Diff | Splinter Review
In Composer, if the background color is set for the page, we should also set the text and link and other color attributes. If we don't know what color should be, we should grab the color used in preferences.
Priority: -- → P3
Whiteboard: [dialog]
Target Milestone: --- → mozilla1.0
The fix is not in the Color Picker dialog, so removing [dialog] status.
Status: NEW → ASSIGNED
Whiteboard: [dialog]
Target Milestone: mozilla1.0 → mozilla0.9.4
Attached patch fix — — Splinter Review
The patch sets the text and link colors when we set the page background color. One issue remains: What should we do if user removes the background color (uses the 'Default' button in colorpicker)? That is tricky. I don't do anything in current fix. The only thing that makes any sense is to compare each text/link color to the author's default, and if it is the same, remove it. That would leave non-default colors set. How does that sound?
Keywords: patch, review
Whiteboard: FIX IN HAND need r=, sr=
About my last suggestion: Note that in cases where the author *wants* to set text/link colors, and they happen to be the same as the default browser colors, removing them would be a bad thing. That is why I'm not sure we should do anything when user removes background color, so I didn't include it in the proposed patch.
Why should we set all the colors on <body>? What's the rationale here?
The rational is in the comment in the patch: If user sets a background color to something very dark because they happen to use a light text color as their browser default, the page may be unreadable to most other readers. We follow the rule of setting *all* colors on the <body> tag when user sets anything in the Page Colors and Background dialog. So this bug was suggested for consistency.
I'm not sure I agree with this. The user can set up all the page colors via the 'Page properties' dialog (although, using that dialog, you always set all the colors; you can't choose to just set, say, link color). Using the toolbar popup, they can set just the page background color. If we set the link and other colors when the user sets the page background from the popup, the user might get confused. She might want to just set the background color (bgcolor="white" is very common). Ideally, we should provide a way that the user can choose which colors are set on <body>.
Summary: bgcolor is set on <body> but other attributes aren't set → Color popup sets bgcolor on <body> but other attributes aren't set
You don't agree that setting all text, link, and background colors together is necessary to avoid unintended bad combinations by viewers who have different default browser colors?
I think it is wrong for us to set just the background color and not the rest of the colors. Documents with only one or a few of these set are very broken and we shouldn't allow users to easily create broken documents. They should either all be set or not at all. If a user wants to set the background color to "default" then we should probably ask if (or just assume?) they want to remove all of the colors. We could also just remove the "default" button from the background and instead put in text pointing users to the appropriate dialog to remove the colors. Regarding the latest patch, it might make sense to grab the current text color (or default color if it isn't set) to use as the color... I'm just thinking out loud. I'm not sure we're doing the best thing for what colors to set but I do think we should set *something*. Probably what Charley has now is the best. Also, because "GetDefaultBrowserColors" has some pref calls that aren't wrapped in try/catch, we should either fix those or add try/catch around our use of the call here (former is preferred).
Hardware: Macintosh → All
r=brade on the new patches
sure. sr=sfraser
Whiteboard: FIX IN HAND need r=, sr= → FIX IN HAND
checked in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Keywords: patch, review
Resolution: --- → FIXED
Whiteboard: FIX IN HAND
Verified on 8-30 Build
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: