Closed Bug 390278 Opened 13 years ago Closed 13 years ago

Shift+tab no longer navigates out of MIDAS area

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: aaronlev, Assigned: peterv)

References

(Blocks 1 open bug)

Details

(Keywords: access, regression, Whiteboard: [dbaron-1.9:RwCc])

Attachments

(2 files, 7 obsolete files)

1. Go to:
http://www.kevinroth.com/rte/demo.htm
or
http://www.mozilla.org/editor/midasdemo/

2. Tab into editor
3. Notice that Shift+Tab no longer works.

I'm really glad that Tab works correctly now, as it does in IE, but Shift+Tab should still work as well.
Flags: blocking1.9?
This probably happened in bug 237964.
Also, there is a bug to change things back so Tab key inserts a tab again, but I hope that is marked WONTFIX because this is a section 508 issue. See bug 389199 for that.
The original bug to fix tabbing was bug 190513 -- that should be marked fixed if this is.
Blocks: 190513
Keywords: regression
Yeah, this regressed when bug 237964 got checked in.
Bug 389199 is a regression, and I think it was unintentional. If you want to see it WONTFIX, then you probably should comment in that bug ;)
Probably related to/the same bug as bug 389097.
Depends on: 389097
Flags: blocking1.9? → blocking1.9+
Assignee: nobody → peterv
Priority: -- → P1
Target Milestone: --- → mozilla1.9 M9
Blocks: fox3access
This patch changes nsEventStateManager::ShiftFocusInternal() so that it won't try to focus a document if the document is an <iframe>. The problem here is that if a document is contained inside an iframe, and it focuses itself, it can't break out to focus its parent container.
Attachment #282059 - Flags: review?(peterv)
Comment on attachment 282059 [details] [diff] [review]
Patch - changes looping conditions in nsEventStateManager

The patch breaks for example http://www.ratol.fi/opensource/xhtml/iframe.htm
Use tab (3 tab presses) to focus link "Tämä avaa toisen sivun IFRAME-kehykseen" and then try to
navigate back using shift-tab. With the patch focus moves to *next* link, without the patch focus moves
to previous iframe.
Attachment #282059 - Flags: review?(peterv) → review-
You would be my hero if the next version of the patch included Mochitests for both the problem described by this bug and the situation described in comment 5.
Flags: in-testsuite?
When writing the testcase, 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/test/test_bug238987.html&rev=1.3&mark=24,31-46,55,78-85,93,114#24
is possibly useful. It shows which key events needs to be dispatched and
how to make the test to work also on Mac.
This is a more complex test case, with 2 designMode and one normal non-editable iframe. The iframes contain elements that can be tabbed between as well.
Target Milestone: mozilla1.9 M9 → ---
This patch extends the previous patch, and handles tabbing inside an iframe. It also sets the caret to follow the focus on tabbing, so if you tab around in an iframe, the caret will follow where you're tabbing. Without this, when you enter text or click the mouse in an iframe, the tabbing gets stuck if there's tabbable content in the iframe (e.g. a link).
Attachment #282059 - Attachment is obsolete: true
Attachment #282355 - Flags: review?(peterv)
Targeting M9 as this bug will block beta.
Target Milestone: --- → mozilla1.9 M9
(In reply to comment #6)
> You would be my hero if the next version of the patch included Mochitests for
> both the problem described by this bug and the situation described in comment
> 5.
> 

Ok, I'll try and get a mochitest for this... It would be good if some guys could look at my previous patch and tell me if what I'm doing is sane. Would making the caret follow the focus when tabbing inside a designMode <iframe> cause problems?
Comment on attachment 282355 [details] [diff] [review]
Patch - handles tabbing inside iframes, and sets caret to follow focus when tabbing

I'm pulling this patch, I realised it breaks tabindex.
Attachment #282355 - Attachment is obsolete: true
Attachment #282355 - Flags: review?(peterv)
This patch extends the previous, and fixes the breakage in tabindex. Please test and review.
Attachment #282655 - Flags: review?(peterv)
Why is this a beta blocker?
Whiteboard: [dbaron-1.9:RwCc]
Comment on attachment 282655 [details] [diff] [review]
Patch - No longer breaks tabindex, shift-tab works...

>Index: content/events/src/nsEventStateManager.cpp
>===================================================================

>+    nsCOMPtr<nsISupports> pcContainer = aPresContext->GetContainer();
>+    nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(pcContainer));
>+    nsCOMPtr<nsIEditorDocShell> editorDocShell(do_QueryInterface(treeItem));

No need for treeItem.

>+      if (IsEditableDoc(mPresContext)) {
>+        // Move the caret to match the new focus. This way when we're tabbing
>+        // inside an editable <iframe> the caret follows the focus.
>+        MoveCaretToFocusInner(PR_TRUE);
>+      }

This resets the selection if you tab within a document that has a contentEditable element. It seems wrong that this clears the selection and I also don't really understand why this is where it is, shouldn't it be in TabIntoDocument?
> >+      if (IsEditableDoc(mPresContext)) {
> >+        // Move the caret to match the new focus. This way when we're tabbing
> >+        // inside an editable <iframe> the caret follows the focus.
> >+        MoveCaretToFocusInner(PR_TRUE);
> >+      }
> 
> This resets the selection if you tab within a document that has a
> contentEditable element. It seems wrong that this clears the selection and I
> also don't really understand why this is where it is, shouldn't it be in
> TabIntoDocument?
> 

