Open Bug 772332 Opened 12 years ago Updated 3 years ago

Cursor jumps to wrong position when deleting spaces between a list and paragraph

Categories

(Core :: DOM: Editor, defect, P5)

Product:
Core
Component:
DOM: Editor
Version:
8 Branch
Type:
defect

Tracking

()

People

(Reporter: samjclarke, Unassigned)

References

Details

(Keywords: regression, Whiteboard: [Leave open])

Attachments

(4 files, 1 obsolete file)

ListBug.html
224 bytes, text/html
Details
Patch for bug 775552 -- Don't access siblings of nodes that might be deleted
1.21 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 1, v2 -- Make nsWSRunObject's type enum type-safe
71.45 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Interdiff for patch part 1
8.48 KB, patch
Details | Diff | Splinter Review
Attached file ListBug.html — 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120615112143

Steps to reproduce:

1. Place cursor after last item and hit enter twice to create a new line
2. Press space a few times then enter to go to a new line
3. Type some text then place the cursor at the start of the text
4. Press backspace so the spaces and new lines get deleted

Video demonstration: http://dl.dropbox.com/u/56830471/ListBug.ogv


Actual results:

Cursor jumps to the end of the entered text


Expected results:

Cursor should stay between last list item and the entered text
OS: Linux → All
Attachment #640491 - Attachment mime type: text/plain → text/html
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/b2db7864c8a8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110722 Firefox/8.0a1 ID:20110722065900
Bad:
http://hg.mozilla.org/mozilla-central/rev/c7985dc6737f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110722 Firefox/8.0a1 ID:20110722125150
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b2db7864c8a8&tochange=c7985dc6737f

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/037dee4ee39f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110722 Firefox/8.0a1 ID:20110722063620
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9ff13e309d9e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110722 Firefox/8.0a1 ID:20110722093810
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b6514fd7c26d&tochange=9ff13e309d9e

Suspected:
e21430d2b2cb	Fabien Cazenave — Bug 449243 - contentEditable: insert <p> instead of <br>; r=ehsan In editable elements, create a paragraph instead of a <br> node when [Return] is pressed: * once in a header node (<h[1..6]>); * twice in a list item node (<li>).
Blocks: 449243
Status: UNCONFIRMED → NEW
Component: Untriaged → Editor
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 13 Branch → 8 Branch
Aryeh, can you please take a look at this?
Self-contained test-case:

data:text/html,<!DOCTYPE html>
<div contenteditable>
<ul><li>foo</ul>
<p><br></p>
</div>
<script>
getSelection().collapse(document.querySelector("p"), 0);
document.execCommand("delete");
document.execCommand("inserttext", false, "x");
</script>

Should be "foox" on one line; that's true in Chrome.  In Gecko, "x" is on its own line.  The backspacing doesn't put the cursor in the right place.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
So I started to look at this, but nsHTMLEditRules::WillDeleteSelection is just a colossal mess.  I started work on rewriting it -- I've wanted to do that for a while.  So far my patch compiles, but I haven't gotten it to pass crashtests yet . . .

(On reflection, in the future I should probably break long functions like this up into smaller pieces before trying to clean them up.  It's way too much pain to do one all at once, but it's hard to clean up a large function only partially because of variables being shared.)
Unrelated to this bug -- I just saw this as I looked through the file.  It probably fixes a regression from bug 755264, but I have no test case.  It's basically the same as bug 767684; the child might be removed from its parent by the loop body, so setting it to GetPreviousNode() after the fact isn't correct.
Attachment #641763 - Flags: review?(ehsan)
This is, again, an unrelated cleanup patch.  It touches a bunch of code, so I'd prefer to get it committed sooner rather than later.  I'm not sure the approach is right -- this is the simplest way I could see to get type safety here, but it seems relatively elaborate.

