175893 - Make XUL <tab>'s focusable

Status: RESOLVED FIXED

(Firefox :: Keyboard Navigation, defect, P2)

Description

[Aaron Leventhal]

1. We should make xul <tab>'s focusable (the label part gets the dotted outline)
2. If a user presses Ctrl+Tab, the next <tab> gets focused if it has no
focusable content, or if the current <tab> is focused

Comment 1

[Aaron Leventhal]

Tab labels are usually focusable in Windows XP.

What about Mac and Unix?

Comment 2

[Aaron Leventhal]

Attachment: Make <tab> focusable, add focus appearance for xul tabs in modern and classic, makes focus stay on tab label if navigating from another tab label

Also seems to fix bug 166001 and bug 130644.
Not sure what we should do for Mac.
Why is there no tabbox.css for unix?

Needs testing. Timeless?

Comment 3

[:deanis74]

On Windows, if a tab has focus you can use the left/right arrows to navigate them.

Comment 4

[Aaron Leventhal]

Dean, that works with this patch, because we used to have this behavior in Classic.

Comment 5

[:deanis74]

This works pretty well.  The only thing I notice is in the (Phoenix) page info
dialog.  I used Ctrl+Tab to move through the tabs, without the tabs having
focus.  When I got to the Security tab, focus was on Help.  Pressing Ctrl+Tab
moved focus to the Security tab.  Is this expected?

Comment 6

[Aaron Leventhal]

Dean, that's bug 166001 -- which it turns out is not fixed by this.

Working on that now.

Comment 7

[Aaron Leventhal]

Attachment: This time, it really does fix all 3 bugs: this one, bug 166001 and bug 130644

Seeking r=, sr=

Comment 8

[Akkana Peck]

Tabs are focusable on linux, at least in the few tabbed apps I've seen.

> Why is there no tabbox.css for unix?

It might be that it uses the Windows one.

Your patch sort-of works on unix: if I tab from the urlbar, it lands on the
current tab, and pressing right/left arrow will switch to an adjacent tab (I
expected it would focus the tab, but not switch to it unless I hit return). 
Once I'm at the enw tab, left/right arrows no longer move between tabs; I'm not
sure where focus is, though the tab name is still outlined.  Hitting tab again
sends focus into the page, but the name of the tab still remains outlined (so
now I have a dotted outline around the banner image in the page, and a dotted
outline around the tab text).  (This is with the default classic theme, today's
build, linux.)

Comment 9

[sairuh (rarely reading bugmail)]

how would this fix affect embed apps, especially those which don't use xul? just
wondering.

Comment 10

[Aaron Leventhal]

This won't affect embedded apps at all, because the changes are in Mozilla's
chrome layer.

Comment 11

[Aaron Leventhal]

Perhaps I should change this so that tabs in the tabbed browser view are not
focusable -- just the tabs in tabbed dialogs would be. Or not?

Comment 12

[Aaron Leventhal]

Attachment: Fixes problems reported by Akkana, wrt to tabbed browser tabs

Seeking r=/sr=

Comment 13

[Samir Gehani]

Comment on attachment 103901 [details] [diff] [review]
Fixes problems reported by Akkana, wrt to tabbed browser tabs

>+      <xul:hbox class="dialog-button-box" pack="end" anonid="Buttons"

Since this is not part of this patch please remember not to check it in.

Comment 14

[Aaron Leventhal]

Attachment: Get rid of anonid that was not part of this patch.

Comment 15

[Samir Gehani]

Comment on attachment 103901 [details] [diff] [review]
Fixes problems reported by Akkana, wrt to tabbed browser tabs

r=sgehani

Comment 16

[Samir Gehani]

Comment on attachment 104198 [details] [diff] [review]
Get rid of anonid that was not part of this patch.

r=sgehani

Comment 17

[Brian Ryner (not reading)]

Just to confirm what Akkana said: tab labels do get focused on click in gtk, and
unix does use the windows tabbox.css.

Comment 18

[David Hyatt]

Mousing down on a tab should not focus it.  How are you handling that case?  
Tabs are one of the reasons CSS3 was going to split the focusable properties 
(what we call -moz-user-focus) into both key and mouse variants.

Comment 19

[David Hyatt]

Sorry, I should have said "On Win32, mousing down on a tab doesn't focus it."

Comment 20

[David Hyatt]

I would suggest implementing two new CSS properties:

-moz-user-mouse-focus
-moz-user-key-focus

with the same range of values as -moz-user-focus.

Then the CSS for tabs can be changed to

tab {
  -moz-user-key-focus: normal;
#ifdef XP_UNIX
  -moz-user-mouse-focus: normal;
#endif
}

The tabbing code would need to be patched to check for user-key-focus followed 
by user-focus.  The mouse code would need to check user-mouse-focus followed 
by user-focus.

Comment 21

