/tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html fails with lazy frame construction for editable elements

RESOLVED FIXED in Firefox 57

Status

()

Core
Editor
RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: tnikkel, Assigned: m_kato)

Tracking

(Depends on: 1 bug)

Trunk
mozilla57
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
The problem is that the caret is in a cell in the last and only row of a table, a tab key press event is sent. This creates a new row in the table (nsHTMLEditor::TabInTable calls InsertTableRow). Before creating the new nodes InsertTableRow puts a nsSetSelectionAfterTableEdit object on the stack that will position the caret in the new row in its destructor by calling SetSelectionAfterTableEdit. SetSelectionAfterTableEdit gets the cell to position the caret in by calling GetCellAt, which calls GetCellDataAt. GetCellDataAt gets the nsITableLayout object from the table frame and asks it for the cell. But this isn't up to date because the new cells don't have frames yet.

It seems like we should be flushing nsHTMLEditor::SetSelectionAfterTableEdit so that we have up to date layout. Can we do that?
(Reporter)

Comment 1

7 years ago
I looked at most (but not all!) call sites to see if a flush there would be okay. It seems okay. I didn't check them all because it just too involved. Ehsan, do you think a flush there would be okay?
(Reporter)

Updated

6 years ago
Assignee: nobody → tnikkel

Comment 2

