101982 - Toolbar does not update colors or other inline attribute states such as bold, italic, and underline

Status: VERIFIED INVALID

(SeaMonkey :: Composer, defect, P2)

Description

[TucsonTester2]

Build ID: 20010924

In Composer if you change the New Page settings to use colors you specify then
change those in composer using the "Page Colors and Background" window, then the
color indicators on the toolbar do not update.

Reproducible:  Always

Steps to Reproduce:
1.Open Composer
2.Click on Edit->Preferences
3.Choose Composer->New page settings
4.Click on "Use custom colors" and change the colors settings to anythig but the
default
5.Click ok and then open a new composer window
6.Click on Format->Page Colors and Background
7.Click on "Readers default colors" and click ok

Actual Resutls:
The colors on the toolbar will not update unless you change the color.  The text
will be black but on the toolbar it will show you the color you defined. The
same thing goes for the background color.

Expected Results:
I would expect that the background and text color would update on the toolbar.

Comment 1

[Syd Logan]

Definitely. EDITORBASE, assigning to cmanske

Comment 2

[Charles Manske]

The Composer toolbar uses an "observes" node to trigger inline style state, e.g.:
<observes element="cmd_bold" type="checkbox" attribute="state"    
   onbroadcast="onButtonUpdate(this.parentNode, 'cmd_bold')"/>

The "state" attribute seems to be set correctly, but the "onbroadcast" methods
are no longer being called. That is why the toolbar does not reflect the state of
the attribute for the colorpicker, paragraph styles, bold, etc.

Waterson: Some have suggested that changes you made in this area may have caused 
this. Any ideas?

Comment 3

[sujay]

is bug 106980 related to this one ?

Comment 4

[Charles Manske]

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

Comment 5

[Chris Waterson]

I'll take a look.

Comment 6

[Chris Waterson]

Attachment: simple test case for |onbroadcast| handler

Here's a simple test case for the |onbroadcast| method. It turns out I blew
this pretty badly when re-writing broadcasters. Patch coming up.

Comment 7

[Chris Waterson]

Attachment: fix for simple test case

This fixes the simple test case, but I'm still having trouble with the editor
buttons.

So what does this do? When moving the ExecuteBroadcastHandler routine from
nsXULElement to nsXULDocument, my ``clean up'' was wrong: I goofed up the
|listener| and |child| pointers in several places in the patch. Added some
comments to keep others from doing the same.

Also, I jiggled the logic in AttributeChanged to more faithfully match what was
happening in the old code. Specifically, we fire the |onbroadcast| even before
notifying any of the document's observers. I'm not sure if this matters or not,
but who knows.

Finally, (hyatt, help me out here), I've made sure that we fire the
|onbroadcast| even during the initial hookup. Is this right? (Seems like we
ought to be doing this.)

Anyway, this patch still needs some work to get editor off the floor. AFAICT,
the editor's |onbroadcast| event is firing just fine, and the correct values
are being set in the |onButtonUpdate| function in editor.js. Any clues would be
welcome here.

Comment 8

[Chris Waterson]

Attachment: hack to editor.js to make this work

So here's a hack to editor.js that gets the bold etc. buttons working
correctly. It explicitly sets the |checked| attribute to the correct value,
because just calling the JS routine doesn't appear to be sufficient. (Was it
ever?)

Comment 9

[Chris Waterson]

Attachment: hack to editor.js that covers all cases

Okay, so this hack gets the editor toolbar back alive. But it seems really
wrong. Did some low-level button XBL change recently? (Noting in editor.js
related to this has.)

Comment 10

[Chris Waterson]

cc'ing <broadcaster> compadres.

Comment 11

[Chris Waterson]

Attachment: test case demonstrating <toolbarbutton autoCheck="false" />

Hmm. This simple test demonstrates that a straight-forward <toolbarbutton> with
som JS on it that sets the |checked| attribute works fine.

Comment 12

[Charles Manske]

Thanks for fixing this!
I also tried all the patches and found that the toolbar buttons don't show the
proper style when "checked" without the extra "hack patch" to editor.js.
(I did a depend build on 11/12/01 and applied the patches.)
To explore this further, I put some dumps in the three methods that Chris put 
the 'hack': onButtonUpdate and onStateButtonUpdate are called correctly (the 
latter is only used for menuitems that observe cmd_align), and onStyleChange()
isn't used at all. So we don't mind if you simply delete onStyleChange() as 
part of this checkin.