[Aaron Leventhal]

It seems like all we really need to do is modify the "click" handler in
tabbox.xml. It just needs to set the focus there.

Comment 22

[Simon Fraser [no longer active]]

I think we should really implement the two kinds of focus as hyatt suggets. It's
not just tabs; buttons have the same issue. Clicking a button should not give it
focus.

Comment 23

[Aaron Leventhal]

Buttons in windows actually take focus when you click on them.
On mac buttons don't take focus at all, so there's no need for a separate
property because of buttons.

And tabs even take focus from the mouse if another tab was focused last, or if
you were already focused on that tab.

Comment 24

[Aaron Leventhal]

Attachment: Makes sure that mouse behavior is correct for tabs. Note they are focusable on windows, depending on what was focused before

Seekign sr=

Comment 25

[Aaron Leventhal]

Comment on attachment 104601 [details] [diff] [review]
Makes sure that mouse behavior is correct for tabs. Note they are focusable on windows, depending on what was focused before

Carrying r= forward

Comment 26

[[e-mail]]

Comment on attachment 104601 [details] [diff] [review]
Makes sure that mouse behavior is correct for tabs. Note they are focusable on windows, depending on what was focused before

>Index: mozilla/xpfe/global/resources/content/bindings/dialog.xml
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/dialog.xml,v
>retrieving revision 1.21
>diff -u -r1.21 dialog.xml
>--- mozilla/xpfe/global/resources/content/bindings/dialog.xml	9 May 2002 00:04:36 -0000	1.21
>+++ mozilla/xpfe/global/resources/content/bindings/dialog.xml	30 Oct 2002 04:02:38 -0000
>@@ -187,8 +187,13 @@
>             var button = buttons[dlgtype];
>             buttons[dlgtype].addEventListener("command", this._handleButtonCommand, true);
>             // don't override custom labels with pre-defined labels on explicit buttons
>-            if (!button.hasAttribute("label"))
>+            if (!button.hasAttribute("label")) {
>               button.setAttribute("label", this.mStrBundle.GetStringFromName("button-"+dlgtype));
>+              var accessKey = this.mStrBundle.GetStringFromName("accesskey-"+dlgtype);
>+              if (accessKey) {
>+                button.setAttribute("accesskey", accessKey);
>+              }
>+            }
>           }
>           
>           // ensure that hitting enter triggers ondialogaccept
This is part of another (checked-in) patch, right?
>@@ -304,12 +309,30 @@
>         </body>
>       </method>
> 
>+      <method name="_fireTabboxEvent">
>+        <parameter name="aEvent"/>
>+        <body>
>+        <![CDATA[
>+          var tabbox = this.ownerDocument.getElementsByTagName('tabbox');
>+          if (tabbox && tabbox.length > 0) {
>+            tabbox[0].dispatchEvent(aEvent);
>+            event.stopPropagation();
>+            event.preventDefault();
>+          }
>+        ]]>
>+        </body>
>+      </method>
>+
>     </implementation>
>     
>     <handlers>
>       <handler event="keypress" keycode="VK_ENTER" action="this._hitEnter();"/>
>       <handler event="keypress" keycode="VK_RETURN" action="this._hitEnter();"/>
>       <handler event="keypress" keycode="VK_ESCAPE" action="this.cancelDialog();"/>
>+      <handler event="keypress" keycode="VK_TAB" modifiers="control" action="this._fireTabboxEvent(event);"/>
>+      <handler event="keypress" keycode="VK_TAB" modifiers="control,shift" action="this._fireTabboxEvent(event);"/>
>+      <handler event="keypress" keycode="VK_PAGE_UP" modifiers="control" action="this._fireTabboxEvent(event);"/>
>+      <handler event="keypress" keycode="VK_PAGE_DOWN" modifiers="control" action="this._fireTabboxEvent(event);"/>
>     </handlers>
> 
>   </binding>
I don't think this is the way to go. You're trying to avoid adding
eventnode="document" to all the affected <tabbox>es; probably nicer behaviour
would be to change the default in tabbox.xml:
<field name="_eventNode">
  var eventNode = top.document.documentElement.localName == 'dialog' ?
    top.document : this;
  switch (this.getAttribute("eventnode")) {
    case "top": eventNode = top.document; break;
    case "this": eventNode = this; break;
    case "parent": eventNode = this.parentNode; break;
    case "window": eventNode = window; break;
    case "document": eventNode = document; break;
  }
  eventNode;
