Closed Bug 1316302 Opened 8 years ago Closed 8 years ago

Backspace key does nothing when collapsed selection is in a block and its previous child block has a non-empty text node at rightmost

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(
URL
)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1316302 part.1 Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1316302 part.2 WillDeleteSelection() should retry to handle it when selection is collapsed and JoinBlocks() doesn't handle nor cancel the action
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1316302 part.3 Create EditActionResult class for making the methods which return nsresult, handled and canceled with out params
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1316302 part.4 Refine HTMLEditRules::TryToJoinBlocks() and HTMLEditRules::MoveNodeSmart() with early return style for making scope of EditActionResult variable smaller
58 bytes, text/x-review-board-request
smaug
: review+
Details 
It's difficult to explain with words. See this image:

+-<blockquote>------------------------------------+
|  +-<p>---------------------------------------+  |
|  | +---+                                     |  |
|  | |abc|                                     |  |
|  | +---+                                     |  |
|  +-------------------------------------------+[]|
+-------------------------------------------------+

When collapsed selection is at "[]", caret is shown at right of "c". In this case, pressing Backspace key is expected as removing "c". However, HTMLEditRules::WillDeleteSelection() tries to join <blockquote> and <p> but it fails.  Then, that causes looking like our editor does nothing.
Comment on attachment 8809703 [details]
Bug 1316302 part.1 Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action

https://reviewboard.mozilla.org/r/92250/#review92682

I don't understand aHandled param usage. It is never set to false in the methods having it as a param. Because of that, r-
(other issues are minor)

::: editor/libeditor/HTMLEditRules.h:168
(Diff revision 1)
>    nsresult InsertBRIfNeeded(Selection* aSelection);
>    mozilla::EditorDOMPoint GetGoodSelPointForNode(nsINode& aNode,
>                                                   nsIEditor::EDirection aAction);
> +
> +  /**
> +   * JoinBlocks() joins two block elements.  The right element is always joined

So this happens even when delete key is used?

::: editor/libeditor/HTMLEditRules.h:176
(Diff revision 1)
> +   * items together into one).  If the elements are not the same type, or one
> +   * is a descendant of the other, we instead destroy the right block placing
> +   * its children into leftblock.  DTD containment rules are followed
> +   * throughout.
> +   *
> +   * @param aHandled    returns true if this actually handles the request.

The parameters are documented in wrong order

::: editor/libeditor/HTMLEditRules.h:178
(Diff revision 1)
> +   * its children into leftblock.  DTD containment rules are followed
> +   * throughout.
> +   *
> +   * @param aHandled    returns true if this actually handles the request.
> +   *                    Note that this may return true even if this does not
> +   *                    join the block.  E.g., if the blocks shouldn't be

Because of this, it might be nice to rename the method to something like TryJoinBlocks or something similar.

::: editor/libeditor/HTMLEditRules.h:181
(Diff revision 1)
> +   * @param aHandled    returns true if this actually handles the request.
> +   *                    Note that this may return true even if this does not
> +   *                    join the block.  E.g., if the blocks shouldn't be
> +   *                    joined or it's impossible to join them but it's not
> +   *                    unexpected case, this returns true with this.
> +   * @param aCanceled   returns true the operation should do nothing anymore

Is the sentence missing one 'if'?

returns true if the...

::: editor/libeditor/HTMLEditRules.h:212
(Diff revision 1)
>    nsresult MoveNodeSmart(nsIContent& aNode, Element& aDestElement,
> -                         int32_t* aOffset);
> +                         int32_t* aOffset, bool* aHandled);
> +
> +  /**
> +   * MoveContents() moves the contents of aElement to (aDestElement,
> +   * aInOutDestOffset).  DTD containment rules are followed throughout.

There is no aInOutDestOffset

::: editor/libeditor/HTMLEditRules.h:214
(Diff revision 1)
> +
> +  /**
> +   * MoveContents() moves the contents of aElement to (aDestElement,
> +   * aInOutDestOffset).  DTD containment rules are followed throughout.
> +   *
> +   * @param aInOutDestOffset        updated to point after inserted content.

ditto

::: editor/libeditor/HTMLEditRules.h:219
(Diff revision 1)
> +   * @param aInOutDestOffset        updated to point after inserted content.
> +   * @param aHandled                returns true if this actually moves the
> +   *                                nodes.
> +   */
>    nsresult MoveContents(Element& aElement, Element& aDestElement,
> -                        int32_t* aOffset);
> +                        int32_t* aOffset, bool* aHandled);

