Closed Bug 175893 Opened 17 years ago Closed 15 years ago

Make XUL <tab>'s focusable

Categories

(Firefox :: Keyboard Navigation, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

(Blocks 1 open bug)

Details

(Keywords: access, helpwanted)

Attachments

(3 files, 15 obsolete files)

7.70 KB, patch
bryner
: superreview+
Details | Diff | Splinter Review
2.82 KB, patch
Details | Diff | Splinter Review
5.80 KB, patch
Details | Diff | Splinter Review
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
Tab labels are usually focusable in Windows XP.

What about Mac and Unix?
Blocks: focusnav
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.3alpha
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?
On Windows, if a tab has focus you can use the left/right arrows to navigate them.
Dean, that works with this patch, because we used to have this behavior in Classic.
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?
Dean, that's bug 166001 -- which it turns out is not fixed by this.

Working on that now.
Seeking r=, sr=
Attachment #103667 - Attachment is obsolete: true
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.)
how would this fix affect embed apps, especially those which don't use xul? just
wondering.
This won't affect embedded apps at all, because the changes are in Mozilla's
chrome layer.
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?
Seeking r=/sr=
Attachment #103705 - Attachment is obsolete: true
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.
Attachment #103901 - Flags: review+
Attachment #103901 - Attachment is obsolete: true
Comment on attachment 103901 [details] [diff] [review]
Fixes problems reported by Akkana, wrt to tabbed browser tabs

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

r=sgehani
Attachment #104198 - Flags: review+
Just to confirm what Akkana said: tab labels do get focused on click in gtk, and
unix does use the windows tabbox.css.
Blocks: 130644
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.
Sorry, I should have said "On Win32, mousing down on a tab doesn't focus it."
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.
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.
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.
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 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
Attachment #104601 - Flags: review+
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 :-)
Attachment #104601 - Flags: needs-work+
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
Attachment #104601 - Attachment is obsolete: true
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?
Attachment #104817 - Flags: needs-work+
Attached patch bindings changes (obsolete) — Splinter Review
This is what I was thinking of when I suggested merging the functions.
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.
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=
Attachment #104817 - Attachment is obsolete: true
Attachment #104867 - Attachment is obsolete: true
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); ?
ccing our team
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).
Blocks: 185772
Blocks: 194819
clearing milestone since 1.3a has passed.
Keywords: nsbeta1
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: mozilla1.3alpha → ---
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
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
Attachment #104887 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #104887 - Attachment is obsolete: true
Attachment #104887 - Flags: review?(neil.parkwaycc.co.uk)
Keywords: access
Attached patch Patch updated to trunk (obsolete) — Splinter Review
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.
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.
On linux/gtk2, it seems to work: http://nexgenmedia.net/Screenshot-18.png

Sometimes I have to click twice to get the outline though.
(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.
that seems to be the case.  I rebuild using gtk2/xft and it still works fine.
I can sometimes get 2 tabs with an outline, trying to debug how that happens.
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.
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.
Attachment #151148 - Attachment is obsolete: true
Attachment #151148 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #151148 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #151547 - Flags: review?
(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.
Attachment #151547 - Flags: review? → review?(neil.parkwaycc.co.uk)
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.
(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 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.
Attachment #151547 - Attachment is obsolete: true
Attachment #152134 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #151547 - Flags: review?(neil.parkwaycc.co.uk)
(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 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.
Attached patch Addressing more comments (obsolete) — Splinter Review
* 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?
Attachment #152134 - Attachment is obsolete: true
Attachment #152134 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #152313 - Flags: review?(neil.parkwaycc.co.uk)
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!)
Attached patch Fix selectNewTab method (obsolete) — Splinter Review
I don't see any problems with this patch and remembering focus in web pages.
Attachment #152313 - Attachment is obsolete: true
Attachment #152313 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #152373 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #152373 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #152373 - Attachment is obsolete: true
Attachment #152579 - Attachment is obsolete: true
Attachment #152581 - Flags: review?(neil.parkwaycc.co.uk)
I think that tabbrowser focus code is getting unwieldy... perhaps a variable for
what you wanted focused would help?
Attachment #152581 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #152581 - Attachment is obsolete: true
Attachment #152657 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #152657 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #152657 - Flags: superreview?(bryner)
Comment on attachment 152657 [details] [diff] [review]
Use variable whatToFocus in tabbrowser.xml

add the appropriate changes under toolkit/ and sr=me
Attachment #152657 - Flags: superreview?(bryner) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Oops. Moving the focus into the timeout means that the suppressFocusScroll never
takes effect :-[
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #153697 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #153697 - Flags: review?(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)

sr=me on the xpfe patch if you don't move the setTimeout comment away from the
setTimeout call...
Attachment #153697 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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.
Attachment #153697 - Flags: review?(mconnor) → review+
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
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Keywords: aviary-landing
Relanding relevant parts of patch following aviary branch landing
Keywords: aviary-landing
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Component: Keyboard: Navigation → Keyboard Navigation
Product: Core → Firefox
Attachment #167675 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #167675 - Flags: review?(bugzilla)
Keywords: aviary-landing
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.
Attachment #167675 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #167675 - Flags: review?(bugzilla)
Relanding relevant parts of first patch following landing of aviary branch -
previously reviewed
Status: REOPENED → RESOLVED
Closed: 16 years ago15 years ago
Keywords: aviary-landing
Resolution: --- → FIXED
Relanding missing line from tabbrowser.xml following landing of aviary branch -
previously reviewed
(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
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.
> 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
> this.mCurrentBrowser.focusedElement.blur();
This is also causing bug 382007.
You need to log in before you can comment on or make changes to this bug.