106123 - Ctrl+Tab focuses hidden panes of tabbrowser

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Brian Ryner (not reading)]

Ctrl+[Shift]+Tab incorrectly focuses the hidden panes when more than one
tabbrowser tab is open.

Comment 1

[sairuh (rarely reading bugmail)]

oh, is this what i described on 2001-10-18 17:38 in bug 104977?

Comment 2

[sairuh (rarely reading bugmail)]

feh, you said that already...what i get for not reading *all* the new bugmail in
a folder. ;)

Comment 3

[Jesse Ruderman]

*** This bug has been marked as a duplicate of 104602 ***

Comment 4

[sairuh (rarely reading bugmail)]

reopening, since i think bryner will be working on this.

Comment 5

[sairuh (rarely reading bugmail)]

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

Comment 6

[Brian Ryner (not reading)]

-> 1.0

Comment 7

[Jesse Ruderman]

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

Comment 8

[Brian Ryner (not reading)]

Nominating for nsbeta1, we'll first need to fix 124946 though.

Comment 9

[Peter Trudelle]

nsbeta1+ per Nav triage team, adding access keyword

Comment 10

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 11

[Brian Ryner (not reading)]

Attachment: patch

This patch does the following:
 - any time we check to see if a docshell is "visible", we also check the
"type" attribute of the <browser> that we're in, and make sure it is
"content-primary"
 - factor out the logic for getting at the content for a subshell into a helper
method
 - remove some unused variable assignments

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 77998 [details] [diff] [review]
patch

This isn't quite right.  New patch coming up soon.

Comment 13

[Brian Ryner (not reading)]

Attachment: new patch, probably still needs work

Ok, this patch works, but I have a hard time believing there isn't an easier
way to do this.  What I do here is walk up the document chain; for each
document I start at the frame for the content enclosing the subshell we're
coming from, walk up until I find a frame with a view, and walk up the view
chain, stopping if I find a hidden view.  If no hidden views are found, it
continues up to the parent document.  If we hit the top without finding any
hidden views, we go ahead and allow ctrl+tab to focus the document.

Comment 14

[Aaron Leventhal]

I can't believe we need that nastiness. Glad you were the one to write it, not me :)

We better put some comments in there, that explain the stages of what's
happening. With all of the variables and getters in there, it gets cumbersome to
understand. Some good comments at the beginning of each section would help, for
example // Get frame and prescontext for docshell, // Check all views in parent
chain for visibility, etc.

Shouldn't we be leaving this method alone, and fixing
nsIBaseWindow::GetVisibility() instead? It seems like the thing should be able
to tell us whether it's visible or not, through some interface.

Comment 15

[Aaron Leventhal]

Did you check with dbaron, or hyatt on this? Although I wouldn't say "there
*has* to be a better way", I would say "we should make a better way", by adding
the necessary methods to interfaces in the right places.

I think basic things like, whether an object is visible on screen, should be
supported by the object itself.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know much about XUL. In HTML content instead of walking the view tree to
see if you an a hidden parent, you could just check the CSS "visibility" style
on your frame.

A time is coming when all subdocuments will be hooked together with their parent
documents into a single giant view tree. (All the work has been done, we're just
waiting to post-1.0 to throw the switch.) At that time you'll be able to take a
view and follow that view's parents up to the very root of the uber-view-tree,
checking visibility as you go, without all the subshelltreeitem etc goop.

Comment 17

[Brian Ryner (not reading)]

Right, I believe the only reason this is necessary is because of the sneaky way
that nsDeckFrame hides its non-active panes, that is, by directly manipulating
view visibility of a single view rather than by changing CSS style.  So, I'm
going on the assumption that (for now), this is actually the only way to do what
I want to do.  New patch coming up (with comments) for review.

Comment 18

[Brian Ryner (not reading)]

Attachment: patch for review

Ok, here it is.  I decided to keep the check for nsIBaseWindow visibility, it
seems possible that there are embedding cases where this is our only indicator
that the document is hidden (and it's cheap to check).

Comment 19

[Brian Ryner (not reading)]

cc'ing Adam for docshell input -- Adam, what do you think about moving these
additional visibility checks into nsDocShell::GetVisibility() ?

Comment 20

[Brian Ryner (not reading)]

Attachment: patch to put the checking in docshell instead

Same functionality, but I moved the logic into nsDocShell::GetVisibility.

Comment 21

[John Keiser (jkeiser)]

Comment on attachment 78651 [details] [diff] [review]
patch to put the checking in docshell instead

r=jkeiser

Be aware that JST's iframe change in bug 52334 will change some of the
mechanics of this.  Or rather, JST should be aware that he will have to change
the mechanics of this to fit his change :)

Comment 22

[Joe Hewitt (gone)]

Comment on attachment 78651 [details] [diff] [review]
patch to put the checking in docshell instead

sr=hewitt

Comment 23

[Brian Ryner (not reading)]

Checked in on the trunk.

Comment 24

[Jaime Rodriguez, Jr.]

Pls update the bug, when it has been tested on the trunk.

Comment 25

[sairuh (rarely reading bugmail)]

tested this using commercial trunk builds (2002.04.16) on linux rh7.2, win2k and
mac 10.1.3. so far, so good!

Comment 26

[Michael Buckland]

per adt we don't want to take this risk for beta, keep baking in the trunk.

Comment 27

[Nicholas Allen]

According to lxr, the checkin for this bug added a printf

printf("rootView: dim=(%d,%d)\n", viewBounds.width, viewBounds.height);

to nsDocShell::GetVisibility.  Was this intentional?

Comment 28

[Brian Ryner (not reading)]

Good catch.  I just removed the printf.

Comment 29

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on behalf ADT's behalf) for checkin to the 1.0 branch, pending
Driver's approval. Pls check this in asap.

Comment 30

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1"
keyword and add the "fixed1.0.1" keyword.

Comment 31

[Brian Ryner (not reading)]

checked in on the 1.0 branch.

Comment 32

[Jesse Ruderman]

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

Comment 33

[sairuh (rarely reading bugmail)]

tested ctrl+[shift]+tab and [shift]+F6 per the test in bug 104977 comment 4. all
looks well using comm branch builds (2002.06.13.0x) on linux rh7.2, mac 10.1.5
and win2k.