87321 - [Access] Shift+Tab has duplicate behavior (event handled twice)

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[kmurray1115]

Using 0.9.1 (commercial build PR1) on Win2K:

In Mail compose, when editing a bulleted list item, and using shift+tab to
outdent, the caret does the following *two* things:
1. outdents the list item
2. jumps back to the subject field

Expected behavior:
When outdenting list items using Shift+tab, we should disable #2 above, but
contextually allow #1.

Comment 1

[rubydoo123]

off to the mail team

Comment 2

[rubydoo123]

changed owner

Comment 3

[Asa Dotzler [:asa]]

OK, this makes HTML message compose completely suck for lists. There is,
however, a pretty simple workaround. Shift+Tab will outdent and focus subject
field. Hitting Tab will return focus to the last outdented position. So
Shift+tab then Tab gets you what you want. 

Comment 4

[Asa Dotzler [:asa]]

An additional workaround is to hit Enter to outdent (instead of Shift+Tab). 

Comment 5

[Scott MacGregor]

not a stop ship bug for eMojo 

Comment 6

[scottputterman]

moving to 0.9.7 and reassigning to varada.

Comment 7

[varada]

This can be argued in another way - The user might want Shift-Tab to get from
the Message Text to the Subject line and not just do a outdent. It is confusing
to have one action do two things. Why dont we decide on one or the other and
give different keys for the two?

Comment 8

[kmurray1115]

Copying Aaronl and JGlick. Comments?

Comment 9

[jglick]

I agree with Varada.

Comment 10

[varada]

If the necessary Accessibility keys can be decided on then we can try to get the
fix in for this deadline. Can the folks in charge of that decide and agree on
the keys by today?

Comment 11

[jglick]

Editor folks need to agree and Aaron would need to suggest an available 
accelerator.

Comment 12

[Aaron Leventhal]

So, is it correct that Accel+plus/minus are taken, as well as Accel+[/]?

Here are some not so perfect, but usable, possibilities:
1. change the behavior of Tab/Shift+tab only when text is selected and in an
editor control.
2. accel+9/0 -- these should be i18n, and on English keyboards at least, they're
the unshifted versions of ( and )
3. Alt+brackets or Alt+plus/minus

Comment 13

[kmurray1115]

my original vote was for option #1. Still is.

Comment 14

[varada]

I think we need to reach some sort of accord so that we can go ahead and start
implementing this.
Can anyone take the lead on this?
Jennifer?

Comment 15

[kmurray1115]

At some point that pre-dates me, the editor team changed the indent/outdent
accelerators from ctrl+/ctrl-(4.x) to tab/shift+tab. That might have been Ok if
they didn't also add support for the same outcome via ctrl+]/ctrl+[ (try it,
they both work!). So, why don't we go back to ctrl+/ctrl- as the accelerators
for indent/outdent, and reserve tab/shift+tab for bouncing between fields?

If someone has a specific alternative recommendation, please state it soon so we
can progress. Varada, if you don't hear an alternative recommendation by EOD
Wednesday, 2/27, go ahead and implement the scenario I suggested.

Comment 16

[jglick]

Sounds good to me.

Comment 17

[Kathleen :Brade]

In Composer, here are some of the the current mappings:
  control-+ (control-=) --> larger font size
  control--             --> smaller font size
  control-[             --> outdent
  control-]             --> indent
  tab                   --> used to navigate table cells and indent list items
  shift-tab             --> used to navigate table cells and outdent list items
          (notice we have problems here if you have a list in a table!)
Note: only tab and shift-tab are accessible on a European (or non-US) keyboard.

To me (a developer), the *big* issue here is that the event is handled twice
(not that it is getting handled by the wrong widget).  That issue needs to be
investigated ASAP (who is ignoring flag, focus? editor?)

Next, if after the previous issue is addressed, there is still a problem
(editing widget handles tab instead of focus), then we'll probably need to
special case the tab and shift-tab key events in editor code to not do what they
do in Composer if certain conditions (flags?) are met.

