Closed Bug 415704 Opened 16 years ago Closed 16 years ago

Selecting "More Information" from the Larry UI does not launch Page Info

Categories

(Firefox :: Security, defect, P2)

Product:
Firefox
Component:
Security
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3

People

(Reporter: marcia, Assigned: enndeakin)

References

Details

Attachments

(2 files, 2 obsolete files)

What triggers this bug (_not a fix_)
1.61 KB, patch
Details | Diff | Splinter Review
more documentation
9.72 KB, patch
smaug
: review+
neil
: superreview+
Details | Diff | Splinter Review 
Seen using : Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008020404 Minefield/3.0b3pre.

STR:
1. Visit a site and click on the Larry UI.
2. Click on the "More Information" link.

Expected: It opens the Page Info UI.
Actual: Nothing happens.

I see this on the nightly Leopard build as well as a debug Tiger build. I do not see it on a nightly Tiger build. Works fine on Win XP as well.
Build ID for the debug build is Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020412 Firefox/3.0b3pre. Bug 414872 looks to be related, but the behavior right now across platform is not consistent for clicking "More Information."
Flags: blocking-firefox3?
Event bug, perhaps?  Leopard but not Tiger has my spidey-sense tingling.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Target Milestone: --- → Firefox 3 beta4
Yeah, this is sort of wonky, and I can't reproduce it here on leopard at the moment, BUT I have the patch from bug 414872 applied, which changes the label into a button, and hence the handler from an onclick with some event filtering (if (event.button == 0) {...}) to an oncommand without any.  My inclination is to land that bug and see if this one is still an issue.
> STR:
> 1. Visit a site and click on the Larry UI.
> 2. Click on the "More Information" link.

Sorry for my ignorance ... but how do you "click on the Larry UI"?

(In reply to comment #4)
> > STR:
> > 1. Visit a site and click on the Larry UI.
> > 2. Click on the "More Information" link.
> 
> Sorry for my ignorance ... but how do you "click on the Larry UI"?

"Larry" is the nickname in general for the site identity UI.  You open it by clicking the favicon button beside the url bar.
Thanks!  (I assume the favicon button is to the left of the URL bar.)

I just tried this on Leopard with yesterday's nightly, and had no
trouble.

My own spidey sense tells me this has nothing to do with events, and
(maybe) a lot to do with whatever your were doing just before you
tried the Larry UI, or (maybe) is a profile problem.

So, if you've ever seen this, try quitting the browser and then
testing first-thing for this bug.  And try testing with a clean
profile.

Using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008020504 Minefield/3.0b4pre and a fresh profile, the first time I click on a site I get the Page Info dialog. Then I open up a few tabs such as paypal.com and some other bugzilla bugs and I am not able to launch the dialog. When it works I see the hand icon, when it doesn't all I see is a cursor.
Marcia, I can't (exactly) reproduce what you report in comment #7
... but I _did_ just see the problem.

> When it works I see the hand icon, when it doesn't all I see is a
> cursor.

I never saw the hand icon (either before clicking on the favicon to
the left of the URL bar, or before clicking on the "More Information
..." link in the Larry UI.

I opened a bunch of URLs in different tabs (including paypal.com and
some bmo pages), and didn't see the problem with any of them.

Then I let the page sit (untouched) for a minute or two ... and now
the "More Information ..." link doesn't work in any of the tabs.

I tested with today's Minefield nightly on Leopard.  Now I'll go test
on Tiger.

Wierd.

> Now I'll go test on Tiger.

I did exactly the same things (on Tiger) that I did on Leopard, using
the same nightly (today's nightly), and didn't see the problem.

So this really does seem to be Leopard-specific.

Someone who knows the Larry UI:

Please tell me where in the source code the "More Information..." link
is implemented.  I'd like to break at that point (in gdb), then step
through the code.

More information is a XUL element in browser.xul.  As of bug 414872 landing, it is a button with an oncommand= handler specified.  The handler is now just a 2 line function (handleMoreInfoClick), round about line 5800 in browser.js

http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#5805

As mentioned above, the changes in 414872 might fix the behaviour here, since the text with click handler is now a button with command handler.
Thanks!

I'll test with and without the patch for bug 414872.

Jonathan, you may be right that your patch for bug 414872 fixes this
problem.

That patch was landed last night, and (as best I can tell) is in
today's Minefield nightly.  I just performed the same steps (on
Leopard) that I describe in comment #8, using today's nightly.  I
didn't see the problem (I didn't have any trouble with the "More
Information..." links).

Let's wait for Marcia's tests.  But with luck I think we can close
this out.

Bad news:

I just reproduced the problem with today's nightly (still following,
more-or-less, my steps from comment #8).

Which shows that this bug is _definitely_ wonky and that I don't yet
really understand what triggers it.  I'll keep trying.

Steven - I still can't reproduce this, but I wonder if maybe it has nothing to do with Larry.  Couple questions:

 - Does it happen more consistently with either https or http urls?  That is, is it related to whether the connection has a cert?

 - If you can get it to happen on https sites, try double clicking the lock icon in the status bar - that should open the same dialog through an almost-identical code path.  If that also breaks in the same way, we know it's unlikely to be related to the actual events being fired by the More Information button

 - When it's in this broken state, does Tools->Page Info still work?

 - I think you would have mentioned it if it were the case, but I assume nothing's being logged to the error console? 
Here is what I see using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008020708 Minefield/3.0b4pre.

Using Leopard when I click the "More Information" button nothing happens on either http or https sites. This happens whether or not I do it right away or wait. No errors in the console on Leopard.

Using the same build on Tiger, clicking the "More Information" dialog launches Page Info perfectly.

(In reply to comment #15)

OK, I've got it happening again.  I still haven't noticed any pattern
... and no (unfortunately) the problem isn't triggered by the screen
saver kicking in :-)

But I'm in a position to answer your questions:

> - Does it happen more consistently with either https or http urls?
> That is, is it related to whether the connection has a cert?

No.  So my answer is the same as Marcia's.

> - If you can get it to happen on https sites, try double clicking
> the lock icon in the status bar

This still works fine.

> - When it's in this broken state, does Tools->Page Info still work?

Yes (on both http and https pages).  Command-i also works fine.

> - I think you would have mentioned it if it were the case, but I
> assume nothing's being logged to the error console?

I don't see anything relevant (timewise) in any of the error console
(Tools : Error Console), console log or system log.

The problem doesn't go away if I switch to another app and then back
again.  It keeps happening in new windows (in all tabs, including
blank ones).

OK, I think I have the key to this, and I've now reproduced it on
Tiger (as well as Leopard):

The problem seems to be triggered by the bar (I don't know what else
to call it) that shows up when (for example) you log in to a site for
the first time (in this case you're asked whether or not you want the
browser to remember your account and password).  I believe this is the
same "bar" that shows up when a popup window has been blocked.

Here's a 100% effective way to reproduce this bug:

1) Give yourself a clean profile.

2) Start the browser and go to https://bugzilla.mozilla.org/.