</field>
Or just change the default to top.document and fix those tabs that break :-)
>Index: mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v
>retrieving revision 1.63
>diff -u -r1.63 tabbrowser.xml
>--- mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml	8 Oct 2002 23:19:03 -0000	1.63
>+++ mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml	30 Oct 2002 04:02:41 -0000
>@@ -438,7 +438,10 @@
>             }
> 
>             // Focus our new content area.
>-            setTimeout("window._content.focus()", 0);
>+            var focusedElt = document.commandDispatcher.focusedElement;
>+            if (!focusedElt || focusedElt.localName != 'tab' || 
>+                focusedElt.namespaceURI != 'http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul')
>+              setTimeout("window._content.focus()", 0);
>           ]]>
>         </body>
>       </method>
Why not:
if (document.commandDispatcher.focusedElement != this.selectedTab)
>Index: mozilla/xpfe/global/resources/content/bindings/tabbox.xml
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbox.xml,v
>retrieving revision 1.26
>diff -u -r1.26 tabbox.xml
>--- mozilla/xpfe/global/resources/content/bindings/tabbox.xml	15 Oct 2002 08:50:01 -0000	1.26
>+++ mozilla/xpfe/global/resources/content/bindings/tabbox.xml	30 Oct 2002 04:02:41 -0000
>@@ -198,6 +198,7 @@
>         var o = this.getAttribute("orient");
>         if (!o)
>           this.setAttribute("orient", "horizontal");
>+        document.commandDispatcher.advanceFocusIntoSubtree(this.selectedItem);
>       ]]>
>       </constructor>
>       
I don't think you want to be messing about with the focus here.
>@@ -287,6 +288,8 @@
>         <body>
>         <![CDATA[
>           var startTab = this.selectedItem;
>+          var isTabLabelFocused = startTab && 
>+            document.commandDispatcher.focusedElement == startTab;
>           var next = startTab[aDir == -1 ? "previousSibling" : "nextSibling"];
> 
>           while (next != startTab && (!next || next.getAttribute("hidden"))) {
You don't need the startTab && test, startTab is "guaranteed".
>@@ -299,7 +302,8 @@
>           if (next && next != startTab) {
>             this.selectedItem = next;
>             next.focus();
>-            document.commandDispatcher.advanceFocusIntoSubtree(next);
>+            if (!isTabLabelFocused)  // otherwise keep focusing tab labels
>+              document.commandDispatcher.advanceFocusIntoSubtree(next);
>           }
>         ]]>
>         </body>
What I don't like about advanceFocusIntoSubtree is that it can advance the
focus right out of the tab control to the OK button (or whatever) if there are
no focusable elements in the tab panel. (Windows behaviour is to focus the tab
if there are no focusable elements in the tab panel).
>@@ -486,7 +490,17 @@
>     <handlers>
>       <handler event="click" button="0">
>       <![CDATA[
>+        var isTabLabelFocused = document.commandDispatcher.focusedElement == this.parentNode.selectedItem;
>         this.parentNode.selectedItem = this;
>+        if (!isTabLabelFocused) { // otherwise keep focusing tab labels
>+          if (window._content)
>+            window._content.focus();
>+          else 
>+            document.commandDispatcher.advanceFocusIntoSubtree(this);
>+        }
>+        else 
>+          this.focus();
>+
>       ]]>
>       </handler>
>       
I don't think you need a content (not _content, anyway) check. Perhaps this and
the ctrl+tab code could be merged into a "select and focus tab" method?
>Index: mozilla/themes/classic/global/win/tabbox.css
>===================================================================
>RCS file: /cvsroot/mozilla/themes/classic/global/win/tabbox.css,v
>retrieving revision 1.10
>diff -u -r1.10 tabbox.css
>--- mozilla/themes/classic/global/win/tabbox.css	22 Jan 2002 23:07:59 -0000	1.10
>+++ mozilla/themes/classic/global/win/tabbox.css	30 Oct 2002 04:02:42 -0000
>@@ -76,9 +76,14 @@
> }
> 
> tab[selected="true"] {
>+  -moz-user-focus: normal;
>   margin-top: 0;
>   border-bottom-color: transparent;
>   padding: 1px 6px 4px 6px;
>+}
>+
>+tab:focus {
>+  -moz-outline: 1px dotted invert;
> }
> 
> tab[beforeselected="true"] {
Isn't there somewhere better to put the outline, such as on the text?
>Index: mozilla/xpfe/browser/resources/content/pageInfo.xul
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/pageInfo.xul,v
>retrieving revision 1.40
>diff -u -r1.40 pageInfo.xul
>--- mozilla/xpfe/browser/resources/content/pageInfo.xul	17 Oct 2002 19:30:13 -0000	1.40
>+++ mozilla/xpfe/browser/resources/content/pageInfo.xul	30 Oct 2002 04:02:42 -0000
>@@ -37,13 +37,15 @@
>   %pageInfoDTD;
> ]>
> 
>-<window id="main-window"
>+<dialog buttons="help"
>+  id="main-window"
>   xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>   windowtype="Browser:page-info"
>   onload="onLoadPageInfo()"
>   align="stretch" class="dialog"
>   width="&pageInfoWindow.width;" height="&pageInfoWindow.height;"
>   screenX="10" screenY="10"
>+  ondialoghelp="return doHelpButton()"
>   persist="screenX screenY width height sizemode">
> 
>   <script type="application/x-javascript" src="chrome://communicator/content/contentAreaUtils.js"/>
>@@ -315,8 +317,4 @@
>       <!-- Others added by overlay -->
>     </tabpanels>
>   </tabbox>
>-  <box>
>-    <spacer flex="1"/>
>-    <button label="&helpButton;" oncommand="doHelpButton();" />
>-  </box>
>-</window>
>+</dialog>
I agree with this bit :-)

