468353 - designmode.css is not removed after designMode is turned off

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Offshoot from bug 440614. That may take longer and it's really annoying to have this in the diffs.

Comment 1

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v1 from 440614

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 351809 [details] [diff] [review]
Patch v1 from 440614

Looks good; let's do it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Mike, do you think this is worth taking on the 1.9.1 branch?

Comment 4

[Mike Kaplinskiy [:mikekap] (at school, no time)]

(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.

Comment 5

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v2, with new find [Checkin: Comment 12]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The spellRecheckAll change looks wrong, no?

Comment 7

[Mike Kaplinskiy [:mikekap] (at school, no time)]

(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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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 9

[Mike Kaplinskiy [:mikekap] (at school, no time)]

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).

Comment 10

[Peter Van der Beken [:peterv]]

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.

Comment 11

[Peter Van der Beken [:peterv]]

And I think we should take this on 1.9.1.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 351835 [details] [diff] [review]
Patch v2, with new find
[Checkin: Comment 12]


http://hg.mozilla.org/mozilla-central/rev/db7d1a86b94a