So should aOffset be renamed

::: editor/libeditor/HTMLEditRules.cpp:2565
(Diff revision 1)
>    nsCOMPtr<Element> leftBlock = htmlEditor->GetBlock(aLeftNode);
>    nsCOMPtr<Element> rightBlock = htmlEditor->GetBlock(aRightNode);
>  
>    // Sanity checks
>    NS_ENSURE_TRUE(leftBlock && rightBlock, NS_ERROR_NULL_POINTER);
>    NS_ENSURE_STATE(leftBlock != rightBlock);

So in case any of these checks fail, aHandled isn't changed at all. Is that expected? Though, looks like aCanceled is handled the same way so fine.
Attachment #8809703 - Flags: review?(bugs) → review-
Comment on attachment 8809704 [details]
Bug 1316302 part.2 WillDeleteSelection() should retry to handle it when selection is collapsed and JoinBlocks() doesn't handle nor cancel the action

https://reviewboard.mozilla.org/r/92252/#review92688

::: editor/libeditor/HTMLEditRules.cpp:2180
(Diff revision 1)
> -        //       we should modify it.
> -        *aHandled = true;
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
> +
> +      // If JoinBlocks() didn't handle it actually and it's not canceled,

drop word "actually"
Attachment #8809704 - Flags: review?(bugs) → review+
Priority: -- → P3
Comment on attachment 8809703 [details]
Bug 1316302 part.1 Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action

https://reviewboard.mozilla.org/r/92250/#review92682

> So this happens even when delete key is used?

Yes.

> The parameters are documented in wrong order

Oops, thanks. Anyway, I'd like to change the result is a struct and which includes "canceled" and "handled" state later.

> Because of this, it might be nice to rename the method to something like TryJoinBlocks or something similar.

Sure.

> So in case any of these checks fail, aHandled isn't changed at all. Is that expected? Though, looks like aCanceled is handled the same way so fine.

Hmm, it's a good point. Although, if it returns an error, caller shouldn't refer aCanceled and aHandled because it cannot handle it correctly with the error.
Okay, I changed each helper method returns what each of them does with aCancel and aHandled. I.e., they do not continue to modify the value passed from their caller.

However, using out param is NOT guaranteed to keep the behavior. Therefore, I'd like to change the return type to a class which have nsresult, canceled and handled state. This is what I have planned since this summer. Ideally, we should use this style in whole places in editor because most methods in editor takes too many arguments and that is one of the causes of ugly code. Therefore, I did it at part.3.

Additionally, this change causes making the callers not easier to read because the scope of the variables which receives the method's result is too wide. Therefore, I refine the callers with "early return style" and making the scope narrower.

If you don't like these additional changes, only part.1 and part.2 can be landed. But I love the new design because it reduces arguments. That makes developers focus on what each method wants/uses.
Comment on attachment 8809703 [details]
Bug 1316302 part.1 Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action

https://reviewboard.mozilla.org/r/92250/#review93530

::: editor/libeditor/HTMLEditRules.cpp:2234
(Diff revision 2)
>          NS_ENSURE_STATE(leftNode->IsContent() && rightNode->IsContent());
> +        bool handled = false, canceled = false;
> +        rv = TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(),
> +                             &canceled, &handled);
> +        // This should claim that trying to join the block means that
> +        // this handles the action because the caller shouldn't do nothing