I'm also just not sure if I actually implemented the operator overloads and such correctly.  I think WSType in this patch can be implicitly converted to any numeric type via operator bool, which I don't want, but I don't know how to avoid it without using C++11 explicit conversion operators.  Also, I wonder if I really need all the WSType::Enum overloads, or if that can be made more succinct.  So please tell me if I should just rewrite this.  :)

Try for the first two patches: https://tbpl.mozilla.org/?tree=Try&rev=65fb74fab444  I'll continue my cleanup work here when I get a chance, then proceed to fixing the actual bug . . .
Attachment #641771 - Flags: review?(ehsan)
The try run was built on top of a patch for bug 772807 that was busted on Windows, so I made a new try run: https://tbpl.mozilla.org/?tree=Try&rev=0b2c2f561977
Attachment #641763 - Flags: review?(ehsan) → review+
Comment on attachment 641771 [details] [diff] [review]
Patch part 1 -- Make nsWSRunObject's type enum type-safe

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

r=me with the below.

::: editor/libeditor/html/nsWSRunObject.cpp
@@ +919,5 @@
>    // if we are before or after a block (or after a break), and there are no nbsp's,
>    // then it's all non-rendering ws.
> +  if (!mFirstNBSPNode && !mLastNBSPNode &&
> +      (mStartReason & WSType::block || mStartReason == WSType::br ||
> +       mEndReason & WSType::block)) {

Nit: please wrap the ands in parenthesis here and elsewhere in the patch, it's usually not safe to trust people to know operator precedence.  :-)

::: editor/libeditor/html/nsWSRunObject.h
@@ +59,5 @@
> +    thisBlock  = 1 << 7, // indicates the block ws run is in
> +    block      = otherBlock | thisBlock // block found
> +  };
> +
> +  WSType(const Enum& aEnum = none) : mEnum(aEnum) {}

Please make this constructor explicit, and also make it not have a default argument, and then add a default ctor which inits mEnum to none.

@@ +60,5 @@
> +    block      = otherBlock | thisBlock // block found
> +  };
> +
> +  WSType(const Enum& aEnum = none) : mEnum(aEnum) {}
> +  operator bool() const { return mEnum; }

Yeah, this is bad because of the exact reason you said.  You can use this trick instead:

  void bool_conversion_helper() {}
  typedef void (WSType::*bool_type)();
  operator bool_type() const { return mEnum ? &bool_conversion_helper : nsnull; }

This makes it possible for your class to be used in conversions to boolean since member function pointers can be converted into bools implicitly, but prevents integer uplifts since those conversions are not implicit on function pointers.  :-)