Comment 27

[Aaron Leventhal]

Attachment: New patch based on Neil's comments

Neil, your reviews are nice and thorough. I hope they make you an sr=.

Let me deal with your comments one by one:
> This is part of another (checked-in) patch, right?
Yes, I removed that part from the new patch

> I don't think this is the way to go. You're trying to avoid adding
> eventnode="document" to all the affected <tabbox>es; probably nicer behaviour

> would be to change the default in tabbox.xml:
Your way is better -- that's in the new patch.

> Why not:
> if (document.commandDispatcher.focusedElement != this.selectedTab)
I tried this. It won't work, because this.selectedTab still has the old tab
where the focus is coming from, not the new one which is in focusedElement.

>You don't need the startTab && test, startTab is "guaranteed".
Okay, fixed.

> What I don't like about advanceFocusIntoSubtree is that it can advance the
> focus right out of the tab control to the OK button (or whatever) if there
are
> no focusable elements in the tab panel. (Windows behaviour is to focus the
tab
> if there are no focusable elements in the tab panel).
Okay, now it checks to see if it ends up on a anonymous dialog button
by looking to see if the focus lands on something with the dlgtype attribute.

> I don't think you need a content (not _content, anyway) check. Perhaps this 
> and the ctrl+tab code could be merged into a "select and focus tab" method?
I need this for the tabbrowser scenario. Tried merging them into a single
method and couldn't get it to work right -- don't remember why at the moment.

> Isn't there somewhere better to put the outline, such as on the text?
No, the dotted outline looks funny and lopsided then.

Seeking r=Neil

Comment 28

[[e-mail]]

Comment on attachment 104817 [details] [diff] [review]
New patch based on Neil's comments

Well, the CSS is missing and I don't think you need the changes to dialog.xml
any more. I'm not even convinced you need to change tabbrowser.xml either; can
you possibly remove the tabbrowser focus code and rely on your tabbox code
instead?

Comment 29

[[e-mail]]

Attachment: bindings changes

This is what I was thinking of when I suggested merging the functions.

Comment 30

[Aaron Leventhal]

Neil, I tried your patch -- it's missing 2 things:

1) initial focus when I pull up page info should not be on the tab label itself,
but on the first item in that tab. That's what the dialog.xml changes did.

2) when you click on a tabbed browser tab, it puts a dotted outline around the
content area. We want it focused, but we don't want the dotted outline on
initial focus to a page. My changes to tabbrowser.xml kept that from occuring.

Comment 31

[Aaron Leventhal]

Attachment: Here's the same patch with the .css and pageinfo.xul files I forgot

Neil, I tried to use your version of tabbrowser.xml file. Eventually I got it
to have the correct initial focus for dialogs, but I had to add a parameter
aAdvanceIfSelected. Then I ran into problem #2, where the dotted outline kept
showing around the content when a browser tab was clicked on. Finally I decided
my original patch was pretty good, because it dealt with all of these issues
correctly.

IMHO I don't really think it's worth much more of our time to play more games
with this code. I appreciate the thorough reviews, though.

Seeking r=

Comment 32

[[e-mail]]

Comment on attachment 104887 [details] [diff] [review]
Here's the same patch with the .css and pageinfo.xul files I forgot

Thank you for explaining the dialog.xml changes. However I still have some
issues; I would appreciate it if you could clear them up.
1. The additions to tabbrowser.xml appear have no effect (i.e. simply
   removing the setTimeout line appears to have the same effect).
2. If you click one of the tabs in the DOM Inspector sidebar then the
   content will get focus, not (as I had hoped) the DOM Inspector panel.
3. Ctrl+Tab will still show the dotted outline around the content
   (but that might just be me fiddling; I was short of time today).
4. While you're fiddling around with the focus would you mind using
   Components.lookupMethod(content, 'focus').call(content); ?

Comment 33

[Kyle Yuan]

ccing our team

Comment 34

[[e-mail]]

I'm seeing a weird effect with tabbed browsing: if you have two tabs open, and
tab to links on those pages, then if you click on a <tab> to change tabs the
focus goes back to the link on the other page. So far so good. But if you then
tab to the <tab> and press an arrow key to switch tabs the link still appears to
have the focus; if you then press tab twice the focus will go to the next tab
stop after the link. (Also if the focus was on a form field then when you switch
tabs the focus doesn't appear on the form field although it is there really).

Comment 35

[sairuh (rarely reading bugmail)]

clearing milestone since 1.3a has passed.

Comment 36

[Samir Gehani]

adt: nsbeta1-

Comment 37

[Daniel Wang]

Comment on attachment 104887 [details] [diff] [review]
Here's the same patch with the .css and pageinfo.xul files I forgot

the pageInfo.xul portion has conflict with another bug, but we can ignore it
for now

Comment 38

[Philip K. Warren]

Attachment: Patch updated to trunk

Here is a version of the last patch updated to the trunk. I am not asking
reviews on it because I am seeing lots of issues with it. Among the issues:

- Sometimes tabs remain highlighted like they have the focus even though
another item has focus. I've even reproduced problems where multiple tabs show
the focused outline.
- On a AIX GTK2 build from a few days ago, I do not see the outline drawn
around focused tabs. Neil mentioned this may be a side effect of using
-moz-appearance, but I have not tracked down the problem yet.

Comment 39

[Philip K. Warren]

Another note on comment 38:

I don't see the outline drawn around the focused tabs in the classic theme on a
trunk AIX GTK2 build, but it does work in the modern theme.

Comment 40

[Doron Rosenberg (IBM)]

On linux/gtk2, it seems to work: http://nexgenmedia.net/Screenshot-18.png

Sometimes I have to click twice to get the outline though.

Comment 41

[[e-mail]]

(In reply to comment #40)
>Sometimes I have to click twice to get the outline though.
At least on windows, if you click on a background tab then the focus gets set to
the newly selected panel, rather than the tab.

Comment 42

[Doron Rosenberg (IBM)]

that seems to be the case.  I rebuild using gtk2/xft and it still works fine.

Comment 43

[Doron Rosenberg (IBM)]

I can sometimes get 2 tabs with an outline, trying to debug how that happens.

Comment 44

[Doron Rosenberg (IBM)]

To fix the 2 selected tabs at once, I had to do:

tabbox.css

 tab[beforeselected="true"] {
   -moz-appearance: tab-left-edge;
   border-right: none;
   -moz-border-radius-topright: 0;
+  -moz-outline: none;
 }

 tab[afterselected="true"] {
   -moz-appearance: tab-right-edge;
   border-left: none;
   -moz-border-radius-topleft: 0;
+  -moz-outline: none;
 }

That seems to have done it for me.

Comment 45

[Philip K. Warren]

The problem with the inverted box not displaying was due to my checkin for Bug
126066. I have backed it out until I get a better fix, but it should allow us to
proceed on this bug. The problematic -moz-outline behavior should be visible in
builds from 2004/06/18-2004/06/21.

Comment 46

[Aaron Leventhal]

Attachment: Patch brought up-to-date. Issues dealt with.

Comment 47

[Aaron Leventhal]

(In reply to comment #32)
> (From update of attachment 104887 [details] [diff] [review])
> Thank you for explaining the dialog.xml changes. However I still have some
> issues; I would appreciate it if you could clear them up.
> 1. The additions to tabbrowser.xml appear have no effect (i.e. simply
>    removing the setTimeout line appears to have the same effect).
Different code now. Have a look at the new patch.

> 2. If you click one of the tabs in the DOM Inspector sidebar then the
>    content will get focus, not (as I had hoped) the DOM Inspector panel.
This doesn't affect sidebar tabs. If we want to fix that it needs to be in a
different bug. The sidebar is definitely a drag on accessibility. See bug 89148.

> 3. Ctrl+Tab will still show the dotted outline around the content
>    (but that might just be me fiddling; I was short of time today).
Not having a problem with that.

> 4. While you're fiddling around with the focus would you mind using
>    Components.lookupMethod(content, 'focus').call(content); ?
Is that really necessary if I'm setting focus to a XUL tab?

The issue you mention in comment #34 is not present in my tests.

Comment 48

[Philip K. Warren]

There is one behavior I'm seeing with the latest patch which I'm not sure is
correct. To reproduce, do the following:

1) Load Mozilla. After the home page has loaded, click Ctrl+T to create a new
tab. Load http://www.google.com/ in the new tab.
2) Make sure the text focus is on the Google search text entry in the second tab.
3) Hit Ctrl+Tab several times, and notice that the focus stays in the text entry
on the Google tab.
4) Type Ctrl+Tab until you get back to the first tab. Click on the first tab to
highlight it.
5) Now click Ctrl+Tab - notice that on the Google page, the focus has been
changed to the tab and is no longer on the text entry.

I'm not sure if this is the intended behavior or not, but I would expect that
the focus is maintained when doing Ctrl+Tab.

Comment 49