s/nothing/anything/
Attachment #8809703 - Flags: review?(bugs) → review+
Comment on attachment 8811197 [details]
Bug 1316302 part.3 Create EditActionResult class for making the methods which return nsresult, handled and canceled with out params

https://reviewboard.mozilla.org/r/93406/#review93532

::: editor/libeditor/EditorUtils.h:34
(Diff revision 1)
>  namespace dom {
>  class Selection;
>  } // namespace dom
>  
>  /***************************************************************************
> + * EditActionResult is useful to return multiple information of an editor

maybe "return multiple results"

::: editor/libeditor/HTMLEditRules.h:179
(Diff revision 1)
>     * placing its children into leftblock.  DTD containment rules are followed
>     * throughout.
>     *
> -   * @param aCanceled   returns true if the operation should do nothing anymore
> -   *                    even if this doesn't join the blocks.
> -   *                    NOTE: When this returns an error, nobody should refer
> +   * @return            Sets mCanceled to true if the operation should do
> +   *                    nothing anymore even if this doesn't join the blocks.
> +   *                    Sets mHandled to true if this actually handles the

s/mHandled/Handled
Attachment #8811197 - Flags: review?(bugs) → review+
Comment on attachment 8811198 [details]
Bug 1316302 part.4 Refine HTMLEditRules::TryToJoinBlocks() and HTMLEditRules::MoveNodeSmart() with early return style for making scope of EditActionResult variable smaller

https://reviewboard.mozilla.org/r/93408/#review93550

ok, this is a bit hard to review, perhaps because of MozReview.
But I don't like adding DeleteNode calls even more. So easy to forget to call it. 
Would it be possible to re-architect the code somehow so that DeleteNode is called when we're about to return from the method? Like some simple stack class? Or just keep the code as it is.

::: editor/libeditor/HTMLEditRules.cpp:2568
(Diff revision 1)
>    }
>    return ret;
>  }
>  
>  EditActionResult
>  HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode,

For some reason the changes to this method are very difficult to review

::: editor/libeditor/HTMLEditRules.cpp:2925
(Diff revision 1)
>  
>    return ret;
>  }
>  
>  EditActionResult
>  HTMLEditRules::MoveNodeSmart(nsIContent& aNode,

Ok, MoveNodeSmart is easy to review
Attachment #8811198 - Flags: review?(bugs) → review-
Comment on attachment 8811199 [details]
Bug 1316302 part.5 Minimize variable scopes in HTMLEditRules::TryToJoinBlocks()

https://reviewboard.mozilla.org/r/93410/#review93560

Because of the r- to the previous patch, I think this doesn't quite apply as such. So clearing r?
Attachment #8811199 - Flags: review?(bugs)
Attachment #8811199 - Attachment is obsolete: true
Smaug:

Okay, you'll go vacation, so, let's take only simple refine patch for making the scope of the variable smaller.

I couldn't request review to you, but could you give r+ for it before going vacation?
Comment on attachment 8811198 [details]
Bug 1316302 part.4 Refine HTMLEditRules::TryToJoinBlocks() and HTMLEditRules::MoveNodeSmart() with early return style for making scope of EditActionResult variable smaller

https://reviewboard.mozilla.org/r/93408/#review94138
Attachment #8811198 - Flags: review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ad5ce748ebdd
part.1 Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action r=smaug
https://hg.mozilla.org/integration/autoland/rev/a4c1e8110537
part.2 WillDeleteSelection() should retry to handle it when selection is collapsed and JoinBlocks() doesn't handle nor cancel the action r=smaug
https://hg.mozilla.org/integration/autoland/rev/43a0b34ba23a
part.3 Create EditActionResult class for making the methods which return nsresult, handled and canceled with out params r=smaug
https://hg.mozilla.org/integration/autoland/rev/c45a1f84b45d
part.4 Refine HTMLEditRules::TryToJoinBlocks() and HTMLEditRules::MoveNodeSmart() with early return style for making scope of EditActionResult variable smaller r=smaug
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: