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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Dave Barrowman]

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.:
line1
line2
line3

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

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.

Comment 1

[Dave Barrowman]

Attachment: html example showing outdent bug

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.

Comment 2

[Michael Buckland]

Discussed in edt.  topembed plussing.  Also editorbase plussing based on saari's
direction.

Comment 3

[Kevin McCluskey (gone)]

Nisheeth is this something you could help with?

Comment 4

[Joe Francis]

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

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.

Comment 5

[Nisheeth Ranjan]

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

Comment 6

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Taking bug.

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix #1

Comment 8

[Kai Engert (:KaiE:)]

The first thing that confuses me in this patch:

You define variable curBlockQuoteIsIndentedWithCSS, and you even test its value
using
  if (curBlockQuoteIsIndentedWithCSS)
but you are not assigning a value to it before your first test!

Comment 9

[Kai Engert (:KaiE:)]

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 10

[Kai Engert (:KaiE:)]

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

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

kaie: thanks for review, new patch pending.

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix #2 in answer to kaie

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 125401 [details] [diff] [review]
fix #2 in answer to kaie

kaie said in #composer r=kaie
kin, can you sr please ?

Comment 14

[kinmoz]

Comment on attachment 125401 [details] [diff] [review]
fix #2 in answer to kaie

sr=[e-mail]

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.

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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 ?

Comment 16

[Aleksey Nogin]

This checkin have added a warning on brad TBox:

+editor/libeditor/html/nsHTMLEditRules.cpp:3836
+ `PRBool curBlockQuoteIsIndentedWithCSS' might be used uninitialized in this
function

Comment 17

[John G. Myers]

This can be resolved FIXED, no?  The 1.4 branch is not particularly relevant
anymore.

Comment 18

[Phil Ringnalda (:philor)]

Ayuh.