550434 - Clicking in an empty contenteditable element that has focus causes the caret to disappear

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's not clear whether the element has focus or not. Typing doesn't work. See bug 539323 comment #8.

Comment 1

[bmccann]

Thanks for the help on this.  I'd love it if we could mark this as a blocker for 3.7.

Comment 2

[Alexander Ljungberg]

This bug affects us too. Sometimes it's possible to make a caret appear through dedicated clicking, but you still can't enter text. A workaround is to seed the contentEditable div with a "<br>" tag.

Comment 3

[bmccann]

I had been trying the <br> tag for awhile, but just couldn't make it work.  I kept getting into weird situation where all of a sudden the browser would start rendering it.  I was almost able to get away with a 0-width space.  In either case the user can copy it out (e.g. Ctrl+A, Ctrl+C) and that was a deal breaker, which I couldn't find anyway around.

Comment 4

[bmccann]

This is one of a few contentEditable bugs that has made it difficult/impossible to implement Google Spreadsheet's formula highlighting in Firefox.  We'd like to turn the feature on in FF 3.7 assuming enough of the issues have been addressed in the browser.  Thanks!
http://googledocs.blogspot.com/2010/05/formula-highlighting-in-spreadsheets.html

Comment 5

[(no longer active)]

The first time that you click the element, the selection code sets the focus to the <div> node.  This includes setting up the selection to be anchored to the div element, and firing a focus event.  The focus event is caught by nsEditorEventListener::Focus.  This function calls nsFrameSelection::SetAncestorLimiter to set the selection limiter to the <div> element.

<http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorEventListener.cpp#925>

Inside this function, we call IsValidSelectionPoint:

<http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#3249>

