Cleanup in nsTextEditRules

RESOLVED FIXED in mozilla12

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
Created attachment 586652 [details] [diff] [review]
Part a: DidUndo
Attachment #586652 - Flags: review?(ehsan)
(Assignee)

Comment 1

6 years ago
Created attachment 586653 [details] [diff] [review]
Part b: RemoveRedundantTrailingBR
Attachment #586653 - Flags: review?(ehsan)
Attachment #586652 - Flags: review?(ehsan) → review+
Comment on attachment 586653 [details] [diff] [review]
Part b: RemoveRedundantTrailingBR

Review of attachment 586653 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/text/nsTextEditRules.cpp
@@ +1077,4 @@
>  
> +  // Rather than deleting this node from the DOM tree we should instead
> +  // morph this br into the bogus node
> +  elem->UnsetAttr(kNameSpaceID_None, nsGkAtoms::type, /* ? */ true);

Why the doubt?  :-)
Attachment #586653 - Flags: review?(ehsan) → review+
(Assignee)

Comment 3

6 years ago
Because the SetAttr right below it doesn't notify
(In reply to Ms2ger from comment #3)
> Because the SetAttr right below it doesn't notify

Sure it does: <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#2575>
(Assignee)

Comment 5

6 years ago
Comment on attachment 586653 [details] [diff] [review]
Part b: RemoveRedundantTrailingBR

Yeah, the current code does notify, but I was wondering about this:

>+  // give it the bogus node attribute
>+  elem->SetAttr(kNameSpaceID_None, kMOZEditorBogusNodeAttrAtom,
>+                kMOZEditorBogusNodeValue, false);
Yeah.  I don't know if we specifically _need_ to notify for removing the attribute (I think we don't) but I'd prefer this patch to not change that behavior.  I would be more than happy to review another patch in a new bug which turns off the notification.  That way we can deal with any possible future regressions better.  :-)
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/581a26455703
https://hg.mozilla.org/mozilla-central/rev/034af96fa579
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.