removeAttribute doesn't work in HTML

VERIFIED FIXED in mozilla1.0

Status

()

Core
Editor
P1
critical
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: sicking)

Tracking

({regression, topembed})

Trunk
mozilla1.0
x86
Windows 98
regression, topembed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [EDITORBASE][ADT1])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

16 years ago
Using Build ID: 2002041103
Steps to reproduce problem:
1. Open the JavaScript console
2. Evaluate this:
with (document.createElement('body')) { setAttribute('text', '#FFFFFF');
removeAttribute('text'); getAttribute('text'); }
3. Evaluate using top.document (i.e. a XUL document)

Expected results: null and null

Actual results: #FFFFFF and null

Additional information:
I think that this blocks several Editor dialogs, such as Page Colours,
because it is impossible to remove existing colours.
(Reporter)

Updated

16 years ago
Keywords: regression
(Reporter)

Updated

16 years ago
Blocks: 137401
The testcase worksforme, linux build 2002-04-12-21...

Does this happen when actually evaluating things in an HTML document (as opposed
to in the wacky environment of the JS console?)
(Reporter)

Comment 3

16 years ago
Actually it was in the context of bug 137401, I was trying to removeAttribute
from a node created by/cloned from an editor document.
(Reporter)

Comment 4

16 years ago
Perhaps the bug only applies to Editor, so I'll reassign it.

One of a possible number of side-effects is that you can no longer use the page
colours and background dialog to turn off page colours and background, so I'm
adding the mozilla1.0 keyword and setting the severity to critical.
Assignee: jst → kin
Severity: major → critical
Component: DOM Core → Editor: Core
Keywords: mozilla1.0
QA Contact: stummala → sujay

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.0

Comment 5

16 years ago
So Neil told me how to reproduce the problem in composer:

1. Startup composer
2. Bring up the Format->PageColorsAndBackground dialog.
3. Click the "Use Custom Colors" radio button.
4. Change the "Normal Text" color to something.
5. Press the OK button.
6. Bring the dialog back up again.
7. Click the "Reader's default colors" radio button.
8. Press the OK button.

At this point the <body> tag should not have any attributes set on it.

Comment 6

16 years ago
Looks like sicking@bigfoot.com's checkin for bug 41983:

    mozilla/content/html/content/src/nsGenericHTMLElement.cpp revision 1.340

    Make (x)html-elements support namespaced attributes.)

    r=heikki sr=jst a=roc+moz



made it so that you can no longer remove attributes from nodes that aren't in a
document.

This bug also exists on the MOZILLA_1_0_0_BRANCH and significantly affects
Composer dialogs.
Assignee: kin → sicking
Status: ASSIGNED → NEW
Keywords: nsbeta1, topembed

Updated

16 years ago
Whiteboard: EDITORBASE

Comment 7

16 years ago
Cc jfrancis and glazman just in case this impacts editor core.

Comment 8

16 years ago
This prevents Composer from being able to remove attributes in the Color Picker,
Image, Horizontal Line, Image, List, and all of the Form element dialogs.
We can set attributes for an element that is not contained in a document (this
is a cloned node we use [globalElement] so we don't set attributes in realy 
document until user clicks "Ok"), so we must also be able to remove attributes
as well.
This is a very significant change in behavior!
Keywords: nsbeta1 → nsbeta1+
Whiteboard: EDITORBASE → [EDITORBASE][ADT1]
i think i have a fix, building with it now
wow !!! that's a serious regression ! Jonas, you should work on that
super-high priority please.
Created attachment 80753 [details] [diff] [review]
patch to fix

sorry guys :(

The problem is the |&& sheet| in the |if| which shouldn't be there. I removed
it and moved the variable to inside the |if| since it's only used there.

(obsoleting testcase since it's not what the bug is about)
Attachment #79172 - Attachment is obsolete: true
*great* thanks to kin for excellent work on detecting what the problem was! 
That directed me to the spot right away.
Status: NEW → ASSIGNED
Comment on attachment 80753 [details] [diff] [review]
patch to fix

r=glazman
Attachment #80753 - Flags: review+
Comment on attachment 80753 [details] [diff] [review]
patch to fix

sr=jst
Attachment #80753 - Flags: superreview+
Comment on attachment 80753 [details] [diff] [review]
patch to fix

a=rjesup@wgate.com for branch checkin (make sure it's in trunk too)
Attachment #80753 - Flags: approval+
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
...on both trunk and branch
Jonas, when you check in on the branch, please add keyword "fixed1.0.0" keyword.
That makes it a lot easier to track what is fixed where. I added it now.

Thanks for quick fix.
Keywords: fixed1.0.0
*** Bug 140010 has been marked as a duplicate of this bug. ***

Comment 20

16 years ago
verified in 4/25 branch build.
Keywords: verified1.0.0

Comment 21

16 years ago
verified in 4/26 trunk build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.