54222 - The "noshade" attribute on Horizontal rule not set at all

Status: VERIFIED FIXED

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

Description

[Nasiruddin M.S]

Build id : 2000090720
platform : Solaris2.8(intel)

(1)open the composer
(2) insert a horizontal line using the icon.
(3) right click with mouse pointer on horizontal line, and look into the
horizontal line properties.
(4) change the width ,height and in alignment check 'center'. press o.k.
(5) Now again try to look into the horizontal line properties.
    It will not show the current alignment and 3d-shading for horizontal line.

This works fine on netscape4.x browsers.

Comment 1

[anthonyd]

confirmed, but moving to future and assigning to cmanske.

anthonyd

Comment 2

[rubydoo123]

ok, I tested this a bit. on win98 this is working fine, the properties sheet is
updated and reselecting the props sheet displays the current state.

asking sujay to test on mac, linux and solaris to see where the disparity lies.

Comment 3

[Charles Manske]

There cannot be platform differences for this.
The "noshade" attribute is not written correctly to the <hr> node on all 
platforms.
The reason is because "noshade" doesn't have a value, and the "CloneAttributes"
method that copies edited attributes misses "noshade"
The fix is very easy - I'll attach it.

Comment 4

[Charles Manske]

Attachment: Set the "noshade" attribute on the <hr> element

Comment 5

[Charles Manske]

Nominating for rtm fix since it is extremely simple and safe, and without it
the attribute can't be set at all.

Comment 6

[Charles Manske]

Reassigning to Kathy since I'm starting sabatical soon.

Comment 7

[Charles Manske]

Reviews have been requested. Note that this is a "low risk/high visibility"
bug.

Comment 8

[Charles Manske]

Attachment: Much better fix to copy empty attribute values

Comment 9

[Charles Manske]

Kin suggested a much better fix: Change "CloneAttributes" to detect empty
values and set them on the target element. This will work for *all* similar
cases, not just for "noshade" on the <hr>. One that come to mind is the "border" 
attribute on <table>, which may also be empty.
Reassigning the bug to kin so he can checkin if approved.

Comment 10

[Kathleen :Brade]

note: there is a bug with rendering hrules with noshade that are too short;
buster currently has that bug and a fix is attached to it.

Comment 11

[kinmoz]

The 2nd patch will not work. The implementation of nsDOMAttribute::GetValue() 
only returns NS_OK as a result and a string that is either empty or contains the 
value.

You either need to QI the node to an nsIContent and call GetAttribute(), which 
will return the nsresult you are looking for, which is what 
nsDOMAttribute::GetValue does, or figure out if presence of an attribute in an 
nsIDOMNamedNodeMap indicates that the attribute is set.

Comment 12

[kinmoz]

Accepting bug.

Comment 13

[Charles Manske]

It's not as simple as using the nsIContent interface instead because of the 
way we set the attributes on the temporary element in the dialog.
Thus this needs more investigation to fix in CloneAttributes.
The first patch, which definitely and very simply fixes the specific "noshade"
attribute problem, should be used for the rtm fix.
Very low risk, completely understood behavior.

Comment 14

[Charles Manske]

Attachment: Alternate simple fix for CloneAttributes that sets attribute value even if empty

Comment 15

[rubydoo123]

Kin, please include the required information per the rtm checkin rules, please 
review the patch that charley attached on 9/29

Comment 16

[rubydoo123]

Kin, this needs super review and module owner approval, once those comments are 
in, please remove the need info and ping me

Comment 17

[kinmoz]

Reassigning to [e-mail] per [e-mail].

Comment 18

[kinmoz]

Adding [e-mail] to Cc list.

Comment 19

[anthonyd]

okay, I tested the fix kin and I talked about and that seems to solve the 
problem here. gonna put an attachment on this report with the final patch.

anthonyd

Comment 20

[anthonyd]

Attachment: this is the patch that kin and I discussed for this bug.

Comment 21

[anthonyd]

okay, so that is pretty much it.  we still need to add some comments in to 
discuss what is going on so people will understand.

anthonyd

Comment 22

[kinmoz]

sr=[e-mail]

As anthonyd stated above, he will checkin in the change with a comment that
states that existence of an attribute in the named node map implies that it has
been set on the source, even if it has no value.

Comment 23

[anthonyd]

Attachment: patch with fix AND comment explaining what is going on.

Comment 24

[anthonyd]

patch attached
sr=kin
r=mjudge
a=mjudge

requesting rtm++

anthonyd

Comment 25

[anthonyd]

new comment for bug:
Presence of an attribute in the named node map indicates that it was set on the 
element even if it has no value.

anthonyd

Comment 26

[Phil Peterson]

rtm++

Comment 27

[anthonyd]

checked in fix on branch and trunk.

anthonyd

Comment 28

[sujay]

verified in 10/10 build, however found two other problems with H. Line props
dialog....

Comment 29

[Nasiruddin M.S]

Tested again on the latest netscape 6 build (Build id:2000102302) on
solaris2.8(sparc). The problem is still there. The horizontal line properties
menu still shows the default value for 3d-shading and alignment.  

Comment 30

[Kathleen :Brade]

[e-mail]--see bug #57298 for a problem with alignment 
controls in the hrule dialog.  If you see other problems, could you please 
clarify with a set of steps on how to reproduce?

Thanks!

Comment 31

[Nasiruddin M.S]

That's fine.