This is here so that the caret follows the tabbing. Without this, the tabbing gets stuck between tabbable elements inside an editable iframe. This is because we by start tabbing from the last caret position, if it exists. So when we tab, it goes from the caret to the next tabbable element, and the next time we tab, it will go back and start searching from the caret again, tabbing onto the same element.

After playing with it a bit more today I see that my last patch is not a complete solution. If you shift-tab right after loading the "more comprehensive test case" it will skip the bottom non-editable iframe. It will also skip the contents of the editable iframes. This is because the caret appears at the start of the iframe when it takes focus, and then we begin searching for the next tabbable content at the start of the iframe when tabbing backwards, and we then tab out of the iframe. 

So I guess that patch is not a complete fix, but it is "less broken"...

Currently things like links and buttons are focusable inside content editable documents. How do we want to handle the caret and focus when tabbing inside an editable document?

It makes sense to me to have the caret follow the focus when tabbing, i.e. when you press tab, the focus jumps to that so you can begin typing where you've tabbed to. This would follow the convention that when you tab to a focusable element (for example a textfield in a form) the caret appears so you can type where you've focused.

The alternative is to not have the caret follow focus, which is I think the intention of a lot the code that's currently in there. This however makes me wonder why we bother focusing the focusables inside the content-editable document then, as tabbing serves no purpose other than to look around the document; you can't press space to activate a focused link, as that will insert a space at the caret position, which will also revert the view back to where the caret was anyway, "undoing" your looking around.

So we have the following options:
1. Have the caret follow focus in content editable when tabbing.
2. Have the caret not move when tabbing, and have text entry enter text at the caret, not activating actionable focusables.
3. Just not focus focusables in content editable documents.

Which are we aiming for?
I see caret following focus inside contentEditable elements in a document. I can't tab into a document with contentEditable elements, which is a bug we'll need to fix.
I'm not sure why you're bringing up caret follows focus. AFAICT the issue with this bug is that the editor's focus listener is not turning off the caret/selection when the document itself is focused, and so shift-tabbing from the document starts from the caret/selection.

And in reply to comment 14, I don't think this is a M9 blocker (a 1.9 blocker for sure).
Attached patch Alternate patch (obsolete) — Splinter Review
I'm not convinced it's the right thing to do but it fixes this bug by making us always focus the document and not the root node in a designmode document.
Attached patch Alternate patch v2 (obsolete) — Splinter Review
This fixes most of the issues for me.
-Don't start from selection if we're shift-tabbing and the document is focused
-Don't focus the root element in designmode documents
-Remove selection in documents with contenteditable nodes on blur
Attachment #285989 - Attachment is obsolete: true
Retargeting to M10 per comment 18.
Target Milestone: mozilla1.9 M9 → mozilla1.9 M10
Attached patch Alternate patch v2.1 (obsolete) — Splinter Review
Doesn't regress tabbing from selection.
Attachment #286020 - Attachment is obsolete: true
(In reply to comment #18)
> I see caret following focus inside contentEditable elements in a document. I
> can't tab into a document with contentEditable elements, which is a bug we'll
> need to fix.
> I'm not sure why you're bringing up caret follows focus. AFAICT the issue with
> this bug is that the editor's focus listener is not turning off the
> caret/selection when the document itself is focused, and so shift-tabbing from
> the document starts from the caret/selection.
> 
> And in reply to comment 14, I don't think this is a M9 blocker (a 1.9 blocker
> for sure).
> 

Ah, I see this is a separate bug. We're not shifting focus properly when there's focusables in a design mode iframe. We get "stuck" between two focusable elements in the iframe. Just try tabbing through the "more comprehensive test case" and you should see what I mean. I'll file a new bug for this.
(In reply to comment #22)
> Created an attachment (id=286035) [details]
> Alternate patch v2.1
> 
> Doesn't regress tabbing from selection.
> 

This patch also fixes bug 389097 - "shift+tab doesn't work going from compose body back to subject in both thunderbird and seamonkey", so I'll mark bug 389097 as a duplicate of this one.

Duplicate of this bug: 389097
What's going on with this bug? There are patches. The older one is awaiting review and the newer "alternative" one isn't.

What can we do to put this one to bed?
Attached patch Alternate patch v2.2 (diff -w) (obsolete) — Splinter Review
Attachment #286035 - Attachment is obsolete: true
Comment on attachment 289531 [details] [diff] [review]
Alternate patch v2.2 (diff -w)

* Don't make the root element of a designmode document focusable (makes it the
  way it was in FF2)
* Remove the selection when we get a blur event for a contentEditable element.
  The focus code starts from the selection when tabbing and the document is
  focussed. So currently if we're tabbing away from a contentEditable element
  we'll focus the document (without clearing the selection) and then focus the element again. Clearing the selection fixes that.
Attachment #289531 - Attachment description: Alternate patch v2.2 → Alternate patch v2.2 (diff -w)
Attachment #289531 - Flags: superreview?(jst)
Attachment #289531 - Flags: review?(jst)
Attachment #289531 - Flags: superreview?(jst)
Attachment #289531 - Flags: superreview+
Attachment #289531 - Flags: review?(jst)
Attachment #289531 - Flags: review+
This bug has r+/sr+ and is blocking 1.9+. It should be able to go in at any time.
I already tried landing it for beta2 and it broke some mochitests, so it needs some more work.
Attachment #282655 - Attachment is obsolete: true
Attachment #289531 - Attachment is obsolete: true
Attachment #282655 - Flags: review?(peterv)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9 M10 → mozilla1.9 M11
Depends on: 413678
You need to log in before you can comment on or make changes to this bug.