@@ +63,5 @@
> +  WSType(const Enum& aEnum = none) : mEnum(aEnum) {}
> +  operator bool() const { return mEnum; }
> +  bool operator==(const WSType& aOther) const { return mEnum == aOther.mEnum; }
> +  bool operator!=(const WSType& aOther) const { return !(*this == aOther); }
> +  WSType operator=(const WSType& aOther) {

Return WSType& here and below, so that things like this would work:

  WSType a, b, c;
  a = b = c;

@@ +80,5 @@
> +  WSType operator&(const WSType& aOther) const
> +  {
> +    WSType ret = *this;
> +    ret &= aOther;
> +    return ret;

This is sort of unreadable!  Please implement it in terms of mEnum.

@@ +87,5 @@
> +  {
> +    WSType ret = *this;
> +    ret |= aOther;
> +    return ret;
> +  }

Ditto.

@@ +97,5 @@
> +// compiler doesn't get confused by the boolean conversion operator (TODO: use
> +// explicit operator bool() when enough compilers support it?)
> +inline bool operator==(const WSType& aLeft, const WSType::Enum& aRight)
> +{
> +  return aLeft == (WSType)aRight;

Nit: use the ctor form here and below.

@@ +143,5 @@
>      };
>  
> +    enum {eBefore = 1};
> +    enum {eAfter  = 1 << 1};
> +    enum {eBoth   = eBefore | eAfter};

Why not put all of this in the same enum?
Attachment #641771 - Flags: review?(ehsan) → review+
(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> Nit: please wrap the ands in parenthesis here and elsewhere in the patch,
> it's usually not safe to trust people to know operator precedence.  :-)

Yeah, I guess bitwise operators are confusing.  :(

> Please make this constructor explicit, and also make it not have a default
> argument, and then add a default ctor which inits mEnum to none.

Hmm . . . mind telling me why?  This means that, for instance, the line MakeSingleWSRun(WSType::normalWS); has to become MakeSingleWSRun(WSType(WSType::normalWS)), and of course I need to define yet more operator overloads.  I was thinking of WSType::Enum as really being a special type of WSType, which I define as an enum because C++ doesn't let you have magic named values for classes -- does it?  Is there a better way to do this?

> @@ +60,5 @@
> > +    block      = otherBlock | thisBlock // block found
> > +  };
> > +
> > +  WSType(const Enum& aEnum = none) : mEnum(aEnum) {}
> > +  operator bool() const { return mEnum; }
> 
> Yeah, this is bad because of the exact reason you said.  You can use this
> trick instead:
> 
>   void bool_conversion_helper() {}
>   typedef void (WSType::*bool_type)();
>   operator bool_type() const { return mEnum ? &bool_conversion_helper :
> nsnull; }
> 
> This makes it possible for your class to be used in conversions to boolean
> since member function pointers can be converted into bools implicitly, but
> prevents integer uplifts since those conversions are not implicit on
> function pointers.  :-)

Won't it be nice when C++11 gives us explicit conversion operators . . . incidentally, I apparently need "&WSType::bool_conversion_helper", or else gcc complains with an error like "ISO C++ forbids taking the address of an unqualified or parenthesized non-static member function to form a pointer to member function."  Fortunately it told me how to fix it.

(When I use this trick, if I still have an implicit constructor, why can't the compiler figure out what operator to use for WSType::Enum == WSType?  Can't it implicitly convert the WSType::Enum to WSType?  If I used regular operator bool, I understand that it gets confused because it can also convert both types to an integer type, but I don't see why there's a problem once WSType can't be converted to an integer.)

> @@ +80,5 @@
> > +  WSType operator&(const WSType& aOther) const
> > +  {
> > +    WSType ret = *this;
> > +    ret &= aOther;
> > +    return ret;
> 
> This is sort of unreadable!  Please implement it in terms of mEnum.

I was deliberately defining it in terms of the &= operator so that it would be correct regardless of how &= was defined, just like I defined != as !(*this == aOther) instead of mEnum != aOther.mEnum.  But I guess in this case the class is so simple that it's not an issue.

> @@ +143,5 @@
> >      };
> >  
> > +    enum {eBefore = 1};
> > +    enum {eAfter  = 1 << 1};
> > +    enum {eBoth   = eBefore | eAfter};
> 
> Why not put all of this in the same enum?

Because that's a totally separate enum, used for directions, not whitespace types.


I'll wait for your response on the explicit-constructor issue before taking the review.
(In reply to :Aryeh Gregor from comment #9)
> > Please make this constructor explicit, and also make it not have a default
> > argument, and then add a default ctor which inits mEnum to none.
> 
> Hmm . . . mind telling me why?  This means that, for instance, the line
> MakeSingleWSRun(WSType::normalWS); has to become
> MakeSingleWSRun(WSType(WSType::normalWS)), and of course I need to define
> yet more operator overloads.  I was thinking of WSType::Enum as really being
> a special type of WSType, which I define as an enum because C++ doesn't let
> you have magic named values for classes -- does it?  Is there a better way
> to do this?

I usually dislike implicit ctors because they sometimes let you construct objects where you don't really mean to do that.  Thinking a bit more about this though, I agree that the whole purpose of WSType here would be to wrap the stuff in Enum, so maybe I'm being over-zealous?  If it causes pain for you in places like MakeSingleWSRun callers, feel free to leave the ctor as is.  :-)

> > @@ +60,5 @@
> > > +    block      = otherBlock | thisBlock // block found
> > > +  };
> > > +
> > > +  WSType(const Enum& aEnum = none) : mEnum(aEnum) {}
> > > +  operator bool() const { return mEnum; }
> > 
> > Yeah, this is bad because of the exact reason you said.  You can use this
> > trick instead:
> > 
> >   void bool_conversion_helper() {}
> >   typedef void (WSType::*bool_type)();
> >   operator bool_type() const { return mEnum ? &bool_conversion_helper :
> > nsnull; }
> > 
> > This makes it possible for your class to be used in conversions to boolean
> > since member function pointers can be converted into bools implicitly, but
> > prevents integer uplifts since those conversions are not implicit on
> > function pointers.  :-)
> 
> Won't it be nice when C++11 gives us explicit conversion operators . . .
> incidentally, I apparently need "&WSType::bool_conversion_helper", or else
> gcc complains with an error like "ISO C++ forbids taking the address of an
> unqualified or parenthesized non-static member function to form a pointer to
> member function."  Fortunately it told me how to fix it.
> 
> (When I use this trick, if I still have an implicit constructor, why can't
> the compiler figure out what operator to use for WSType::Enum == WSType? 
> Can't it implicitly convert the WSType::Enum to WSType?  If I used regular
> operator bool, I understand that it gets confused because it can also
> convert both types to an integer type, but I don't see why there's a problem
> once WSType can't be converted to an integer.)

Think of operator== like this: a == b gets interpreted as a.operator==(b) if a is a class type and defines operator==, and as ::operator==(a, b) otherwise.  In this case, the enum is an integral type, so the compiler tries to see if b can be converted to the same type as a, which in this case is not possible, so it will emit an error.  I don't know enough C++ standard jargon to tell you more exactly why this happens!

FWIW, I don't know how to make every possible combination of this work, other than by implementing all combinations of the binary operators, as you have done.

> > @@ +80,5 @@
> > > +  WSType operator&(const WSType& aOther) const
> > > +  {
> > > +    WSType ret = *this;
> > > +    ret &= aOther;
> > > +    return ret;
> > 
> > This is sort of unreadable!  Please implement it in terms of mEnum.
> 
> I was deliberately defining it in terms of the &= operator so that it would
> be correct regardless of how &= was defined, just like I defined != as
> !(*this == aOther) instead of mEnum != aOther.mEnum.  But I guess in this
> case the class is so simple that it's not an issue.

If anything, I'd choose to implement operator&= in terms of operator&, like:

  return (*this = *this & aOther);

But I don't care much about which one you pick, although my preference would be slightly towards implementing both on top of mEnum, since it should be pretty evident what one needs to do when changing this class.

> > @@ +143,5 @@
> > >      };
> > >  
> > > +    enum {eBefore = 1};
> > > +    enum {eAfter  = 1 << 1};
> > > +    enum {eBoth   = eBefore | eAfter};
> > 
> > Why not put all of this in the same enum?
> 
> Because that's a totally separate enum, used for directions, not whitespace
> types.

Good point!
(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> I usually dislike implicit ctors because they sometimes let you construct
> objects where you don't really mean to do that.  Thinking a bit more about
> this though, I agree that the whole purpose of WSType here would be to wrap
> the stuff in Enum, so maybe I'm being over-zealous?  If it causes pain for
> you in places like MakeSingleWSRun callers, feel free to leave the ctor as
> is.  :-)

It doesn't cause a *lot* of pain with existing code, but I think in this case it makes more sense to have an implicit constructor.  I agree that it usually doesn't make sense to have an implicit constructor from a *built-in* type, because it's easy to get confused.  And an implicit constructor from one class to another is also probably liable to cause confusion, since if it makes sense to implicitly convert from one class to another, it probably should be derived from it.  But IMO, implicit conversion from WSType::Enum to WSType is not going to be a problem, because they're really conceptually the same.

Really, I'd prefer to make WSType::Enum the same type as WSType, with static class members instead of enum values.  I think this might be doable with constexpr, but obviously, our compilers don't all support that yet.  So it's a bit of a hack.

> Think of operator== like this: a == b gets interpreted as a.operator==(b) if
> a is a class type and defines operator==, and as ::operator==(a, b)
> otherwise.  In this case, the enum is an integral type, so the compiler
> tries to see if b can be converted to the same type as a, which in this case
> is not possible, so it will emit an error.  I don't know enough C++ standard
> jargon to tell you more exactly why this happens!
> 
> FWIW, I don't know how to make every possible combination of this work,
> other than by implementing all combinations of the binary operators, as you
> have done.

I think the issue is here:

"""
The set of non-member candidates is the result of the unqualified lookup of operator@ in the context of the expression according to the usual rules for name lookup in unqualified function calls (3.4.2) except that all member functions are ignored. However, if no operand has a class type, only those non-member functions in the lookup set that have a first parameter of type T1 or “reference to (possibly cv-qualified) T1”, when T1 is an enumeration type, or (if there is a right operand) a second parameter of type T2 or “reference to (possibly cv-qualified) T2”, when T2 is an enumeration type, are candidate functions.
"""
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3376.pdf 13.3.1.2(3)

I don't fully understand that, but I think the last sentence says that in a comparison that doesn't involve classes, it will only look at candidate operators that have enum types.  I don't know why it won't look at class types that the enum type can be implicitly converted to.

> If anything, I'd choose to implement operator&= in terms of operator&, like:
> 
>   return (*this = *this & aOther);

The problem with this is that &= then creates a temporary, which it doesn't have to.  If you do it the other way, everything is as efficient as it can be.

> But I don't care much about which one you pick, although my preference would
> be slightly towards implementing both on top of mEnum, since it should be
> pretty evident what one needs to do when changing this class.

Yes, since you point it out, I think that's simplest here.
I made a bunch of changes, so I'd like your re-review.
Attachment #641771 - Attachment is obsolete: true
Attachment #643843 - Flags: review?(ehsan)
In addition to the changes you requested, and adding some comments, I cut down on the number of operator overloads by putting the WSType-WSType operators in the global namespace instead of making them members.  This way, the compiler will allow "WSType::Enum == WSType" etc. using the implicit constructor.  (It won't look for member overloads at all if the lhs is not a class type.)  I also changed the return type of operators like & and | to const.  And my previous patch had a very embarrassing mistake -- I was returning bool instead of WSType from some of the & and | operators.

Try: https://tbpl.mozilla.org/?tree=Try&rev=9bc47141d0cc
Attachment #643843 - Flags: review?(ehsan) → review+
Comment on attachment 641763 [details] [diff] [review]
Patch for bug 775552 -- Don't access siblings of nodes that might be deleted

(This deserves to be its own bug, so I filed bug 775552 and will push it as part of that bug.)
Attachment #641763 - Attachment description: Patch part 1 -- Don't access siblings of nodes that might be deleted → Patch for bug 775552 -- Don't access siblings of nodes that might be deleted
Attachment #643843 - Attachment description: Patch part 2, v2 -- Make nsWSRunObject's type enum type-safe → Patch part 1, v2 -- Make nsWSRunObject's type enum type-safe
Attachment #643846 - Attachment description: Interdiff for patch part 2 → Interdiff for patch part 1
Attachment #641771 - Attachment description: Patch part 2 -- Make nsWSRunObject's type enum type-safe → Patch part 1 -- Make nsWSRunObject's type enum type-safe
https://hg.mozilla.org/integration/mozilla-inbound/rev/89758fae5f2d

(Deliberately not setting target milestone yet.)
Hardware: x86_64 → All
Assignee: ayg → nobody
Status: ASSIGNED → NEW

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.

Severity: normal → S4
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: