Closed
Bug 468353
Opened 16 years ago
Closed 16 years ago
designmode.css is not removed after designMode is turned off
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: mikekap, Assigned: mikekap)
References
Details
Attachments
(1 file, 1 obsolete file)
6.78 KB,
patch
|
peterv
:
superreview+
|
Details | Diff | Splinter Review |
Offshoot from bug 440614. That may take longer and it's really annoying to have this in the diffs.
![]() |
||
Updated•16 years ago
|
Attachment #351809 -
Flags: superreview?(bzbarsky)
Attachment #351809 -
Flags: superreview+
Attachment #351809 -
Flags: review+
![]() |
||
Comment 2•16 years ago
|
||
Comment on attachment 351809 [details] [diff] [review] Patch v1 from 440614 Looks good; let's do it.
![]() |
||
Comment 3•16 years ago
|
||
Mike, do you think this is worth taking on the 1.9.1 branch?
Keywords: checkin-needed
Assignee | ||
Comment 4•16 years ago
|
||
(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.
Assignee | ||
Comment 5•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•16 years ago
|
||
(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•16 years ago
|
||
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?
Assignee | ||
Comment 9•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #351835 -
Flags: superreview?(peterv) → superreview+
Comment 10•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
No longer blocks: 454114
Keywords: checkin-needed
Updated•16 years ago
|
Blocks: 454114
Keywords: checkin-needed
Updated•16 years ago
|
Attachment #351835 -
Attachment description: Patch v2, with new find → Patch v2, with new find
[Checkin: Comment 12]
Comment 12•16 years ago
|
||
Comment on attachment 351835 [details] [diff] [review] Patch v2, with new find [Checkin: Comment 12] http://hg.mozilla.org/mozilla-central/rev/db7d1a86b94a
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 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.
Description
•