[Aaron Leventhal]

(In reply to comment #48)
> 5) Now click Ctrl+Tab - notice that on the Google page, the focus has been
> changed to the tab and is no longer on the text entry.

This is intentional. I'm following Windows tab behavior. Once you have focus on
a tab, other keys to navigate between tabs keeps focus on one of the tabs.

Comment 50

[[e-mail]]

Comment on attachment 151547 [details] [diff] [review]
Patch brought up-to-date. Issues dealt with.

>+tab:focus {
>+  -moz-outline: 1px dotted invert;
>+}
You do know -moz-outline is deprecated, right ;-)

>+  -moz-outline: none;
I wonder why you think you need this...

>+      <method name="advanceFocus">
>+        <body>
>+          <![CDATA[
>+            document.commandDispatcher.advanceFocusIntoSubtree(this);
>+            // We don't want to focus on anonymous OK, Cancel, etc. buttons
>+            if (document.commandDispatcher.focusedElement.hasAttribute("dlgtype"))
>+              this.focus(); // focus label rather than focusing dialog button
>+          ]]>
>+        </body>
>+      </method>
Why is this here, and not in dialog.xml?

>       <handler event="click" button="0">
>       <![CDATA[
>+        var isTabLabelFocused = (document.commandDispatcher.focusedElement == this.parentNode.selectedItem);
>         this.parentNode.selectedItem = this;
>+        if (!isTabLabelFocused) { // otherwise keep focusing tab labels
>+          if (window._content)
>+            window._content.focus(); // Focus the content window for tabbed browser tab
>+          else
>+            document.commandDispatcher.advanceFocusIntoSubtree(this);
>+        }
>+        else
>+          this.focus();
>       ]]>
>       </handler>
Shouldn't this do the same tests as the keyboard navigation version? Maybe you
should have a shared method on <tabbox>.