3) Log in (it makes no difference whether or not the login succeeds)
   -- the "bar" I'm talking about will appear.  It also makes no
   difference whether you close the "bar" or leave it open.

   From this point forward (in the current browser session), the "More
   Information..." link will always fail (nothing will happen when you
   select it).

To be absolutely sure, I'd also need to test with a site that has
popup(s) that Minefield can block ... but I can't think of one,
offhand.

This STR doesn't work on Windows (with a recent nightly).

On OS X, the appearance of the "bar" must throw off the coordinate
system of the Larry UI.  It doesn't appear to effect context menus,
and other popup windows, though.

If I'm right, this is pretty clearly a Cocoa widgets bug.  But, before
changing the categorization, I'd like somebody to test with the "bar"
that appears when the browser blocks one or more popup windows.

Steven: Using the latest minefield nightly on Leopard, here is what I see with a popup blocker site.

STR:
1. With a fresh profile launch FF and go to orbitz.com
2. Allow the popup notification bar to launch.
3. Click around on the Larry UI on various open tabs -> Page info launches.
4. Go to bugzilla and login. Leave the notification bar up -> Page Info fails on that tab, as well as all other open tabs, even ones that previously worked.
Interesting.  And I'm able to reproduce what you report, Marcia.

So the "remember password" bar is different in some crucial way from
the "popup windows blocked" bar.  Can anyone think what that might be?

This is sort of crazy - gavin and I are both on Leopard, and these STR don't produce a problem for either of us on trunk.

At least we have a reliable path for *some* people to reproduce the bug, but whatever it is, it seems pretty specific to... something.  Really helpful, I know.  :\
I've got very plain-vanilla setups on both machines where I can
reproduce this bug (an early MacBook Pro (Intel Core Duo) running OS X
10.4.11, and a brand new Mac Pro running OS X 10.5.1) -- basically the
standard OS install, plus XCode, and nothing else.

(I also don't use any extensions on either machine -- but a clean
profile should get rid of your extensions.)

Both of my Macs have ATI Radeon graphics cards (my MacBook Pro has a
Radeon X1600, my Mac Pro a Radeon HD 2600).  Could _that_ be making a
difference? :-)

I also wonder about pointing devices:

My MacBook Pro is pretty standard -- I'm using the built-in scratch pad.

But the setup for my Mac Pro is quite non-standard -- I'm using a
Microsoft 3-button mouse (and an old Dell keyboard) via an Iogear
4-port KVM switch (model GCS614A).

> gavin and I are both on Leopard, and these STR don't produce a
> problem for either of us on trunk.

If "on trunk" doesn't mean "using a nightly", please test with today's
nightly.

This problem still happens with today's Minefield nightly ... but it's
even stranger than I first realized.

I've boiled down my STR from comment #18, and found that the "save
password bar" _isn't_ implicated:

1) Give yourself a clean profile.

2) Start the browser and go to https://bugzilla.mozilla.org/.

3) Type some text in the "Login" field.

4) Type some text in the "Password" field.

5) Take the focus out of the "Password" field by clicking somewhere
   else on the browser page.

   From this point the Larry UI's "More Information..." link stops
   working.

I got these results with both today's and yesterday's Minefield
nightlies.  I haven't yet tested further.

I've found a regression range for the STR from comment #26:  The
2008-01-23-04-trunk nightly doesn't have the problem, but the
2008-01-24-04-trunk nightly does have it.

I can't figure out which checkin in this range might have
triggered/caused this problem.  But none of the Cocoa-widgets-specific
changes (there are two) could possibly have caused it (I think).  So
if this is a Cocoa widgets bug, it isn't one in any straightforward
way.  In other words, the only way this could be a Cocoa widgets bug
is if some other change uncovered a pre-existing Cocoa widgets bug.

(In reply to comment #27)
> I've found a regression range for the STR from comment #26:  The
> 2008-01-23-04-trunk nightly doesn't have the problem, but the
> 2008-01-24-04-trunk nightly does have it.
> 
> I can't figure out which checkin in this range might have
> triggered/caused this problem.  But none of the Cocoa-widgets-specific
> changes (there are two) could possibly have caused it (I think).  So
> if this is a Cocoa widgets bug, it isn't one in any straightforward
> way.  In other words, the only way this could be a Cocoa widgets bug
> is if some other change uncovered a pre-existing Cocoa widgets bug.

Hmm - bug 407359 is in that window, I think, and impacts all kinds of stuff to do with popup behaviour.  Aaron - any chance you're involved here?

Smichaud - you have the time and wherewithal to try a build with that patch backed out, as an experiment?  Dunno how much pain that is for this patch, since a lot of that code may have changed since, but I can't help, since I can't make this happen.  :(  
> Smichaud - you have the time and wherewithal to try a build with
> that patch backed out, as an experiment?

I could, in principle.

But that's a big patch, and I'd like to avoid spending the time.

Maybe Aaron made tryserver builds with the patch for bug 407359 before
that patch was landed.  If so I could test one -- and if I see the
problem with it, that patch would (in some way) be implicated.

Alternatively, it'd be nice to hear from Aaron how to disable those
parts of his patch (for bug 407359) that he thinks are most likely to
have triggered this problem -- this'd presumably be a lot less messy
than trying to back out the whole patch.

And it'd be easier on someone who's totally unfamiliar with the code
that patch touches :-)

Did you check to see if that's the day it regressed?

Between then and now it's been changed from a link to a button. 

I've found an easy way to test the patch for bug 407359 ... and I've
found that it isn't implicated in this bug.

What I did was to pull two source trees from CVS -- one dated just
before the patch for bug 407359 landed, and one dated just afterwards.
I built and tested both trees, and on both of them the STR from
comment #26 didn't "work".

I don't know where that leaves us :-)

Just went through a very interesting conversation with Melissa, and we might have cracked it.  She pinged to tell me that More Info was broken on Beta 3, and I was sad. Then I asked if she had the Proto test-theme still installed.  She did (v.11).  Uninstalling the theme fixed the more info link for her.

Any chance that smichaud and marcia also have the proto theme kicking around?  My macbook didn't, tomorrow I'll reinstall it and see if the breakage shows up.

I'm provisionally cc'ng kevin gerich too, just to make him aware.
I see this problem using the 2008-01-24 nightly (and later ones)
without the Proto theme.  I just retested, to be sure.

(The Proto theme didn't become the default until later.  And remember
that my STRs call for a fresh profile, which would have gotten rid of
any copy of the Proto theme just lying around.)

Still though, the regression timetable may be more complex than I
first thought -- the problem may have come and gone more than once.

(In reply to comment #35)
> Just went through a very interesting conversation with Melissa, and we might
> have cracked it.  She pinged to tell me that More Info was broken on Beta 3,
> and I was sad. Then I asked if she had the Proto test-theme still installed. 
> She did (v.11).  Uninstalling the theme fixed the more info link for her.

Only until it broke again.  This was the observed behavior because it initially works after a restarted of Firefox.  It only quits after you've been doing a few things (check the STR above).  I initially implicated the test version of Proto for the same reason (problem went away when I uninstalled it), but after browsing a little, the problem came back.
I have tested this issue with some of my existing profiles as well as with fresh profiles and have seen the bug using both. Yesterday I created a fresh trunk profile and did not see it initially, but I do see it today when using the nightly with the same profile.
Marcia, does my STR from comment #26 "work" for you?  For me it works
100% of the time, on both Tiger and Leopard.

And if so, do you see the same regression range that I did in comment
#27?

More data:

For what it's worth, I've just tested with a CVS tree dated 2008-01-24
00:00 PDT.  With it I saw the problem (my STR from comment #26 "works"
on it, on both Tiger and Leopard).

So whatever patch triggered this bug must have landed between
2008-01-23 16:00 PDT and 2008-01-24 00:00 PDT.

Anyone have any idea which one it could have been?

I'll keep doing brute force tests over the next few days.

> PDT

Groan.  I just realized I should have been using PST.

So my tests of the patch for bug 407359 were thrown off by an hour (in
both cases, the trees I pulled were an hour older than I thought they
were).  And I need to redo them.

Sorry for the confusion.

(Likewise, the tree I thought was dated 2008-01-24 00:00 was actually
dated 2008-01-23 23:00.)

Alright, my new tests _do_ implicate the patch for bug 407359.

I pulled two copies of the CVS tree, one dated 2008-01-23 15:40 PST
(just before the patch landed), and the second dated 2008-01-23 16:00
PST (just after the patch landed, and also just after another small
patch landed to fix a test for popups).

Then I built and tested both.  The STR from comment #26 doesn't "work"
on the first build, but does "work" on the second build.

Once again, Aaron, could you tell me how to disable the parts of your
patch that you think might have triggered this problem?

(Notice that I don't say "caused".  It's quite likely that something
in your patch uncovered another bug elsewhere, perhaps in Cocoa
widgets.)

Once more I've been banging my head against this, and I have a
revision to my STR from comment #26 (I've boiled it down some more):

1) Give yourself a clean profile.

2) Start the browser.

