Closed Bug 468353 Opened 14 years ago Closed 14 years ago

designmode.css is not removed after designMode is turned off

Categories

(Core :: DOM: Editor, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: mikekap, Assigned: mikekap)

References

Details

Attachments

(1 file, 1 obsolete file)

Offshoot from bug 440614. That may take longer and it's really annoying to have this in the diffs.
Attached patch Patch v1 from 440614 (obsolete) — Splinter Review
Attachment #351809 - Flags: superreview?(bzbarsky)
Attachment #351809 - Flags: superreview?(bzbarsky)
Attachment #351809 - Flags: superreview+
Attachment #351809 - Flags: review+
Comment on attachment 351809 [details] [diff] [review]
Patch v1 from 440614

Looks good; let's do it.
Mike, do you think this is worth taking on the 1.9.1 branch?
Keywords: checkin-needed
(In reply to comment #3)
> Mike, do you think this is worth taking on the 1.9.1 branch?

There's no harm in doing it, but since the editor normally removes the override stylesheets on destruction, you can't reproduce this bug without seriously trying. So to answer your question, probably not.

I actually just found another occurrence at:
http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#3367 &&
http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#3407

This one might be a little more serious, since the case it deals with is "designMode is being turned off (contentEditable is still on)."

I'll attach a patch in a sec.
I didn't have time to test this yet, but I know the included test passes with the patch applied.

If this does manage to break something, I would be very surprised.
Attachment #351809 - Attachment is obsolete: true
Attachment #351835 - Flags: review?(bzbarsky)
The spellRecheckAll change looks wrong, no?
(In reply to comment #6)
> The spellRecheckAll change looks wrong, no?

I'm pretty sure that its still in the block with nsAutoEditingState. Doesn't really make sense as it is now.
Hmm.  That's true, but are we trying to sync when turning off or turning on?  It sorta doesn't make sense no matter how you slice it, given the early return for turning editing off.

Maybe have whoever wrote or reviewed this code review that part?  Or at least peterv?
Comment on attachment 351835 [details] [diff] [review]
Patch v2, with new find
[Checkin: Comment 12]

(In reply to comment #8)
> Hmm.  That's true, but are we trying to sync when turning off or turning on? 
> It sorta doesn't make sense no matter how you slice it, given the early return
> for turning editing off.
> 
> Maybe have whoever wrote or reviewed this code review that part?  Or at least
> peterv?

The resync is done when editing type is switched (when just contentEditable changes to just designMode+contentEditable).
Attachment #351835 - Flags: review?(bzbarsky) → superreview?(peterv)
Attachment #351835 - Flags: superreview?(peterv) → superreview+
Comment on attachment 351835 [details] [diff] [review]
Patch v2, with new find
[Checkin: Comment 12]

The spellRecheckAll should be true when going from contentEditable -> designMode (designMode overrides contentEditable), so |designMode && oldState == eContentEditable|. Looks correct to me.
And I think we should take this on 1.9.1.
No longer blocks: 454114
Keywords: checkin-needed
Attachment #351835 - Attachment description: Patch v2, with new find → Patch v2, with new find [Checkin: Comment 12]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.