167712 - Can't revert to default attribute value, CSS mode

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

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

1. launch Composer, CSS mode
2. insert a table, 50% width
3. open Table properties dialog and align table to the right, click on Apply
4. align now table to the left, click on Apply

Expected result: left alignment of the table
Actual result: table remains right-aligned

This bug is not a simple one and I admit that I totally missed that during my
work on the CSSization of Composer. Our dialogs remove an attribute when we
revert to default values. But in CSS mode, they should also revert to CSS
default values or remove the property from the inline styles of the element!
The only way to do that is using the magic in nsHTMLCSSUtils.cpp and the
ChangeCSSInlineStyle transaction. But we need to call DoTransaction() without
putting the txn on the UndoStack.

This bug affects table props, image props, HR props and List props... I very
strongly trunk landing with drivers approval.
recommend

Comment 1

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

Attachment: patch v1.0

Fixes several bugs at once :

1. the current one; this implied adding the aDontRecordTransaction to
   nsEditor::RemoveAttributeOrEquivalent()
2. bug 167716; this implied adding the same to
   nsEditor::SetAttributeOrEquivalent()
3. VALIGN attribute on table elements was not CSSized
4. page colors remained HTML attrs when creating a new document in CSS mode
5. fixes nsEditor::CloneAttributes() as per discussion with Kin in #composer;
   this needs item 2
6. error in
http://lxr.mozilla.org/seamonkey/source/editor/ui/dialogs/content/EdTableProps.js#964

   should be testing newAlign instead of align

Comment 2

[kinmoz]

Daniel, I appologize for not reviewing this thoroughly today due to other things
going on ... some cursory comments for now:

-- Use PR_TRUE/PR_FALSE instead of true/false.

-- I normally try to avoid names with negatives in them because they become
confusing when people use the '!' operator with them. For example
|!aDontRecordTransaction|. |aRecordTransaction| or |aSupressTransaction| are
examples of things that don't sound negative to me. :-) By the way, in cases
like this, Joe normally uses a default initializer for the most common case so
that not every caller has to specifiy true or false ... but I understand if you
want to force every call to be explicit ... I normally do.

-- Make sure any of the transactions you are manually calling DoTransaction() on
don't make editor calls that use transactions, otherwise your effort will have
been in vain. :-)

Comment 3

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

> Daniel, I appologize for not reviewing this thoroughly today due to other
> things going on ... some cursory comments for now:

No problem :-)

> Use PR_TRUE/PR_FALSE instead of true/false.

Oh! Did I leave true/false in c++ ? Oh, yes once ! Hum, sorry for that.

> I normally try to avoid names with negatives in them because they become
> confusing when people use the '!' operator with them. For example
> |!aDontRecordTransaction|. |aRecordTransaction| or |aSupressTransaction| are
> examples of things that don't sound negative to me. :-)

Ok. Updating the patch.

>  By the way, in cases
> like this, Joe normally uses a default initializer for the most common case so
> that not every caller has to specifiy true or false ... but I understand if you
> want to force every call to be explicit ... I normally do.

Right, I forced it on purpose. 

> Make sure any of the transactions you are manually calling DoTransaction()
> on don't make editor calls that use transactions, otherwise your effort will
> have been in vain. :-)

Yes, I figured that. But ChangeCSSInlineStyleTxn is independant of all other
transactions.

Comment 4

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

Attachment: patch v1.1, in answer to Kin's comments

Comment 5

[Kathleen :Brade]

Comment on attachment 98864 [details] [diff] [review]
patch v1.1, in answer to Kin's comments

r=brade

Comment 6

[Kevin McCluskey (gone)]

nsbeta1+, EDITORBASE+

Comment 7

[kinmoz]

Comment on attachment 98864 [details] [diff] [review]
patch v1.1, in answer to Kin's comments

sr=[e-mail]

I forgot to mention that there is another way to prevent a transaction from
being pushed onto the undo stack, and that is to have your transaction return
|PR_TRUE| in it's GetIsTransient() method, though I don't remember what
assumptions the editor makes as you call nsIEditor::Do().

Comment 8

[Joe Francis]

Comment on attachment 98864 [details] [diff] [review]
patch v1.1, in answer to Kin's comments

r=jfrancis

I seee that in the js you now say:

gEditor.setAttributeOrEquivalent(bodyelement, "text", text_color, true);

where you used to say:

bodyelement.setAttribute("text", text_color);

Is that related to this bug or is that a seperate fix?

Comment 9

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

Joe, see above comment #2, item 2.

Comment 10

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

checked in (trunk)

Comment 11

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

Reopening, I missed one set of modifs in nsHTMLEditor.cpp.

Comment 12

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

Attachment: missing changes

r/sr needed please

Comment 13

[kinmoz]

Comment on attachment 99826 [details] [diff] [review]
missing changes

sr=[e-mail]

Comment 14

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

checked in

Comment 15

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.02.19 on linux rh8.0, mac 10.2.4 and win2k.