390278 - Shift+tab no longer navigates out of MIDAS area

Status: RESOLVED FIXED

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

Description

[Aaron Leventhal]

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.

Comment 1

[Aaron Leventhal]

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.

Comment 2

[Martijn Wargers (dead)]

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 ;)

Comment 3

[Martijn Wargers (dead)]

Probably related to/the same bug as bug 389097.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - changes looping conditions in nsEventStateManager

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: More comprehensive test case, with tabbable elements inside iframes

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.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - handles tabbing inside iframes, and sets caret to follow focus when tabbing

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).

Comment 10

[Damon Sicore (:damons)]

Targeting M9 as this bug will block beta.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - No longer breaks tabindex, shift-tab works...

This patch extends the previous, and fixes the breakage in tabindex. Please test and review.

Comment 14

[Mike Schroepfer]

Why is this a beta blocker?

Comment 15

[Peter Van der Beken [:peterv]]

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?

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

> >+      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"...

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 18

[Peter Van der Beken [:peterv]]

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).

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: Alternate patch

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.

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: Alternate patch v2

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

Comment 21

[Damon Sicore (:damons)]

Retargeting to M10 per comment 18.

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: Alternate patch v2.1

Doesn't regress tabbing from selection.

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 26

[Aaron Leventhal]

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?

Comment 27

[Peter Van der Beken [:peterv]]

Attachment: Alternate patch v2.2 (diff -w)

Comment 28

[Peter Van der Beken [:peterv]]

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.

Comment 29

[Aaron Leventhal]

This bug has r+/sr+ and is blocking 1.9+. It should be able to go in at any time.

Comment 30

[Peter Van der Beken [:peterv]]

I already tried landing it for beta2 and it broke some mochitests, so it needs some more work.

Comment 31

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in