hrule align attribute defaults to left not center

VERIFIED FIXED in mozilla0.9.3

Status

()

P3
minor
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: Brade, Assigned: cmanske)

Tracking

Trunk
mozilla0.9.3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

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

Comment 1

18 years ago
easy.
Status: NEW → ASSIGNED
Hardware: Macintosh → All
Target Milestone: --- → mozilla0.9.2
(Assignee)

Comment 2

18 years ago
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.
(Assignee)

Comment 3

18 years ago
*** Bug 83483 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 4

18 years ago
Created attachment 36686 [details] [diff] [review]
Fixes all problems with HLine align attribute
(Assignee)

Comment 5

18 years ago
Note that patch also fixes other places where we were writing the "align"
attribute as "center" when we don't need to.
Keywords: correctness, patch, review
Whiteboard: FIX IN HAND need r=, sr=
(Assignee)

Comment 6

18 years ago
Created attachment 36689 [details] [diff] [review]
Updated patch (last one wasn't the most recent)
(Reporter)

Comment 7

18 years ago
* 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?
(Assignee)

Comment 8

18 years ago
* 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.

(Assignee)

Comment 9

18 years ago
Created attachment 36738 [details] [diff] [review]
Updated patch in response to brade's comments

Comment 10

18 years ago
Changes look good to me ... sr=kin@netscape.com

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");

Whiteboard: FIX IN HAND need r=, sr= → FIX IN HAND need r=
(Assignee)

Comment 11

18 years ago
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
(Assignee)

Comment 12

18 years ago
r=sfraser
Whiteboard: FIX IN HAND need r= → FIX IN HAND
(Assignee)

Comment 13

18 years ago
Created attachment 37446 [details] [diff] [review]
Final patch with changes suggested by reviewers.

Updated

18 years ago
Blocks: 83989

Comment 14

18 years ago
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)

Updated

18 years ago
Whiteboard: FIX IN HAND → fixed, reviewed, a=asa
(Assignee)

Comment 15

18 years ago
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Keywords: patch, review
Resolution: --- → FIXED
Whiteboard: fixed, reviewed, a=asa → fixed

Comment 16

18 years ago
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
Status: RESOLVED → REOPENED
OS: Mac System 9.x → All
Resolution: FIXED → ---
Summary: hrule align attribute problems → hrule align attribute defaults to left not center

Updated

18 years ago
Severity: normal → minor
Priority: -- → P3
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 17

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

reviewed and approved
Keywords: nsBranch
Whiteboard: fixed
(Assignee)

Comment 18

18 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → INVALID
(Reporter)

Comment 19

18 years ago
reopen bug for different resolution
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Reporter)

Comment 20

18 years ago
original bug reported is fixed
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 21

18 years ago
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.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 22

18 years ago
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

18 years ago
I"m seeing an alignment value of "left".

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

Comment 24

18 years ago
Yes, delete prefs.js

Comment 25

18 years ago
Leaving this bug as closed out...

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

You need to log in before you can comment on or make changes to this bug.