3) Press the tab key.

   From this point the Larry UI's "More Information..." link stops
   working.

Yes, it's that simple!

And I'm not at all sure step 1 is necessary.

With luck Jonathan and Gavin will now be able to reproduce this bug,
too.

My STR "works" on both Leopard and Tiger.

Sigh.

Leopard (with 10.5.2 update), current trunk, non-debug, clean profile, start browser, press tab, button still works in all cases.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008022010 Minefield/3.0b4pre

Are we exposing some underlying mac bug or something?  Maybe something catastrophically stupid like multiple displays, or external keyboard, or ...? It sounds like we can be code-identical and still see the differing behaviour, so it really feels like it's tied to the operating environment.  Not that that means we shouldn't fix it.

I'll copy Neil Deakin and Josh in, in case they are aware of anything mac-platform level that could be causing this.

By cc to Neil/Josh: smichaud and Marcia can reproduce (using STR in comment 43) a bug where the More Information button in the Larry popup doesn't do anything, or log any error.  Gavin and I can't make this happen.  Do you know of any sometimes-broken widget behaviour like this? 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008022004 Minefield/3.0b4pre

I can still reproduce this at will as well.  MacBook Pro, 2.16 GHz Core Duo (not a Core 2 Duo), 2 GB RAM.  Mac OS X 10.5.2.  I *do* have an external keyboard, display, and mouse connected, if it matters.

Leaving this bug open (and a few other things so I get the Save Tabs dialog) I quit Firefox.  Run it again, the page showing this bug opens.  Click on the favicon, and Larry shows up.  Click More Information and the Page Info window opens.  Close the window.  Click the favicon again, click More Information again, the Page Info window opens.  Close the window.  Click in the search box on the right.  Click the favicon, click More Information, Page Info opens.  Close the window.  Click in the search box, hit the tab key.  Click on the favicon, click More Information.  Larry goes away, Page Info window never opens.

Quit Firefox, saving tabs again.  Run it.  Click on the bugzilla favicon, click More Information, Page Info opens.  Close it.  Click in the keywords field of the bug.  Click on the favicon, click More Information, Page Info opens.  Close it.  Click in the keywords field.  Press the tab key (highlight moves to whiteboard).  Click on the favicon, click on More Info.  Larry goes away, Page info does not show up.

This definitely feels attached to the tab key, and it doesn't seem specific to having chrome fields in focus at the time either. 
I'm slowly working through the patch for bug 407539 by brute force --
disabling parts of it and retesting.

So far I've found that rolling back the patch in
toolkit/content/widgets (in the files autocomplete.xml, popup.xml and
textbox.xml) has no effect -- the bug still happens (and my STRs from
comment #26 and comment #43 still "work").

> I'm slowly working through the patch for bug 407539 by brute force --
> disabling parts of it and retesting.

bug 407359

Don't know if it helps any with troubleshooting it, but when the More Information button works, it highlights blue when you click it and the action (closing Larry and opening the Page Info window) doesn't happen until you let go of the mouse (normal button behavior).  When the More Information button doesn't work, Larry goes away the moment you click, and the button never highlights (and it doesn't wait for you to let go of the mouse).
I can't reproduce this either and have tried all kinds of variations of the steps listed in comments here.

Dave, is there a blue ring around the More Info button when the identity panel is first opened?

Bug 407359 does seem a likely cause though as it affected focus behaviour in popup, with additional special casing for the identity popup.

Comment 48 suggests a widget level bug as the mouse event doesn't seem to be targeted at the identity popup, but is instead causing the popup to rollup. Stephen, what is the rollup widget set to when it works versus fails when the mouse event is receieved?

I suspect that pressing the tab button just has the side effect of focusing something in the document, rather than in chrome, thus making a different widget the target of events.