Note that here, the limiter is null (because we're not a text control), and the aNode parameter is set to the element currently registered to have the focus (which is the *body* element, not the div element yet).  Before IsValidSelectionPoint is called, we set the ancestor limiter to the div element <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#3243>, so, effectively the result of IsValidSelectionPoint is the result of this expression: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#570> where limiter is the div, and aNode is the body element.  This check fails, which causes us to nuke our selection <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#3250>, which messes up the selection, and the next time when you click on the div, you won't get a caret because we don't have a selection any more.

I _think_ the correct fix here is that in the SetAncestorLimiter call from nsEditorEventListener::Focus, we skip the ancestor limiter checks.  This is actually the correct thing to do because we know that later on when the div takes focus, the ancestor limiter set is the correct one (because the ancestor limiter and the focused node are the same.)

Comment 6

[(no longer active)]

Actually, it seems that the behavior in comment 5 only causes problems for text area elements which are empty initially.  This has started to look a lot like bug 551704.

Comment 7

[Alexander Ljungberg]

The <br /> problem in bug 551704 is in fact the reason the work-around I described above happens to work for us. Firefox acts like the <br /> isn't there, but selection and so on works as opposed to if the field had been entirely empty.

Comment 8

[(no longer active)]

(In reply to comment #7)
> The <br /> problem in bug 551704 is in fact the reason the work-around I
> described above happens to work for us. Firefox acts like the <br /> isn't
> there, but selection and so on works as opposed to if the field had been
> entirely empty.

Yes.  Furthermore, the existence of the <br> node has many more implications than what's apparent.  That problem really needs to be addressed.

Comment 9

[(no longer active)]

Attachment: Patch (v1)

So, we need to ensure that our selection is alive and well on clicking as well as on focus.  This patch does that through the SetAncestorLimiter call.

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e9b7e557499d

Comment 11

[(no longer active)]

Comment on attachment 450478 [details] [diff] [review]
Patch (v1)

We definitely would want this fix on our branches as well as on trunk.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that the patch is wrong.

When 2nd click on the editor, nsFrameSelection::HandleClick() is called with aNewFocus == <body>. Then, TakeFocus() steals the focus.

nsFrameSelection::HandleClick()
nsFrame::HandlePress()
nsFrame::HandleEvent()
nsPresShellEventCB::HandleEvent()
nsEventTargetChainItem::HandleEventTargetChain()
nsEventDispatcher::Dispatch()
PresSHell::HandleEventInternal()
PresSHell::HandlePositionedEvent()
PresShell::HandleEvent()
...

However, the actual focused element is still <div>, therefore, nsEditorEventListener::Focus() isn't called again.

I think that this shouldn't be an editor bug.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1971
> 1862 NS_IMETHODIMP
> 1863 nsFrame::HandlePress(nsPresContext* aPresContext, 
> 1864                      nsGUIEvent*     aEvent,
> 1865                      nsEventStatus*  aEventStatus)
> 1866 {

> 1971   nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, this);
> 1972   ContentOffsets offsets = GetContentOffsetsFromPoint(pt);

> 2041   // Do not touch any nsFrame members after this point without adding
> 2042   // weakFrame checks.
> 2043   rv = fc->HandleClick(offsets.content, offsets.StartOffset(),
> 2044                        offsets.EndOffset(), me->isShift, control,
> 2045                        offsets.associateWithNext);

The result of GetContentOffsetsFromPoint() is strange.

Comment 14

[(no longer active)]

(In reply to comment #12)
> I think that the patch is wrong.
> 
> When 2nd click on the editor, nsFrameSelection::HandleClick() is called with
> aNewFocus == <body>.

Actually the same thing happens when nsFrameSelection::HandleClick/Focus are called for the first time.

> Then, TakeFocus() steals the focus.

It doesn't steal the focus.  The focus remains in the div both before and after this patch.  It's the selection which was going away, and that's the problem.

> nsFrameSelection::HandleClick()
> nsFrame::HandlePress()
> nsFrame::HandleEvent()
> nsPresShellEventCB::HandleEvent()
> nsEventTargetChainItem::HandleEventTargetChain()
> nsEventDispatcher::Dispatch()
> PresSHell::HandleEventInternal()
> PresSHell::HandlePositionedEvent()
> PresShell::HandleEvent()
> ...
> 
> However, the actual focused element is still <div>, therefore,
> nsEditorEventListener::Focus() isn't called again.

nsEditorEventListener::Focus is only called for the first click, both before and after this patch.

(In reply to comment #13)
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1971
> > 1862 NS_IMETHODIMP
> > 1863 nsFrame::HandlePress(nsPresContext* aPresContext, 
> > 1864                      nsGUIEvent*     aEvent,
> > 1865                      nsEventStatus*  aEventStatus)
> > 1866 {
> 
> > 1971   nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, this);
> > 1972   ContentOffsets offsets = GetContentOffsetsFromPoint(pt);
> 
> > 2041   // Do not touch any nsFrame members after this point without adding
> > 2042   // weakFrame checks.
> > 2043   rv = fc->HandleClick(offsets.content, offsets.StartOffset(),
> > 2044                        offsets.EndOffset(), me->isShift, control,
> > 2045                        offsets.associateWithNext);
> 
> The result of GetContentOffsetsFromPoint() is strange.

Yes, that was what I encountered as well.  But the exact same result happens when the div is clicked for the first time as well.  I wasn't so sure what that function is supposed to return, and stepping through it I didn't see anything particularly wrong (although I don't claim to understand all the details).  Therefore I didn't touch it.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #14)
>> Then, TakeFocus() steals the focus.
> 
> It doesn't steal the focus.  The focus remains in the div both before and after
> this patch.  It's the selection which was going away, and that's the problem.

I meant it steals selection (caret) from editor (set to body). It's the cause of this bug. Your patch just reinitialize the selection/caret on mouseclick event after that.

> Yes, that was what I encountered as well.  But the exact same result happens
> when the div is clicked for the first time as well.  I wasn't so sure what that
> function is supposed to return, and stepping through it I didn't see anything
> particularly wrong (although I don't claim to understand all the details). 
> Therefore I didn't touch it.

Because after here, nsEditorEventListener::Focus() will be called and it initialize the selection, then, caret is moved to the editor. Therefore, only when we click at first time, caret doesn't disappear.

Comment 16

[(no longer active)]

(In reply to comment #15)
> (In reply to comment #14)
> >> Then, TakeFocus() steals the focus.
> > 
> > It doesn't steal the focus.  The focus remains in the div both before and after
> > this patch.  It's the selection which was going away, and that's the problem.
> 
> I meant it steals selection (caret) from editor (set to body). It's the cause
> of this bug. Your patch just reinitialize the selection/caret on mouseclick
> event after that.

Do you mean that we want the selection to be on the body element?

> > Yes, that was what I encountered as well.  But the exact same result happens
> > when the div is clicked for the first time as well.  I wasn't so sure what that
> > function is supposed to return, and stepping through it I didn't see anything
> > particularly wrong (although I don't claim to understand all the details). 
> > Therefore I didn't touch it.
> 
> Because after here, nsEditorEventListener::Focus() will be called and it
> initialize the selection, then, caret is moved to the editor.

Yes, but this only happens the first time that the element is clicked.

> Therefore, only when we click at first time, caret doesn't disappear.

This is actually the opposite of the problem.  When you click on the div for the first time, the caret shows up correctly.  When you click on it for the second time, it goes away.  And that happens because of SetAncestorLimiter clearing the selection...

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > >> Then, TakeFocus() steals the focus.
> > > 
> > > It doesn't steal the focus.  The focus remains in the div both before and after
> > > this patch.  It's the selection which was going away, and that's the problem.
> > 
> > I meant it steals selection (caret) from editor (set to body). It's the cause
> > of this bug. Your patch just reinitialize the selection/caret on mouseclick
> > event after that.
> 
> Do you mean that we want the selection to be on the body element?

No, the selection should be in the div. I said the actual behavior.

> > Therefore, only when we click at first time, caret doesn't disappear.
> 
> This is actually the opposite of the problem.  When you click on the div for
> the first time, the caret shows up correctly.  When you click on it for the
> second time, it goes away.

??

> And that happens because of SetAncestorLimiter clearing the selection...

Really? Who calls SetAncestorLimit? At second click, nsEditorEventListener::Focus() isn't called. And I cannot break my debug build (without your patch) in SetAncestorLimiter() at second click.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Alternative approach v1.0

I think that approach like this patch is better.

Comment 19

[(no longer active)]

(In reply to comment #17)
> > > Therefore, only when we click at first time, caret doesn't disappear.
> > 
> > This is actually the opposite of the problem.  When you click on the div for
> > the first time, the caret shows up correctly.  When you click on it for the
> > second time, it goes away.
> 
> ??
> 
> > And that happens because of SetAncestorLimiter clearing the selection...
> 
> Really? Who calls SetAncestorLimit? At second click,
> nsEditorEventListener::Focus() isn't called. And I cannot break my debug build
> (without your patch) in SetAncestorLimiter() at second click.

My apologies.  I meant by setting mAncestorLimier here: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#1654>.

Comment 20

[(no longer active)]

Comment on attachment 450771 [details] [diff] [review]
Alternative approach v1.0

The general approach of this patch (special casing editable elements in GetSelectionClosestFrameForBlock) was what I tested before writing the patch that I landed here.  It seemed to work, but I was worried making this change because I wasn't feeling quite confident if this won't break anything else.

The patch definitely looks good, but I have the same lack of confidence with this as well.  Let's see what roc thinks.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I posted the patch to tryserver.

Comment 22

[(no longer active)]

Thanks a lot! :-)

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

You're welcome. The patch passed the tests on Linux and Mac, is still testing on Windows.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Taking and this should be a bug of selection.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

passed on windows too.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 450478 [details] [diff] [review]
Patch (v1)

We shouldn't take this patch for branch.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, sorry for the spam.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 450771 [details] [diff] [review]
Alternative approach v1.0

I like this approach

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Roc and Ehsan.

http://hg.mozilla.org/mozilla-central/rev/c70244e2e622

Comment 30

[(no longer active)]

Thank *you*!

Could you please provide a unified patch to submit it for branch consideration?

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

O.K. But shouldn't we watch regression reports for a week?

Comment 32

[bmccann]

Thanks!!  This bug was driving me crazy and I was unable to find a JS/HTML workaround, so I really appreciate the fix!

Comment 33

[(no longer active)]

(In reply to comment #31)
> O.K. But shouldn't we watch regression reports for a week?

Of course.  We can hold on until then.  I usually prepare the branch patch sooner though, to make sure that I don't forget about it.  YMMV.  :-)

Comment 34

[(no longer active)]

This caused a Tp4 mem regression: <http://mzl.la/ct7vnB>.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Really? The patch doesn't use heap. And the new member |emptyBlock| is 3rd PRPackedBool, so, it shouldn't cause increasing footprint.

Comment 36

[(no longer active)]

Agreed!