83335 - hrule align attribute defaults to left not center

Status: VERIFIED FIXED

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

Description

[Kathleen :Brade]

The hrule align attribute is always written out.  We shouldn't write it out if 
it's the default value (center).

Comment 1

[Charles Manske]

easy.

Comment 2

[Charles Manske]

To simplify, I'm going to fix all simple HLine problems with this bug.
This is another simple problem:
Start Composer
Insert a Horizontal Line from H.Line toolbar button.
Double click on it to edit properties. You can set the alignmen via radio
button. Set "Center"
Click on Advanced Edit button and change alignment to "right". Click OK.
When you return to H.Line dialog, both "Center" and "Right" are checked.
Simple fix -- just clear all 3 buttons before getting current align state.

Comment 3

[Charles Manske]

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

Comment 4

[Charles Manske]

Attachment: Fixes all problems with HLine align attribute

Comment 5

[Charles Manske]

Note that patch also fixes other places where we were writing the "align"
attribute as "center" when we don't need to.

Comment 6

[Charles Manske]

Attachment: Updated patch (last one wasn't the most recent)

Comment 7

[Kathleen :Brade]

* why did you remove the ';' from the end of this line in ComposerCommands.js:
  hLine.setAttribute("align", "right")
* in nsHTMLEditor.cpp, I don't understand why you removed that line and not the 
others; maybe add a comment in there so someone doesn't add it back in?
* I don't understand why we need to set all of the radio buttons' checked to 
false.  Do we need to do that anywhere else?
* why change the code that sets checked to true?
* can we remove all or most of the dumps in this file?

Comment 8

[Charles Manske]

* why did you remove the ';' from the end of this line in ComposerCommands.js:
  hLine.setAttribute("align", "right")
An accident! Fixed.

* in nsHTMLEditor.cpp, I don't understand why you removed that line and not the
others; maybe add a comment in there so someone doesn't add it back in?

We set attributes (not many now) that we really want, but
as you observed, it isn't necessary to set "center" for HLine, since it's default.

* I don't understand why we need to set all of the radio buttons' checked to
false.  Do we need to do that anywhere else?
I wondered about that too. The only other example I could find using radio
buttons for an element props dialog that accessed Advanced Edit was in Image
dialog, and setting checks is done correctly there. This seems to be only
example with 3 radio buttons.

* why change the code that sets checked to true?
Diff is confusing. Changed to was use "center" as default

* can we remove all or most of the dumps in this file?
Sure. Done.

Comment 9

[Charles Manske]

Attachment: Updated patch in response to brade's comments

Comment 10

[kinmoz]

Changes look good to me ... sr=[e-mail]

Not required:

Would it be better to do the following instead of clearing the checked values 
first then checking which is set?

    dialog.centerAlign.checked = (align == "center");
    dialog.rightAlign.checked  = (align == "right");
    dialog.leftAlign.checked   = (align == "left");

Comment 11

[Charles Manske]

Kin's suggestion was good, but must also check null:
  dialog.centerAlign.checked = (align == "center" || !align);
  dialog.rightAlign.checked  = (align == "right");
  dialog.leftAlign.checked   = (align == "left");

This is the new version.

sr=kin

Comment 12

[Charles Manske]

r=sfraser

Comment 13

[Charles Manske]

Attachment: Final patch with changes suggested by reviewers.

Comment 14

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 15

[Charles Manske]

checked in.

Comment 16

[rubydoo123]

updated summary to reflect the remaining issue from this bug, when insert an 
hrule, the default align value being set is left, the default should be center, 
and should be written out for correctness. See section 15.3

Comment 17

[rubydoo123]

trivial to fix, one line correction, and the horizontal rule will insert correctly

reviewed and approved

Comment 18

[Charles Manske]

The point of the orginal bug was to *not* explicitly write the 'center'
attribute in the <hrule> tag. I don't think there's a bug here.
The default value is saved in preferences, so if you ever used align = "left"
and clicked on the "Use as default" button in the H.Line properties dialog, the
pref would be saved and used as new default. If no "align" attribute
is set, layout will default to center. So insert a new H.Line, click on "center"
radio button, then click on "Use as default" to clear the pref.
The next line you insert should have no "align" set on the <hrule>, and should
display centered. Close the app and restart, then insert a new line in Composer
to see if you are still getting the default "center" behavior.

Comment 19

[Kathleen :Brade]

reopen bug for different resolution

Comment 20

[Kathleen :Brade]

original bug reported is fixed

Comment 21

[sujay]

Okay two issues here that I see now after inserting an H. Line:

1) H. Line default = Left. 
2) after changing alignment to Center when you look at the source, we don't
write out the alignment for Center.

If the above are correct, then I am marking this VERIFIED-FIXED.

otherwise REOPEN.

Comment 22

[Charles Manske]

Are you sure the H.Line default is "left"? Did you delete your prefs first?
The default should be to not put any "align" value, thus layout will center
it.

Comment 23

[sujay]

I"m seeing an alignment value of "left".

What do you mean by delete my prefs? my prefs.js file?

Comment 24

[Charles Manske]

Yes, delete prefs.js

Comment 25

[sujay]

Leaving this bug as closed out...

Charley recommends filing a new bug on the default align = left issue.