(In reply to comment #49)
> Dave, is there a blue ring around the More Info button when the identity panel
> is first opened?

Yes, there is.  And there's still a blue ring around it even when it doesn't work.

> Comment 48 suggests a widget level bug as the mouse event doesn't seem to be
> targeted at the identity popup, but is instead causing the popup to rollup.

As a side note though, clicking elsewhere in the identity popup doesn't dismiss it, only clicking on the button does.

> I suspect that pressing the tab button just has the side effect of focusing
> something in the document, rather than in chrome, thus making a different
> widget the target of events.

It doesn't seem to matter what has focus.  I can put the focus back in the URL bad before opening the identity popup and it still won't work, once the failure gets triggered.
(In reply to comment #49)

> Stephen, what is the rollup widget set to when it works versus fails
> when the mouse event is receieved?

I'll test as soon as I get the chance (later today or tomorrow).

(In reply to comment #48)

> When the More Information button doesn't work, Larry goes away the
> moment you click, and the button never highlights (and it doesn't
> wait for you to let go of the mouse).

This rings a bell.  I'll try to get my tests to explain it.

(Following up comment #51)

I've done some debugging and found out some interesting things ... but
nothing that really gets to the heart of this bug.

When the Larry UI works "normally", the following happens after
[ChildView mouseUp:] sends Gecko an NS_MOUSE_BUTTON_UP event and
returns (that is, this happens after Gecko has finished processing the
NS_MOUSE_BUTTON_UP event):

nsCocoaWindow::CaptureRollupEvents() is called three times on the
popup window's widget (an nsCocoaWindow object, the same one all three
times), with the following parameters:

1) aListener (nsXULPopupManager), aDoCapture (false),
              aConsumeRollupEvent (false)

2) aListener (nsXULPopupManager), aDoCapture (true),
              aConsumeRollupEvent (true)

3) aListener (nsXULPopupManager), aDoCapture (false),
              aConsumeRollupEvent (false)

When (after you've pressed the Tab key) the Larry UI no longer works
"normally", the following happens while Gecko is processing an
NS_MOUSE_BUTTON_DOWN event (called from [ChildView mouseDown:]):

nsCocoaWindow::CaptureRollupEvents() is called once on the popup
window's widget (again the same one as above, which gets re-used),
with the following parameters:

1) aListener (nsXULPopupManager), aDoCapture (false),
              aConsumeRollupEvent (false)

I'm quite sure this won't be fixed in time for beta4.

But now that my work for beta4 is done, I hope to have more time to
spend on this.
Assignee: nobody → smichaud
Here's a one-line change (which reverses a small part of the patch for
bug 407539) that stops this bug from happening in my tests.

No, this isn't a fix.  And no, I don't (yet) understand its
significance.

Just another clue.

Here's a tryserver build made with this "patch".  Those of you who can
reproduce this bug, please test it to see if it also makes the problem
go away for you.

https://build.mozilla.org/tryserver-builds/2008-02-27_11:45-smichaud@pobox.com-bugzilla415704-trigger/smichaud@pobox.com-bugzilla415704-trigger-firefox-try-mac.dmg
> Here's a one-line change (which reverses a small part of the patch
> for bug 407539) that stops this bug from happening in my tests.

bug 407359

Sorry.

Steven: I took your tryserver build for a test drive. Almost every time I was able to get the "More information" to launch. The **only** time it did not work for me in the tryserver build was when I opened the "Getting Started" bookmark in a new window, and clicked on it quickly, there was a delay and then I crashed the build. I did not get a breakpad report but I have the apple report and I can attach it to the bug if it useful.
This one line change has made me think of a possibility. I can reproduce this now if I change the Full Keyboard Access preference to 'textbox and lists only'. The bug does not occur if it is set to 'all controls'.
(In reply to comment #56)

> I did not get a breakpad report but I have the apple report and I
> can attach it to the bug if it useful.

Go ahead.  I think it's probably not relevant ... but with this bug
you never know :-)

And in any case it might contain enough information to make it
worthwhile opening another bug.