6 years ago
(In reply to Timothy Nikkel (:tn) from comment #0)
> It seems like we should be flushing nsHTMLEditor::SetSelectionAfterTableEdit
> so that we have up to date layout. Can we do that?

Would that be not a better idea to get that information from the content tree?  The only concern that I can think of with that is figuring out if the cell is actually visible or not.

A flush there _seems_ to be OK to me.  If you wanna go down that path, make sure to run things on try, but I don't expect a lot of problems there...

Comment 3

6 years ago
OK, let's just flush here!  :-)
(Reporter)

Comment 4

6 years ago
I wanted to expend at least some energy determining how feasible it is to look at the content tree for the information we need, but I haven't gotten to it yet.
(Assignee)

Updated

10 months ago
Blocks: 1348073
(Assignee)

Updated

10 months ago
Depends on: 1373477
(Assignee)

Updated

10 months ago
Assignee: tnikkel → m_kato
Comment hidden (mozreview-request)

Comment 6

10 months ago
mozreview-review
Comment on attachment 8886505 [details]
Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit.

https://reviewboard.mozilla.org/r/157318/#review162462

::: commit-message-67cd1:3
(Diff revision 1)
> +Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit. r?masayuki
> +
> +HTMLEditor::TabInTable inserts row element, then select a cell.  But when enabling lazy frame construction for editable node, it select invalid cell and table.

s/it select/it selects

::: editor/libeditor/HTMLTableEditor.cpp:698
(Diff revision 1)
> +  nsCOMPtr<nsIPresShell> ps = GetPresShell();
> +  if (ps) {
> +    ps->FlushPendingNotifications(FlushType::Frames);
> +  }
> +
> +  return NS_OK;

I don't think that this is safe. The flush may cause destroying the editor. So, all root callers need to grab the HTMLEditor instance and do nothing if the editor itself is destroyed.

How about return error or new succeeded code in such case? Then, you should check if the caller checks the error result. For example, if the caller touches Selection, it may cause unexpected result for users and web apps.

HTMLEditor::DoInlineTableEditingAction() doesn't check the result. Its caller is HTMLEditorEventListener::MouseClick() but it doesn't grab the instance with local variable and keep going to EditorEventListener::MouseClick().

HTMLEditor::TabInTable() checks the result. It's caller is HTMLEditor::HandleKeyPressEvent(), and it also checks the result but it doesn't consume the event because HTMLEditor::TabInTable() sets outHandled to true after InsertTableRow() call is succeeded. (It't caller is EditorEventListener::KeyPress() and it grabs the editor instance with local variable, so, this case is safe.)
Attachment #8886505 - Flags: review?(masayuki) → review-
Comment hidden (mozreview-request)

Comment 8

10 months ago
mozreview-review
Comment on attachment 8886505 [details]
Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit.

https://reviewboard.mozilla.org/r/157318/#review162476

You still don't change the result when the editor instance is destroyed during the flush.

E.g., TabInTable will change selection after the editor is outdated:
https://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/editor/libeditor/HTMLEditor.cpp#1070,1091
and EditorEventListener::MouseClick() might try to paste text into the editor (I'm not sure if this is actually occurs though).

Perhaps, you need to define NS_SUCCESS_EDITOR_BUT_DESTROYED or something in ErrorList.py handle it correctly in HTMLEditorEventListener::MouseClick(), HTMLEditor::TabInTable(), HTMLEditor::HandleKeyPressEvent() and HTMLEditor::DoInlineTableEditingAction().

::: editor/libeditor/HTMLInlineTableEditor.cpp:130
(Diff revision 2)
>  HTMLEditor::DoInlineTableEditingAction(nsIDOMElement* aElement)
>  {
>    NS_ENSURE_ARG_POINTER(aElement);
> +
> +  // InsertTableRow might cause reframe, so we have to grab editor
> +  RefPtr<HTMLEditor> kungFuDeathGrip(this);

I don't think that this is necessary. The root caller should guarantee this (fortunately, there are not so many callers).
Attachment #8886505 - Flags: review?(masayuki) → review-

Comment 9

10 months ago
mozreview-review-reply
Comment on attachment 8886505 [details]
Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit.

https://reviewboard.mozilla.org/r/157318/#review162476

Ah, or, keep returning NS_OK but the callers should check HTMLEditor has been destroyed.
(Assignee)

Comment 10

10 months ago
mozreview-review-reply
Comment on attachment 8886505 [details]
Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit.

https://reviewboard.mozilla.org/r/157318/#review162476

TabInTable returns sucessful even if setting selection by AutoSelectionSetterAfterTableEdit RAIL class is failed..  Then, lcoal row value by GetCellContext becomes incorerct.  So even if we can get cell value, this cell becomes incorrect cell.

> I don't think that this is necessary. The root caller should guarantee this (fortunately, there are not so many callers).

Since DoInlineTableEditingAction is XPCOM public method, it adds it.  If you say OK, I don't add grab.
(Assignee)

Comment 11

10 months ago
mozreview-review-reply
Comment on attachment 8886505 [details]
Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit.

https://reviewboard.mozilla.org/r/157318/#review162476

/s/RAIL/RAII
(In reply to Makoto Kato [:m_kato] from comment #10)
> Comment on attachment 8886505 [details]
> Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit.
> 
> https://reviewboard.mozilla.org/r/157318/#review162476
> 
> TabInTable returns sucessful even if setting selection by
> AutoSelectionSetterAfterTableEdit RAIL class is failed..  Then, lcoal row
> value by GetCellContext becomes incorerct.  So even if we can get cell
> value, this cell becomes incorrect cell.
> 
> > I don't think that this is necessary. The root caller should guarantee this (fortunately, there are not so many callers).
> 
> Since DoInlineTableEditingAction is XPCOM public method, it adds it.  If you
> say OK, I don't add grab.

It's okay. If it's called from script, HTMLEditor instance is automatically grabbed by binding code. If it's called from C++ code, the caller owns the responsibility.
(Assignee)

Comment 13

10 months ago
mozreview-review-reply
Comment on attachment 8886505 [details]
Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit.

https://reviewboard.mozilla.org/r/157318/#review162476

Also, SetSelectionAfterTableEdit cannot get cell, it will select parent table instead....
Okay, then make mTableEditor as RefPtr<HTMLEditor> and refer EditorBase::mDidPreDestroy with creating new method EditorBase::Destroyed() const.
Comment hidden (mozreview-request)

Comment 16

9 months ago
mozreview-review
Comment on attachment 8886505 [details]
Bug 699703 - Need reframe before calling SetSelectionAfterTableEdit.

https://reviewboard.mozilla.org/r/157318/#review165068

And I guess, don't we need to check Destroy() at first of HTMLEditor::SetSelectionAfterTableEdit()? If the editor has gone, I think that the editor should do nothing anymore.

::: editor/libeditor/HTMLEditor.cpp:659
(Diff revision 3)
> +        // TabInTable may release HTMLEditor by reframe
> +        RefPtr<HTMLEditor> kungFuDeathGrip(this);
> +
>          rv = TabInTable(aKeyboardEvent->IsShift(), &handled);
> +        // TabInTable might cause reframe
> +        if (Destroyed()) {
> +          return NS_OK;
> +        }

The only caller of HandleKeyPressEvent is here:
https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/editor/libeditor/EditorEventListener.cpp#603,609

So, you don't need to create kungFuDeathGrip here.
Attachment #8886505 - Flags: review?(masayuki) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

9 months ago
mozreview-review
Comment on attachment 8892353 [details]
Bug 699703 - Part 2. SetSelectionAfterTableEdit should check whether editor is destroyed.

https://reviewboard.mozilla.org/r/163314/#review168744
Attachment #8892353 - Flags: review?(masayuki) → review+

Comment 20

9 months ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/68241e0357c2
Need reframe before calling SetSelectionAfterTableEdit. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/5ddcc92a2af6
Part 2. SetSelectionAfterTableEdit should check whether editor is destroyed. r=masayuki

Comment 21

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68241e0357c2
https://hg.mozilla.org/mozilla-central/rev/5ddcc92a2af6
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.