Closed Bug 194963 Opened 18 years ago Closed 12 years ago

cannot outdent single line when line is one of mutliple lines indented together


(Core :: DOM: Editor, defect)

Windows XP
Not set





(Reporter: barrowma, Assigned: glazou)


(Keywords: topembed+, Whiteboard: EDITORBASE+)


(2 files, 1 obsolete file)

Outdenting seems to be broken.  Not sure if this is a regression but I have not
noticed it before.

To reproduce:
1. Enter several lines of text - e.g.:

2. Indent a couple of the lines - e.g.:

3. Now try to outdent one of the indented lines; nothing happens for me.  This
seems like a bug, as I expect the particular line to be outdented (independent
of the other indented lines).

4. Go to your last indented line and hit enter; a new blank indented line
appears, but the outdent button is greyed out.  This seems like a bug to me - I
should be able to outdent the blank line.

5. Type something in the blank line and the outdent button will become
clickable, but clicking on it does nothing.  This is the same bug as in step 3.
Open this attachment in the editor.  
1. Try outdenting line 2 or line 3.  Does nothing for me.
2. Place the caret on the line below line 3.  Note that the outdent button is
greyed out.

If you carefully select everything which is part of the indentation (i.e., from
the beginning of line 2 to the end of the blank line below line 3), you can
outdent it as a whole, but you cannot outdent any subset of it.
Keywords: topembed
Whiteboard: EDITORBASE
Discussed in edt.  topembed plussing.  Also editorbase plussing based on saari's
Keywords: topembedtopembed+
QA Contact: sujay → beppe
Nisheeth is this something you could help with?
Target Milestone: --- → mozilla1.5beta
Looking at this I see that it's a problem when editing in css mode.  If you edit
a new document in html (non-css) mode, and create a similar testcase by
indenting some lines, then outdenting works.

If you try the testcase here in non-css mode, it doesn't work, but that's not a
bug per se, since non-css mode is not supposed to recognize css indention.  

I don't think this is related to bug 113514, which is something I speculated on

My guess is that the problem lies in
nsHTMLEditRules::RelativeChangeIndentation().  Daniel Glazman wrote this so he
may know more, but I think the problem is that this routine assumes that only
elements will be passed in, and in the testcase I bet a textnode is passed in.
I looked into this more and Joe, you are exactly right.  The problem is that a
text node is passed into nsHTMLEditRules::RelativeChangeIndentation() which QIs
the incoming nsIDOMNode to a nsIDOMElement before operating on it.

CCing Daniel...
Taking bug.
Assignee: jfrancis → glazman
Attached patch fix #1 (obsolete) — Splinter Review
Attachment #125307 - Flags: superreview?(kin)
Attachment #125307 - Flags: review?(kaie)
The first thing that confuses me in this patch:

You define variable curBlockQuoteIsIndentedWithCSS, and you even test its value
  if (curBlockQuoteIsIndentedWithCSS)
but you are not assigning a value to it before your first test!
I realize that curBlockQuoteIsIndentedWithCSS is only accessed when
curBlockQuote is true, so the access to uninitialized memory doesn't happen. I
feel better in general when every variable receives a safe initial value, so I'd
request to init curBlockQuoteIsIndentedWithCSS with PR_FALSE:

Could you move the common code if(curBlockQuote){}, that is currently duplicated
4 times, into a common helper method? When doing so, instead of using a function
with many arguments, could it make sense to use a small helper class that
carries all the various variables that play together. That would make it more
obvious that  curBlockQuote and curBlockQuoteIsIndentedWithCSS play together.
(curBlockQuote, curBlockQuoteIsIndentedWithCSS, firstBQChild, lastBQChild, ...)?

But I'm fine with a function that takes multiple parameters. I think it's better
to use such a function and have the code only once, makes maintaining easier.

After having understood the above and looked at the rest of this function, I
think I understand what you are doing and it seems reasonable.

Trying to summarize, nsHTMLEditRules::WillOutdent() was not correctly aware of
CSS indented blocks. We did not discover the node that is responsible for the
current indendation. As a result, if the node-to-outdent was not a list item,
RelativeChangeIndentation was called on the text-node-indent-by-style, but that
method was unable to do it.

(Note: I wrote this text during the process of trying to understand the code.
Please feel free to provide some sentences of explanation in the future, it will
help me spend less time on reviewing.)

To summarize: I'm fine with this patch, except the code fragment that ges
duplicated 4 times.
Comment on attachment 125307 [details] [diff] [review]
fix #1

review denied on this patch

attach a new patch that combines the repeated code into a function, and you
have r=kaie
Attachment #125307 - Flags: superreview?(kin)
Attachment #125307 - Flags: review?(kaie)
Attachment #125307 - Flags: review-
kaie: thanks for review, new patch pending.
Attachment #125307 - Attachment is obsolete: true
Comment on attachment 125401 [details] [diff] [review]
fix #2 in answer to kaie

kaie said in #composer r=kaie
kin, can you sr please ?
Attachment #125401 - Flags: superreview?(kin)
Attachment #125401 - Flags: review+
Comment on attachment 125401 [details] [diff] [review]
fix #2 in answer to kaie

It kind of bugs me that we duplicate the code to "finish up" handling of
curBlockQuote at the top of the |for| loop like 3 times ... at some point other
 point in time, we should figure out if there is a way to  restructure	that
logic so that the duplication isn't necessary.
Attachment #125401 - Flags: superreview?(kin) → superreview+
checked in (trunk).

I am not closing the bug since Kevin McCluskey may need this in the
1.4 branch too given the topembed+ keyword. Kevin ?
This checkin have added a warning on brad TBox:

+ `PRBool curBlockQuoteIsIndentedWithCSS' might be used uninitialized in this
This can be resolved FIXED, no?  The 1.4 branch is not particularly relevant
QA Contact: rubydoo123 → editor
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.