(In reply to comment #57)

Interesting!

I assume Full Keyboard Access is a an OS pref (though I don't know
where to find it).  When I do manage to find it I'll try out your
change.

(In reply to comment #57)
> This one line change has made me think of a possibility. I can reproduce this
> now if I change the Full Keyboard Access preference to 'textbox and lists
> only'. The bug does not occur if it is set to 'all controls'.

With this, I can reproduce the bug.  Of course, now that smichaud has narrowed down the culprit, I have nothing to offer by way of fix - but hopefully Neil's epiphany means he has thoughts on solutions, or at least bugs we can dup this to.  :)

(In reply to comment #58)
> I assume Full Keyboard Access is a an OS pref (though I don't know
> where to find it).  When I do manage to find it I'll try out your
> change.

System Preferences -> Keyboard and Mouse -> Keyboard Shortcuts (bottom of panel)


Comment on attachment 306084 [details] [diff] [review]
What triggers this bug (_not a fix_)

What if we change this to set focus after setTimeout(0) ? Does that fix it? Just a guess.
Comment on attachment 306084 [details] [diff] [review]
What triggers this bug (_not a fix_)

What if we change this to set focus after setTimeout(0) ? Does that fix it? Just a guess.
(In reply to comment #59)

> System Preferences -> Keyboard and Mouse -> Keyboard Shortcuts
> (bottom of panel)

Thanks, Jonathan!

And yes, changing this from "Text boxes and lists only" (which seems
to be the OS default) to "All controls" also "fixes" the problem for
me on both Tiger (10.4.11) and Leopard (10.5.2).

Grepping on "Full Keyboard Access" in the tree turns up a bunch of OS
X-specific (and Cocoa-specific) code.  The problem(s) must be
somewhere in that code (which explains why this problem is Mac-only).

At this point I have no idea what those problems might be ... but now
it shouldn't be too hard to find out.

So thanks, Neil, for thinking of Full Keyboard Access!
Attached patch adding a tabindex seems to work (obsolete) — — Splinter Review 
Assignee: smichaud → enndeakin
Status: NEW → ASSIGNED

    
    

    
  
(In reply to comment #60)

> What if we change this to set focus after setTimeout(0) ? Does that
> fix it?  Just a guess.

If all else fails I'll try that.

But first I'll play out the "Full Keyboard Access" clue.

(In reply to comment #63)

> adding a tabindex seems to work

Sounds like a good fallback.

But I think I should first look through the OS X-specific "Full
Keyboard Access" code for more fundamental problems.


    
    

    
  
> (In reply to comment #63)
>
>> adding a tabindex seems to work
>
> Sounds like a good fallback.

I'm going to test this to see if it's really a comprehensive fix.


    
    

    
  
(In reply to comment #63)
> Created an attachment (id=306106) [details]
> adding a tabindex seems to work

This fixes the problem for me (now that I can reproduce it in the first place).
Target Milestone: Firefox 3 beta4 → Firefox 3

    
    

    
  
> This fixes the problem for me (now that I can reproduce it in the
> first place).

For me, too (as best I can tell).  I tested on 10.4.11 and 10.5.2.

I guess the fix just hard-codes the "identity-popup-more-info-button"
button's tab index in its parent.

But I'll still keep looking through the "Full Keyboard Access" code
for more general problems.

Target Milestone: Firefox 3 → Firefox 3 beta4
Target Milestone: Firefox 3 beta4 → Firefox 3

    
    

    
  
Another bizarre clue:

Pressing the Tab key appears to change sTabFocusModel from '7'
(eTabFocus_any) to '1' (undefined and supposedly unused).


    
    

    
  
> Pressing the Tab key appears to change sTabFocusModel from '7'
> (eTabFocus_any) to '1' (undefined and supposedly unused).

But only if "Full keyboard access" is set to "All controls".


    
    

    
  
This (changing the value of sTabFocusModel when the Tab key is
pressed) appears to happen when
nsLookAndFeel::GetMetric(nsILookAndFeel::eMetric_TabFocusModel,
nsIContent::sTabFocusModel) is called from
nsEventStateManager::ShiftFocus().


    
    

    
  
On all platforms besides OS X, nsIContent::sTabFocusModel gets
initialized to '7' (eTabFocus_any) and stays that way.  (Though
there's a browser preference, "accessibility.tabfocus" that (I
believe) can override this.)

On OS X nsIContent::sTabFocusModel also starts out initialized to
eTabFocus_any.  But the first time nsLookAndFeel::GetMetric() (in
nsLookAndFeel.mm) is called with the appropriate parameters (currently
only from nsEventStateManager::ShiftFocus()), it can get changed to
'1' (if "Full keyboard access" isn't set to "All controls").

OS X is the only platform where nsIContent::sTabFocusModelAppliesToXUL
(also overridable, by the "accessibility.tabfocus_applies_to_xul" user
setting) is set to "yes" -- which means that only on OS X does the
setting of sTabFocusModel govern what happens both in the browser page
and outside of it (in XUL).


    
    

    
  
I can't think how to change all this, or even decide whether or not it
should be changed.  But it's currently producing very strange results.


    
    

    
  
Oops, I skipped a step in the story:

Changing nsIContent::sTabFocusModel from eTabFocus_any ('7') to '1'
changes the behavior of nsXULElement::IsFocusable() when it's called
on the "More information" button in the Larry UI: xWhen sTabFocusModel
is eTabFocus_any it always returns 'true' (implying that the button is
focusable).  But when sTabFocusModel is '1' it always returns 'false'
(implying that the button isn't focusable).

Neil's fix (attachment 306106 [details] [diff] [review]) does change this behavior, but only by
hacking a "tabindex" attribute into the "More information" button.


    
    

    
  
> when it's called on the "More information" button in the Larry UI

When it's called on that button after you click on it.


    
    

    
  
Yes, the patch I provided in intended as a workaround.

Although I haven't been able to create a minimal testcase, I suspect what is happening is because nothing in the popup 'IsFocusable', either the parent window is being focused or nothing is, confusing some code into thinking that the panel should be closed. 

It seems that fundamentally though that the IsFocuable method is the wrong place to check the sTabFocusModel. The 'Full Keyboard Access' setting shouldn't affect whether the element can be focused or not, it should only affect whether pressing the Tab key skips over the element.

Mano wrote this code originally it seems; maybe he might have some insight.


    
    

    
  
> The 'Full Keyboard Access' setting shouldn't affect whether the
> element can be focused or not, it should only affect whether
> pressing the Tab key skips over the element.

Yes, I'd agree with that.

In this case the button's (it's XUL element's) isFocusable() method is
returning false even when it's being focused with a mouse click.


    
    

    
  
This issue is caused because in nsEventStateManager::PostHandleEvent is looking for something to focus when the mousedown event occurs. Since the button that was clicked returned false from IsFocusable, it proceeds to clear the current focus, which causes the blur handler on the More Info button to be called:

onblur="getIdentityHandler().hideIdentityPopup();"

which, of course, closes the popup without doing activating the button action.

It appears to work fine if this blur handler is removed.

The actual bug though, is that IsFocusable should only check sTabFocusModel if the Tab key was pressed for navigation. This would fix the related bug as sTabFocusModel in only initialized when Tab is pressed and an incorrect value is used beforehand.



    
    

    
  
Neil, I agreed with you for a while, but now I'm having second
thoughts.

On the face of it, it makes sense to have an nsIContent object's
IsFocusable() method do whatever needs to be done to find out whether
or not the object is focusable.  And for any given object, this
question has a yes or no answer.

> This issue is caused because in nsEventStateManager::PostHandleEvent
> is looking for something to focus when the mousedown event
> occurs. Since the button that was clicked returned false from
> IsFocusable, it proceeds to clear the current focus, which causes
> the blur handler on the More Info button to be called:
>
> onblur="getIdentityHandler().hideIdentityPopup();"

As best I can tell, this only happens (after the user has pressed Tab
at least once) if the "More information" button is already focused
when the mouse clicks on it.

If it _isn't_ already focused (as happens with my "trigger" patch,
attachment 306084 [details] [diff] [review]), the "More Information" button's
nsXULElement::IsFocusable() method still returns FALSE when the user
clicks on it, but this doesn't cause any trouble, and the button's
oncommand handler _does_ get called.

So I now think that there two underlying problems:

1) nsIContent::sTabFocusModel should be initialized to its "correct"
   value as soon as possible.  This shouldn't have to wait until the
   first time the user presses the Tab key.

2) Calling an object's .focus() method (from JS) shouldn't "work" if
   the object isn't focusable.  So the call to
   document.getElementById('identity-popup-more-info-button').focus();
   in the Larry panel's onpopupshown method shouldn't focus the "More
   information" button when nsIContent::sTabFocusModel is '1'.

   It _does_ always focus the button now (regardless of the setting of
   sTabFocusModel) -- which (you could argue) is the source of all the
   trouble.


    
    

    
  
You might also argue that all IsFocusable() methods should have a
"PRBool aWithMouse" parameter.  Currently nsIFrame (and its
subclasses') IsFocusable() methods do have such a parameter, but
nsIContent (and its subclasses') IsFocusable() methods don't.


    
    

    
  
> If it _isn't_ already focused (as happens with my "trigger" patch,
> attachment 306084 [details] [diff] [review]), the "More Information" button's
> nsXULElement::IsFocusable() method still returns FALSE when the user
> clicks on it, but this doesn't cause any trouble, and the button's
> oncommand handler _does_ get called.

Yes, because the button doesn't need to be blurred, so the blur event never fires to closes the popup.

> Calling an object's .focus() method (from JS) shouldn't "work" if the
> object isn't focusable.  So the call to
> document.getElementById('identity-popup-more-info-button').focus();
> in the Larry panel's onpopupshown method shouldn't focus the "More
> information" button when nsIContent::sTabFocusModel is '1'.

No that's not right. The button should be focusable, it just shouldn't be in the tab order. I think the issue may be in nsXULElement::IsFocusable and am looking into a patch.


    
    

    
  
>> Calling an object's .focus() method (from JS) shouldn't "work" if
>> the object isn't focusable.  So the call to
>> document.getElementById('identity-popup-more-info-button').focus();
>> in the Larry panel's onpopupshown method shouldn't focus the "More
>> information" button when nsIContent::sTabFocusModel is '1'.
>
> No that's not right. The button should be focusable, it just
> shouldn't be in the tab order. I think the issue may be in
> nsXULElement::IsFocusable and am looking into a patch.

Well, you do know this code much better than I do.

But I also have an outsider's perspective ... which sometimes results
in a cleaner and more thorough analysis :-)

In any case, I'm happy to defer to your judgment.  And I'm glad that
this bug will probably be fixed soon.


    
    

    
  

      
      
      
      

        Attached patch
          
          
        adjust IsFocusable method (obsolete)
        — 
          — Splinter Review
      

    


  
This patch makes it so that an element can be focused if:

- it is not a disabled control
- if it has a -moz-user-focus of none/ignore, and has a tabindex >= 0
- if it has some other value for -moz-user-focus

Non-controls do not have a disabled state or a tabindex, so only the -moz-user-focus is checked.

The tabindex determines the tab order. The 'Full Keyboard Access' setting only affects the tab order, not the focusability.

        Attachment #306106 -
        Attachment is obsolete: true

    
  

        Attachment #306360 -
        Flags: superreview?(neil)

        Attachment #306360 -
        Flags: review?(Olli.Pettay)

    
    

    
  
(In reply to comment #82)
>Created an attachment (id=306360)
>adjust IsFocusable method
Out of interest, do you know who calls IsFocusable?

    
    

    
  
(In reply to comment #83)
> (In reply to comment #82)
> >Created an attachment (id=306360) [details]
> >adjust IsFocusable method
> Out of interest, do you know who calls IsFocusable?
> 

It's called from nsIFrame::IsFocusable (which is called when the mouse is pressed) and from the Tab key navigation code in the EventStateManager.
 

    
    

    
  
IsFocusable() is also used by mozilla/accessible.

    
    

    
  
Comment on attachment 306360 [details] [diff] [review]
adjust IsFocusable method

>+    if (xulControl && HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)) {
>+      // if either the aTabIndex argument or a specified tabindex is non-negative,
>+      // the element becomes focusable.
>+      PRInt32 tabIndex = 0;
>       xulControl->GetTabIndex(&tabIndex);
>+      shouldFocus = *aTabIndex >= 0 || tabIndex >= 0;

What if *aTabIndex >= 0 but tabIndex == -1 ?

    
    

    
  
(In reply to comment #86)
> >+      PRInt32 tabIndex = 0;
> >       xulControl->GetTabIndex(&tabIndex);
> >+      shouldFocus = *aTabIndex >= 0 || tabIndex >= 0;
> 
> What if *aTabIndex >= 0 but tabIndex == -1 ?

The element should be focusable but not part of the tab order.


    
    

    
  


  

        Attachment #306360 -
        Attachment is obsolete: true

        Attachment #307470 -
        Flags: superreview?(neil)

        Attachment #307470 -
        Flags: review?(Olli.Pettay)

        Attachment #306360 -
        Flags: superreview?(neil)

        Attachment #306360 -
        Flags: review?(Olli.Pettay)

    
    

    
  
OK, so have I understood this correctly? IsFocusable may be called in one of two ways:
1) To see if an element is focusable by the mouse, IsFocusable is passed in a null tab index pointer. A XUL Element is focusable by the mouse if it is a non-disabled control.
2) To see if an element is focusable by the keyboard, IsFocusable is passed in a non-null tab index representing the state of the -moz-user-focus computed style, however controls may override this if they are disabled or have a tabindex attribute, in which case the tab index is updated to reflect this.

What happens if you click on a toolbarbutton, which is supposedly unfocusable?

    
    

    
  
Comment on attachment 307470 [details] [diff] [review]
more documentation

Still one question. Is it really needed that non-disabled non-control without tabindex attribute is focusable?

    
    

    
  
(In reply to comment #89)
> OK, so have I understood this correctly? IsFocusable may be called in one of
> two ways:
> 1) To see if an element is focusable by the mouse, IsFocusable is passed in a
> null tab index pointer. A XUL Element is focusable by the mouse if it is a
> non-disabled control.
> 2) To see if an element is focusable by the keyboard, IsFocusable is passed in
> a non-null tab index representing the state of the -moz-user-focus computed
> style, however controls may override this if they are disabled or have a
> tabindex attribute, in which case the tab index is updated to reflect this.
> 

Yes. It seems a bit wrong to do a different check in each case though. We should really fix up this strange api post 1.9

> What happens if you click on a toolbarbutton, which is supposedly unfocusable?

Mouse events still occur but the toolbarbutton just doesn't take the focus.

> Still one question. Is it really needed that non-disabled non-control without
tabindex attribute is focusable?

Non-controls don't check the tabindex attribute.


    
    

    
  
(In reply to comment #91)
> (In reply to comment #89)
> > What happens if you click on a toolbarbutton, which is supposedly unfocusable?
> Mouse events still occur but the toolbarbutton just doesn't take the focus.
But setting style="-moz-user-focus: normal;" or an explicit .focus() call still works, right?

    
    

    
  
(In reply to comment #92)
> (In reply to comment #91)
> > (In reply to comment #89)
> > > What happens if you click on a toolbarbutton, which is supposedly unfocusable?
> > Mouse events still occur but the toolbarbutton just doesn't take the focus.
> But setting style="-moz-user-focus: normal;" or an explicit .focus() call still
> works, right?
> 

Should do. Note that I haven't changed any other behaviour other than make the sTabFocusModel setting affect only the tabindex and not whether it is focusable and make the code clearer as to what it is doing. So whatever was happening before should still be happening the same way.


    
  

        Attachment #307470 -
        Flags: superreview?(neil) → superreview+

    
    

    
  
(In reply to comment #91)
> > Still one question. Is it really needed that non-disabled non-control without
> tabindex attribute is focusable?
> 
> Non-controls don't check the tabindex attribute.

Currently there is code to check existence of tabindex attribute even if the 
element isn't a control. See the last line of ::IsFocusable



    
    

    
  
> Currently there is code to check existence of tabindex attribute even if the 
> element isn't a control. See the last line of ::IsFocusable

That case never gets checked for non-controls. The logic in the existing code:

PRInt32 tabIndex = aTabIndex? *aTabIndex : -1;
PRBool disabled = tabIndex < 0;
...
return tabIndex >= 0 || (!disabled && HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex));

If aTabIndex is null or *aTabIndex is -1, then disabled is set to true, so the attribute is not checked. If *aTabIndex is greater or equal to 0, then the first check in the if condition will be true already. It's confusing, true, which is why I reworked this method.

 

        Attachment #307470 -
        Flags: review?(Olli.Pettay) → review+

    
  
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

    
    

    
  
verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032615 Firefox/3.0b5pre

-> Verified
Status: RESOLVED → VERIFIED

    
  
Flags: in-testsuite+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: