102679 - Composer loses focus after deleting a linked image

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[TucsonTester2]

Build ID: 20011001

Composer looses focus after deleting a linked image.

Reproducible: Always

Steps to Reproduce:
1.Open Composer
2.Insert an image using the image button on the toolbar
3.Highlight the image and click the link button on the toolbar then create a
link and click ok
4.Click the image and delete it
5.Type some text

Actual Results:
The text does not type because the composer window has lost focus.  In order to
type again you need to click in the window.

Expected Results:
I would expect that composer would maintain it's focus after deleting a linked
image.

Comment 1

[rcummins]

Changed spelling from "looses" to "loses" in summary, in case someone tries to
do a query for it.

Comment 2

[rcummins]

<spam>Would you believe that Bugzilla's quip after updating the summary was
"Notice: Spelling mistakes left in for people who need to correct others to make
their life fulfilled"?  And here I thought I was making a tiny
contribution...</spam>

Comment 3

[Syd Logan]

charles, can you look at this after the EDITORBASE bugs are all fixed? 

Comment 4

[Charles Manske]

I can't confirm this.

Comment 5

[michael wendell]

I am only seeing this on the Mac.  I was able to reproduce it on OSX using the
20011012 trunk build, and the 20011011 branch build.  I am marking this new, and
switching to Mac only.

Comment 6

[Kathleen :Brade]

I see this on my Macintosh debug build from today.
The caret is there blinking but no insertion happens until you click and then 
type.  Very bizarre and unfriendly!

Comment 7

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 8

[Charles Manske]

I can definitely reproduce this.
Seems this has to be a core editor issue?

Comment 9

[kinmoz]

Giving this to aaronl, it looks like his modification to
nsEventStateManager::ContentRemoved() in nsEventStateManager.cpp revision 1.329,
which adds a call to SendFocusBlur(), leaves gLastFocusedContent and
mCurrentFocus set to the image.

It should probably traverse up the mCurrentFocus' parent hierarchy to see who it
should pass the focus up to after it is deleted, and then use that, or null, as
the arg to SendFocusBlur().

I think in this particular case, the image is focusable because it is in an anchor.

Cc a bunch of other focus savvy people to comment on whether or not this is the
right thing to do.

Comment 10

[Aaron Leventhal]

Will look at this ASAP.

Comment 11

[Aaron Leventhal]

Attachment: Changes nsEventStateManager::ContentRemoved() back to the way it was

This was an absolute mistake that no one caught. The ContentRemoved() method
was never supposed to be modified by the original patch. This changes it back.

Seeking r=/sr=

Comment 12

[Kyle Yuan]

Comment on attachment 89026 [details] [diff] [review]
Changes nsEventStateManager::ContentRemoved() back to the way it was

r=kyle

Comment 13

[Kyle Yuan]

Comment on attachment 89026 [details] [diff] [review]
Changes nsEventStateManager::ContentRemoved() back to the way it was

r=kyle

Comment 14

[Kyle Yuan]

Comment on attachment 89026 [details] [diff] [review]
Changes nsEventStateManager::ContentRemoved() back to the way it was

r=kyle

Comment 15

[Kyle Yuan]

Comment on attachment 89026 [details] [diff] [review]
Changes nsEventStateManager::ContentRemoved() back to the way it was

r=kyle

Comment 16

[Kyle Yuan]

Comment on attachment 89026 [details] [diff] [review]
Changes nsEventStateManager::ContentRemoved() back to the way it was

r=kyle

Comment 17

[Kyle Yuan]

oh, my god! my bugzilla is crazy! sorry for my spam!

Comment 18

[kinmoz]

Comment on attachment 89026 [details] [diff] [review]
Changes nsEventStateManager::ContentRemoved() back to the way it was

sr=[e-mail]

Just out of curiosity, I know the original code didn't do this either, but
should gLastFocusedContent
 also be released?

Comment 19

[Aaron Leventhal]

> Just out of curiosity, I know the original code didn't do this either, but
> should gLastFocusedContent also be released?

Kin, I don't actually know if it should. I never meant to touch
::ContentRemoved(). Bryner?

Comment 20

[Aaron Leventhal]

checked in

Comment 21

[Brian Ryner (not reading)]

gLastFocusedContent should be released if it is the same as mCurrentFocus, but
this may happen via another code path.  I can't remember offhand.

Comment 22

[rubydoo123]

using the trunk build 2003040308 on win2K, this is what I did:
1. opened new compse window
2. inserted an image
3. selected image, then selected link icon from toolbar, entered location,
selected ok
4. moved focus elsewhere, reselected image and hit delete.

Image deleted, but anchor did not, but I think that is another issue.

so, back in step 4, with the image as the only content, I did ctrl+A (select
all) and both the image and anchor deleted. 

I also selected the image, hit end, held shift+home and selected delete and the
image and anchor was deleted.

Selecting the anchor symbol in the status bar and seleting delete also deletes
the anchor and image.