Comment 13

[Charles Manske]

Comment on attachment 57390 [details] [diff] [review]
hack to editor.js that covers all cases

r=cmanske. Note that onStyleChange() is not used anymore, so the third part
isn't needed. onStyleChange() method should be removed.

Comment 14

[Charles Manske]

Note that the fix to nsXULDocument.cpp and .h look fine to me, but I don't feel
qualified to give it an r= since I'm not familiar with that code.

Comment 15

[David Hyatt]

sr=hyatt

Comment 16

[Charles Manske]

Comment on attachment 57387 [details] [diff] [review]
fix for simple test case

Went over the code thoroughly with hyatt
r=cmanske
sr=hyatt

Comment 17

[Charles Manske]

Comment on attachment 57390 [details] [diff] [review]
hack to editor.js that covers all cases

I'll look into this problem further and figure out why ".checked" doesn't work
or file a bug on the issue.
sr=hyatt for hack.

Comment 18

[Chris Waterson]

Okay, fix checked in on the trunk. Hyatt figured out why the .checked attribute
wasn't doing anything: turns out that the |onbroadcast| handler was firing
before the XBL for <toolbarbutton> had loaded, and this whomped the JS property.
So, I've just #if 0'd and commented the code that hooks up the broadcaster
before |onload| fires (maybe we can try to fix this some day if we care).

Comment 19

[Chris Waterson]

Marking fixed. Can we live without this on the mozilla-0.9.6 branch? It smells a
bit risky to me, but...

Comment 20

[michael wendell]

I am still seeing the original problem on Win 2k and Win Me using build
2001111503 and Mac OS 9.1 using build 2001111504.  Also, after step 7, the
toolbar icon is not clickable.  

106980 which was duped to this is fixed, but the original problem described here
is still happening.

I am reopening this bug.  

Comment 21

[Charles Manske]

I will be fixed in today's build. Waterson didn't checkin the editor portion,
but I did early this morning.
But someday, we should really fixt the ".checked" problem as waterson describes.

Comment 22

[Charles Manske]

I tested this in today's build after removing the extra "setAttribute" hacks
and it works as Waterson claimed.
I'll attach a patch to remove that from editor.js

Comment 23

[Charles Manske]

Attachment: Remove unnecessary "setAttribute" calls for "checked" toolbar buttons

Comment 24

[Kathleen :Brade]

Comment on attachment 57986 [details] [diff] [review]
Remove unnecessary "setAttribute" calls for "checked" toolbar buttons

r=brade

Comment 25

[Charles Manske]

Conflicting reports with today's build on this issue: bug 74811.

Comment 26

[kinmoz]

Comment on attachment 57986 [details] [diff] [review]
Remove unnecessary "setAttribute" calls for "checked" toolbar buttons

sr=[e-mail]

Comment 27

[Charles Manske]

this is really fixed; checking in removal of editor.js hack today. 
Changing milestone anyway.

Comment 28

[Charles Manske]

Hacks in editor.js checked in. 
Resolving as fixed as I reassign to waterson, who did the real work.

Comment 29

[Charles Manske]

fixed

Comment 30

[michael wendell]

I am still seeing the original issue on build 2001112803.

I am reopening this bug again.

Comment 31

[TucsonTester1]

on Build 2001121405, Mac OS X, I am seeing part of this bug reoccuring. The item
that was referenced in on bug 106980 is apparently working, however the original
issue on this bug where the toolbar color palette is not updating after changing
the color selection for text and background is still showing. You can even
recreate this by opening composer, going to the Format/Page Colors and
Background menu without having to set the selection originally in preferences.

Comment 32

[Charles Manske]

The orginal bug description is invalid anyway.
The settings described are for a "New Page" not the currentl page. The issue fixed 
in this bug was really the general "observer" problem that also caused bug 106980.
The text and background color should change in the current page when you change it
using the "Page Colors and Background" dialog. If that problem occurs, I'll
open a new bug so we don't confuse the issue with the settings in the pref dialog.

Comment 33

[sujay]

verified.

Comment 34

[Charles Manske]

Bug 115694 is filed for the color on toolbar problems.