>+            if (this.mCurrentTab == document.commandDispatcher.focusedElement) {
Nit: I think you put document.commandDispatcher.focusedElements on the left of
your other conditions.

>             // Focus the previously focused element or window
>             document.commandDispatcher.suppressFocusScroll = true;
>-            if (newBrowser.focusedElement) {
>+            if (newBrowser.focusedElement &&
>+                newBrowser.focusedElement.localName != "tab") {
I'm not sure that this gains - if focussing the focusedElement isn't possible
then the focusedWindow is focused, which iirc will try to focus the most
recently focused element anyway.

Comment 51

[Aaron Leventhal]

Attachment: New patch addressing Neil's comments

Comment 52

[Aaron Leventhal]

(In reply to comment #50)
> (From update of attachment 151547 [details] [diff] [review])
> >+tab:focus {
> >+  -moz-outline: 1px dotted invert;
> >+}
> You do know -moz-outline is deprecated, right ;-)
Not yet. When it is I'll change it.

> 
> >+  -moz-outline: none;
> I wonder why you think you need this...
Luckily don't need it anymore. There were some outline ghosts it cleaned up, but
it was a hack I suppose.

> 
> >+      <method name="advanceFocus">
> >+        <body>
> >+          <![CDATA[
> >+            document.commandDispatcher.advanceFocusIntoSubtree(this);
> >+            // We don't want to focus on anonymous OK, Cancel, etc. buttons
> >+            if
(document.commandDispatcher.focusedElement.hasAttribute("dlgtype"))
> >+              this.focus(); // focus label rather than focusing dialog button
> >+          ]]>
> >+        </body>
> >+      </method>
> Why is this here, and not in dialog.xml?
Right, moved to dialog.xml

> 
> >       <handler event="click" button="0">
> >       <![CDATA[
> >+        var isTabLabelFocused = (document.commandDispatcher.focusedElement
== this.parentNode.selectedItem);
> >         this.parentNode.selectedItem = this;
> >+        if (!isTabLabelFocused) { // otherwise keep focusing tab labels
> >+          if (window._content)
> >+            window._content.focus(); // Focus the content window for tabbed
browser tab
> >+          else
> >+            document.commandDispatcher.advanceFocusIntoSubtree(this);
> >+        }
> >+        else
> >+          this.focus();
> >       ]]>
> >       </handler>
> Shouldn't this do the same tests as the keyboard navigation version? Maybe you
> should have a shared method on <tabbox>.
You mean check to see if it's a hidden tab? It wouldn't be able to receive a
click then.

> 
> >+            if (this.mCurrentTab == document.commandDispatcher.focusedElement) {
> Nit: I think you put document.commandDispatcher.focusedElements on the left of
> your other conditions.
Fixed.

> 
> >             // Focus the previously focused element or window
> >             document.commandDispatcher.suppressFocusScroll = true;
> >-            if (newBrowser.focusedElement) {
> >+            if (newBrowser.focusedElement &&
> >+                newBrowser.focusedElement.localName != "tab") {
> I'm not sure that this gains - if focussing the focusedElement isn't possible
> then the focusedWindow is focused, which iirc will try to focus the most
> recently focused element anyway.
No it will focus the document itself if that's what was focused. Without this
code, if you click on some empty space in the document and then hit Ctrl+tab and
then ctrl+shift+tab, focus ends up on the tab itself.

Comment 53

[[e-mail]]

Comment on attachment 152134 [details] [diff] [review]
New patch addressing Neil's comments

>Index: xpfe/global/resources/content/bindings/tabbox.xml
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbox.xml,v
>retrieving revision 1.30
>diff -u -r1.30 tabbox.xml
>--- xpfe/global/resources/content/bindings/tabbox.xml	31 Oct 2003 03:56:22 -0000	1.30
>+++ xpfe/global/resources/content/bindings/tabbox.xml	1 Jul 2004 20:16:58 -0000
>@@ -301,11 +301,13 @@
>           }
> 
>           if (next && next != startTab) {
>-            this.selectedItem = next;
>-            if (this.getAttribute("setfocus") != "false") {
>+            if (document.commandDispatcher.focusedElement == startTab) {
>               next.focus();
>+            }
>+            else if (this.getAttribute("setfocus") != "false") {
>               document.commandDispatcher.advanceFocusIntoSubtree(next);
>             }
>+            this.selectedItem = next;
>           }
>         ]]>
>         </body>
>@@ -499,7 +501,16 @@
>     <handlers>
>       <handler event="click" button="0">
>       <![CDATA[
>+        var isTabLabelFocused = (document.commandDispatcher.focusedElement == this.parentNode.selectedItem);
>         this.parentNode.selectedItem = this;
>+        if (!isTabLabelFocused) { // otherwise keep focusing tab labels
>+          if (window._content)
>+            window._content.focus(); // Focus the content window for tabbed browser tab
>+          else
>+            document.commandDispatcher.advanceFocusIntoSubtree(this);
>+        }
>+        else
>+          this.focus();
>       ]]>
>       </handler>
>       
What I meant was that these two hunks use different tests to determine how to
update the focus.

>-            // Focus the previously focused element or window
>+            // Focus the previously focused element or window, unless it's a tab
>             document.commandDispatcher.suppressFocusScroll = true;
>-            if (newBrowser.focusedElement) {
>+            if (newBrowser.focusedElement &&
>+                newBrowser.focusedElement.localName != "tab") {
>               try {
>                 setFocus(newBrowser.focusedElement);
>               } catch (e) {
I see the unwanted effect without this hunk, unfortunately this doesn't quite
fix it correctly, as the focus is moved into never-never-land just before the
urlbar. Additionally this test won't work with tabs in content, it's probably
easier to compare the focusedElement's parent against a known element.

Comment 54

[Aaron Leventhal]

Attachment: Addressing more comments

* Added selectNewTab() to share code for selecting a new tab in click and
advanceSelectedTab()
* Instead of testing for localName=='tab', compared to known element
+	     if (newBrowser.focusedElement &&
+		 newBrowser.focusedElement != this.mCurrentTab) {

I was unable to duplicate any problem with focus getting lost where you click
in the empty space in page, hit Ctrl+Tab then Ctrl+Shift+Tab.

You sure that's right? If you hit arrow keys the page won't scroll? Can you
give me steps to reproduce that?

Comment 55

[[e-mail]]

As to your latest patch, if in a normal tabbed dialog you press ctrl+tab (or
otherwise change tab to the right) then everything works fine, but if you then
press ctrl+shift+tab (or otherwise change tab to the left) then the previously
selected tab element itself gets focus.

As to your previous patch, one of the issues I had was that if you load a XUL
web page that uses tabs then I was worried that you would not restore the focus.
Unfortunately your latest patch doesn't seem to handle it at all and if you're
not careful you'll find focus going to the previous tab element (again!)

Comment 56

[Aaron Leventhal]

Attachment: Fix selectNewTab method

I don't see any problems with this patch and remembering focus in web pages.

Comment 57

[Aaron Leventhal]

Attachment: Changes only to tabbrowser.xml. Really really tested.

Comment 58

[Aaron Leventhal]

Attachment: Adds a comment to explain setTimeout() calls

Comment 59

[[e-mail]]

I think that tabbrowser focus code is getting unwieldy... perhaps a variable for
what you wanted focused would help?

Comment 60

[Aaron Leventhal]

Attachment: Use variable whatToFocus in tabbrowser.xml

A further simplification was to get rid of the try/catch. I can't think of what
we'd need it for since we have a null check.

Comment 61

[Brian Ryner (not reading)]

Comment on attachment 152657 [details] [diff] [review]
Use variable whatToFocus in tabbrowser.xml

add the appropriate changes under toolkit/ and sr=me

Comment 62

[Aaron Leventhal]

Checking in themes/classic/global/win/tabbox.css;
/cvsroot/mozilla/themes/classic/global/win/tabbox.css,v  <--  tabbox.css
new revision: 1.12; previous revision: 1.11
done
Checking in themes/modern/global/tabbox.css;
/cvsroot/mozilla/themes/modern/global/tabbox.css,v  <--  tabbox.css
new revision: 1.10; previous revision: 1.9
done
Checking in xpfe/global/resources/content/bindings/dialog.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/dialog.xml,v  <-- 
dialog.xml
new revision: 1.26; previous revision: 1.25
done
Checking in xpfe/global/resources/content/bindings/tabbox.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbox.xml,v  <-- 
tabbox.xml
new revision: 1.31; previous revision: 1.30
done
Checking in xpfe/global/resources/content/bindings/tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <-- 
tabbrowser.xml
new revision: 1.101; previous revision: 1.100
done
Checking in toolkit/content/widgets/dialog.xml;
/cvsroot/mozilla/toolkit/content/widgets/dialog.xml,v  <--  dialog.xml
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/content/widgets/tabbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbox.xml,v  <--  tabbox.xml
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.40; previous revision: 1.39
done
Checking in toolkit/themes/qute/global/tabbox.css;
/cvsroot/mozilla/toolkit/themes/qute/global/tabbox.css,v  <--  tabbox.css
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/themes/winstripe/global/tabbox.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/tabbox.css,v  <--  tabbox.css
new revision: 1.3; previous revision: 1.2
done

Comment 63

[[e-mail]]

Oops. Moving the focus into the timeout means that the suppressFocusScroll never
takes effect :-[

Comment 64

[Aaron Leventhal]

Attachment: Duh, move scrolling suppression into local setFocus() and restore missing code from mozilla/toolkit version (for when tab bar is already focused)

Comment 65

[[e-mail]]

Comment on attachment 153697 [details] [diff] [review]
Duh, move scrolling suppression into local setFocus() and restore missing code from mozilla/toolkit version (for when tab bar is already focused)

sr=me on the xpfe patch if you don't move the setTimeout comment away from the
setTimeout call...

Comment 66

[Mike Connor [:mconnor]]

Comment on attachment 153697 [details] [diff] [review]
Duh, move scrolling suppression into local setFocus() and restore missing code from mozilla/toolkit version (for when tab bar is already focused)

okay, so that's twice that I read the whole patch and didn't submit the review.
 Neil therefore beat me to the comment bit.

Comment 67

[Aaron Leventhal]

Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.42; previous revision: 1.41
done
Checking in xpfe/global/resources/content/bindings/tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <-- 
tabbrowser.xml
new revision: 1.102; previous revision: 1.101
done

Comment 68

[Ian Neal]

Relanding relevant parts of patch following aviary branch landing

Comment 69

[Aaron Leventhal]

This has regressed since aviary stuff started getting merged in. With a build
from 12/2/2004 I cannot put focus on a tab label.

Comment 70

[Aaron Leventhal]

Attachment: mozilla/toolkit changes that were eaten by aviary branch landing

Comment 71

[Ian Neal]

Comment on attachment 167675 [details] [diff] [review]
mozilla/toolkit changes that were eaten by aviary branch landing

Was previously reviewed so doesn't need doing again - sorry I missed this first
time round, I only checked in the second patch.

Comment 72

[Ian Neal]

Relanding relevant parts of first patch following landing of aviary branch -
previously reviewed

Comment 73

[Ian Neal]

Relanding missing line from tabbrowser.xml following landing of aviary branch -
previously reviewed

Comment 74

[PikeUK]

(In reply to comment #72)
> Relanding relevant parts of first patch following landing of aviary branch -
> previously reviewed

There are still changes from the browser.css files that are missing (and
breaking the tab loading indicator in today's builds):

http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2004-07-24+15%3A50&maxdate=2004-07-24+16%3A00

Comment 75

[Aaron Leventhal]

Relanding missing lines from browser.css in qute and winstripe themes following
landing of aviary branch - previously reviewed.

Should fix the missing tab loading indicators.

Comment 76

[Ian Neal]

The only difference I see now is in tabbox.xml where the trues and falses have
been reversed:
http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Ftoolkit%2Fcontent%2Fwidgets&file=tabbox.xml&rev1=1.16&rev2=1.18&whitespace_mode=ignore&diff_mode=context

Comment 77

[Boris Zbarsky [:bzbarsky]]

> this.mCurrentBrowser.focusedElement.blur();

This throws if the focusedElement is not HTML or XUL (say an <svg:a> or an XLink), which pretty completely horks the UI...  See bug 323805 and 323806

Comment 78

[Aaron Leventhal]

> this.mCurrentBrowser.focusedElement.blur();
This is also causing bug 382007.