By the way, in Netscape 6.2, Composer handled the tab (focus wasn't changed)

Comment 18

[Charles Manske]

Composer has bug 98624 to deal with the VERY messy issue of tab key behavior.
It is grossly overloaded. IMHO, using it to change focus from editing window
to other parts of Mail Composer UI is absolutely the LAST thing it should be 
used for.

Comment 19

[varada]

So where do go from here?

Comment 20

[kmurray1115]

Given the info from brade, we should remove the ability to use tab to indent and
shift-tab to outdent. We already have an accelertator to do that (ctrl]/ctrl[).
Charlie, I understand your position, but we've made a broader decision to use
tab to move from element to element in the browser, and considerable work has
been done in the mail three pane window to tab from elememt to element there.
the natural extension of that in mail compose is to use tab to move from element
to element there and use ctrl+* within the message body to perform
indent/outdent operations.
 
Go for it varada.

KM

Comment 21

[Kathleen :Brade]

Kevin--I think that is entirely the wrong thing to do.
Control-[ and Control-] (as well as -,=, and +) are *not*
international-friendly.  Those keybindings can't be used *AT ALL* on non-US
keyboards.  We really should be moving away from *ALL* of those (at least until
we have configurable keybindings which won't happen for quite some time).

I think the right thing to do is find out why the event is getting handled
twice.  Once we know what's going on there and address that issue, we can decide
how to proceed (possibly disabling tab key editing functions in mail compose only).

Honestly, I *really* believe that it is getting late to be changing *ANY* of
this stuff without introducing risks of huge regressions.  This bug is more of
an RFE/enhancement than a bug.  (Prior versions of Mail Compose only allowed for
users to reverse tab to get out of editing area.)

Varada--please let me know if you need help debugging the event stuff.  If we
can't nail that it's pointless to try to fix this bug since someone else may
"fix" a bug and cause any band-aid fix we do to regress in a worse way.

Comment 22

[kmurray1115]

Control-[ and Control-] (as well as -,=, and +) are *not*
international-friendly.

Somebody made the decision to support that without regard for international
then. So what's the low-cost and quick solution to this? 

I think the right thing to do is find out why the event is getting handled
twice.  Once we know what's going on there and address that issue, we can decide
how to proceed (possibly disabling tab key editing functions in mail compose only).

Sure - if it gets us where we need to be quickly, fine. But what we don't have
the luxury of doing is investigating this well outside the scope of 1.0. Absent
the ability to make real headway on solving the tough issues you cite above, we
should remove the redundancy.

Comment 23

[timeless]

i think that we could alter the behavior of ctrl+[shift+]arrows so that they 
walked words and nodes. leaving [shift+]tab for indentation.

but i'm still trying to puzzle this through.

Comment 24

[Kathleen :Brade]

I don't see the "double-behavior" any more.  Does anyone else?

Comment 25

[Kathleen :Brade]

nevermind; I was testing the wrong thing; I do see the problem

Comment 26

[Kathleen :Brade]

OK, I checked... Composer gets the event and handles it.  It correctly sets the
"event handled" flag.  The bug is coming later in focus when focus isn't paying
attention to the "handled" flag.

AaronL--do you know anything about this stuff or do we need to ship this bug off
to Chris Saari?

Comment 27

[rubydoo123]

removing myself from the cc list

Comment 28

[Kathleen :Brade]

Chris Saari--I think your expertise is needed here.  Aaron can't help with this
bug right now.  :-(

Varada--you should probably reassign this bug to Chris Saari or talk to him and
see if there is a better person to reassign this to.

Comment 29

[saari (gone)]

nsbeta1- as per ADT

Comment 30

[Daniel Clemente]

   Hi, what I would do is: a single Shift+Tab outdents the list. If, after that,
the next keystroke were another Shift+Tab, it would return to the Subject field.
   I think it's better not to send the user away while he/she's typing the mail...

Comment 31

[(not reading, please use [e-mail] instead)]

taking all of varada's bugs.

Comment 32

[Kathleen :Brade]

*** Bug 84454 has been marked as a duplicate of this bug. ***

Comment 33

[Kathleen :Brade]

*** Bug 179250 has been marked as a duplicate of this bug. ***

Comment 34

[Kathleen :Brade]

*** Bug 187253 has been marked as a duplicate of this bug. ***

Comment 35

[Serge Gautherie (:sgautherie)]

As a reminder,

bug 179250 (and possibly others) was about another case:
when editing a message without any list, Shift-Tab gets back to the subject
(:-)) but add a Tab in the message body (:-().

With obvious workaround of going back to the editor and removing the inserted
Tab (with Back, for example).

Comment 36

[[e-mail]]

Attachment: diff -uw

Comment 37

[[e-mail]]

Attachment: Proposed patch

Comment 38

[Jean-Francois Ducarroz]

Comment on attachment 112410 [details] [diff] [review]
Proposed patch

I'll let sspitzer do the review as I am too busy right know...

Comment 39

[[e-mail]]

brade thinks I need a couple of extra lines:
+      if (isShift)
+        return NS_OK; // don't type text for shift tabs
     }
     else if (keyCode == nsIDOMKeyEvent::DOM_VK_RETURN
              || keyCode == nsIDOMKeyEvent::DOM_VK_ENTER)
     {
+      aKeyEvent->PreventDefault();
       nsString empty;
       if (isShift && !(mFlags&eEditorPlaintextMask))
       {
@@ -1346,6 +1352,7 @@
     }
     else if (keyCode == nsIDOMKeyEvent::DOM_VK_ESCAPE)
     {
+      aKeyEvent->PreventDefault();
       // pass escape keypresses through as empty strings: needed forime support
       nsString empty;
       return TypedText(empty, eTypedText);

Comment 40

[[e-mail]]

Fixing component.

Comment 41

[(not reading, please use [e-mail] instead)]

Comment on attachment 112410 [details] [diff] [review]
Proposed patch

let's get a review from brade first, then I can sr.

Comment 42

[(not reading, please use [e-mail] instead)]

re-assign to neil.

Comment 43

[Kathleen :Brade]

Comment on attachment 112410 [details] [diff] [review]
Proposed patch

r=brade with the addition of the changes in comment 39

Comment 44

[(not reading, please use [e-mail] instead)]

Comment on attachment 112410 [details] [diff] [review]
Proposed patch

sr=sspitzer, once you make brade's requested changes.

can you attach that final path to this bug, for completeness?

Comment 45

[[e-mail]]

Attachment: Patch incorporating review comments for completeness

Comment 46

[[e-mail]]

Comment on attachment 112974 [details] [diff] [review]
Patch incorporating review comments for completeness

/me wishes there was an easier way to transfer r/sr

Comment 47

[(not reading, please use [e-mail] instead)]

Comment on attachment 112974 [details] [diff] [review]
Patch incorporating review comments for completeness

sr=sspitzer, assuming tab and shift tab still do the right thing in the other
scenarios.

Comment 48

[Kathleen :Brade]

Neil--I have your patch in my current tree and I just ran into a problem:
  inside a table in Composer, press tab
  expectation: move to next table cell
  actual result: 4 spaces inserted

I have lots of patches in my tree but yours seems like an obvious candidate for
this regression.  Can you verify that tabbing in lists and tables works with
your patch?

Thanks!

Comment 49

[[e-mail]]

Attachment: Fixed missing !

You're right, when I split up the condition I missed off a ! by mistake...
Testing this, I found an issue with shift+tab in editor, so I added a fix.

Comment 50

[[e-mail]]

Comment on attachment 113090 [details] [diff] [review]
Fixed missing !

The ! was missing on this line:
>+      if (!(mFlags & eEditorPlaintextMask)) {

Comment 51

[Kathleen :Brade]

Comment on attachment 113090 [details] [diff] [review]
Fixed missing !

I know this was in the old code but I'd like to see it fixed for consistency. 
For PRBool assignments, I prefer this format:	  PRBool isBlock = PR_FALSE; 
instead of: PRBool isBlock (PR_FALSE);

What is the new change in editor.xul?  What issue does that fix?  I'd prefer
not to make that change unless it fixes a known issue.

Comment 52

[(not reading, please use [e-mail] instead)]

waiting for r=brade before I sr.

Comment 53

[Kathleen :Brade]

Comment on attachment 113090 [details] [diff] [review]
Fixed missing !

r=brade IF the change to editor.xul include a check to only call
event.preventDefault() if the keypress is a tab.  I'm worried about regressions
for other keys which we don't want to handle.

Comment 54

[[e-mail]]

Attachment: brade was right, I was too hasty :-[

onkeypress="if (event.keyCode == KeyEvent.DOM_VK_TAB) event.preventDefault();"

Comment 55

[[e-mail]]

Comment on attachment 113090 [details] [diff] [review]
Fixed missing !

oops, forgot to mark obsolete

Comment 56

[Kathleen :Brade]

Comment on attachment 113392 [details] [diff] [review]
brade was right, I was too hasty :-[

r=brade

Comment 57

[(not reading, please use [e-mail] instead)]

Comment on attachment 113392 [details] [diff] [review]
brade was right, I was too hasty :-[

sr=sspitzer

Comment 58

[Asa Dotzler [:asa]]

Comment on attachment 113392 [details] [diff] [review]
brade was right, I was too hasty :-[

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 59

[[e-mail]]

Fix checked in.