Closed Bug 406596 Opened 14 years ago Closed 13 years ago

Link/anchor elements are focused within an contentEditable element

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: spam, Assigned: peterv)

References

()

Details

(Keywords: testcase)

Attachments

(3 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120305 Minefield/3.0b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120305 Minefield/3.0b2pre

When you enable contentEditable on an element, you are still able to focus the anchors within that element.

Reproducible: Always

Steps to Reproduce:
1. Open the attached URL.
2. Click the link within the editable area.
3. Observe that the link gets focused.
Actual Results:  
Link is getting the focus with a dotted outline.

Expected Results:  
The focus should still be on the editable area not inside the link.
Version: unspecified → Trunk
Component: General → Editor
Product: Firefox → Core
QA Contact: general → editor
Flags: blocking1.9?
Does the link being focused cause problems, or is it just that it appears focused?
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
It's causing problems since it sends a blur event to the contentEditable container and the dotted border of the editable region becomes undotted and it's inconsistent with other contentEditable implementations.
This issue also affects existing designMode implementations like TinyMCE or FCKEditor, it at least adds an dotted focus border on links inside these areas but it doesn't seem to send the blur event but still I can not out rule that it breaks some existing code and it looks really strange. You can also tab to them inside the editable area and that is also a problem, they look focused but the focus isn't moved because if you type the content will be inserted at caret position.

Check this URL and click or tab to the links inside the editable area:
http://tinymce.moxiecode.com/example_full.php?example=true

I really think this one needs to be blocking now since it affects existing implementations it's one thing if it only produces problems with the new contentEditable feature but another thing it changes the behavior of existing code.
Flags: blocking1.9- → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Comment 3 is implying this is a regression from 1.8 at least for designMode.  Is that the case?
Attached patch v1Splinter Review
Need to do at least this, to fix designmode. Fixing it for contentEditable is trickier, need to think a bit about interactions with tabindex etc. And most other browsers allow focusing at least some elements (so they get a focus ring), though not anchor elements I think.
Assignee: chris → peterv
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Blocks: 417533
Comment on attachment 301953 [details] [diff] [review]
v1

Stop focussing anything except the document in designMode.
Attachment #301953 - Flags: superreview?(jst)
Attachment #301953 - Flags: review?(jst)
Attachment #301953 - Flags: superreview?(jst)
Attachment #301953 - Flags: superreview+
Attachment #301953 - Flags: review?(jst)
Attachment #301953 - Flags: review+
Added a testcase with event log.
Flags: wanted1.9+ → blocking1.9+
Priority: P3 → P2
Flags: tracking1.9+
Peter - how we doin on this bug?
Keywords: testcase
Peter we are nearing RC1 freeze.  Is this ready to go or should we punt to .next?
It's not ready to go, I'm still debugging why the caret disappears in the anchor element.
I think I have a fix for this and for bug 421640, turns out they're related. Still need to clean up and do a bit of testing.
That sounds super nice since I think this and 421640 is the most serious ones they are show stoppers for one of our applications. Keep up the good work. :)
This is looking pretty good. Still a minor issue when tabbing from a contentEditable link.
Attachment #310327 - Attachment is obsolete: true
Attachment #314421 - Attachment is obsolete: true
We wouldn't hold the release for this if it were the last bug on the list (we're at that point now).  Moving to wanted1.9.0.x+.
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
Including mochitest for this and bug 421640. Still tracking down an issue with a disappearing selection when tabbing. Really close now.
Attachment #314676 - Attachment is obsolete: true
This one looks good to go.
Attachment #314976 - Attachment is obsolete: true
Make anchor elements non-focusable if they're in a contenteditable region, except if they are a contenteditable root. Don't clear the selection when focus moves out of a contenteditable region. Instead, make the event state manager not start from the selection when tabbing in a document where an editable contenteditable element is focused. Only execute the code in nsTextEditorFocusListener::Blur for the nsTextEditorFocusListener of the editor that was focussed last (there can be multiple editors and so multiple nsTextEditorFocusListeners per document).
Attachment #314994 - Attachment is obsolete: true
Attachment #315189 - Flags: superreview?
Attachment #315189 - Flags: review?
Attachment #315189 - Flags: superreview?(jst)
Attachment #315189 - Flags: superreview?
Attachment #315189 - Flags: review?(jst)
Attachment #315189 - Flags: review?
Comment on attachment 315189 [details] [diff] [review]
Fix anchor contenteditable elements v2.4

r+sr=jst
Attachment #315189 - Flags: superreview?(jst)
Attachment #315189 - Flags: superreview+
Attachment #315189 - Flags: review?(jst)
Attachment #315189 - Flags: review+
Comment on attachment 315189 [details] [diff] [review]
Fix anchor contenteditable elements v2.4

This patch fixes a number of bugs:
- this bug (  	 wanted1.9.0.x)
- bug 413678 (wanted‑next)
- bug 418135
- bug 421640 (  	 blocking1.9)

We have had regressions from previous fixes for contenteditable focus/caret fixes, so this isn't risk-free. However, I think the approach here is quite sound, I've run the whole mochitest suite and I've added tests for this bug and bug 421640. I'll try to convert the testcase from bug 413678 to an automated test (but don't think we can do that for the one from bug 418135).
Attachment #315189 - Flags: approval1.9?
Comment on attachment 315189 [details] [diff] [review]
Fix anchor contenteditable elements v2.4

a=beltzner, please do continue to file those tests, but let's get this in earlier so that any regressions (fingers crossed!) get on our radars earlier.
Attachment #315189 - Flags: approval1.9? → approval1.9+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
OS: Windows Vista → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Flags: in-testsuite+
(In reply to comment #21)
> (From update of attachment 315189 [details] [diff] [review])
> This patch fixes a number of bugs:
> - this bug (     wanted1.9.0.x)
> - bug 413678 (wanted‑next)
> - bug 418135
> - bug 421640 (           blocking1.9)
> 
> We have had regressions from previous fixes for contenteditable focus/caret
> fixes, so this isn't risk-free. However, I think the approach here is quite
> sound, I've run the whole mochitest suite and I've added tests for this bug and
> bug 421640. I'll try to convert the testcase from bug 413678 to an automated
> test (but don't think we can do that for the one from bug 418135).
> 



Could bug 419808 also be fixed by this?
Depends on: 430351
This broke most Flash text input in Camino; see bug 430351. (Given the timing and behavior, it may also be responsible for bug 429771, but that hasn't been tested yet)
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.