206668 - [Mac OS X classic theme] context menu only work on frontmost tab, additional tabs opening

Status: VERIFIED FIXED

(Core Graveyard :: Skinability, defect)

Description

[sairuh (rarely reading bugmail)]

found using jag's test build for bug 203960 on Mac OS X (10.2.6). specific to
the classic theme, not a problem on modern.

1. have multiple tabs open.
2. bring up the context menu for the tab bar over one of the background tabs.
3. select either "close tab" or "close other tabs".

expected: if "close tab" is selected, the background tab should close. if "close
other tabs", then all other tabs except the background one (where the pointer
was over) should close.

actual results: regardless of where the mouse pointer is, the frontmost tab is
closed when "close tab" is selected, or remains the only one open when "close
other tabs" is selected.

Comment 1

[jag (Peter Annema)]

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

Comment 2

[jag (Peter Annema)]

Seems like a regression from bug 192029.

Comment 3

[sairuh (rarely reading bugmail)]

found another case where this (classic-only bug) occurs, so making the summary
more general.

1. have a group tabs open, eg, containing 3 tabs.

2. make sure the 1st tab is the frontmost one, and that 2nd and 3rd remain in
background.

3. do any of the following:

* from the 1st tab, drag a link onto either the 2nd or 3rd tab.
* drag a bookmark from the PT onto either the 2nd or 3rd tab.
* drag the proxy icon from the urlbar onto either the 2nd or 3rd tab.

expected: that link/url should load in that background tab.

actual results: a new tab is added, loading that link/url. (not a problem with
today's verification build.)

Comment 4

[Samir Gehani]

adt: nsbeta1+/adt2

Comment 5

[shliang]

Attachment: patch

Comment 6

[jag (Peter Annema)]

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

>Index: tabbrowser.xml
>===================================================================
>@@ -955,10 +967,17 @@
>             var bgLoad = this.mPrefs.getBoolPref("browser.tabs.loadInBackground");
> 
>             if (aEvent.target.localName == "tabs") {
>-              // We're adding a new tab.
>-              var tab = this.addTab(getShortcutOrURI(url));
>-              if (!bgLoad)
>-                this.selectedTab = tab;
>+              if (aEvent.originalTarget.localName == "tab") {
>+                this.getBrowserForTab(aEvent.originalTarget).loadURI(getShortcutOrURI(url));
>+                if (!bgLoad)
>+                  this.selectedTab = aEvent.originalTarget;
>+              }
>+              else {
>+                // We're adding a new tab.
>+                var tab = this.addTab(getShortcutOrURI(url));
>+                if (!bgLoad)
>+                  this.selectedTab = tab;
>+              }
>             }
>             else if (aEvent.target.localName == "tab") {
>               // Load in an existing tab.

You're duplicating the code here for loading the URI into an existing tab. One
way around this is something like this:

	    var tab;
	    if (aEvent.target.localName == "tabs" &&
		aEvent.originalTarget.localName != "tab") {
	      // We're adding a new tab.
	      tab = this.addTab(getShortcutOrURI(url));
	      if (!bgLoad)
		this.selectedTab = tab;
	    }
	    else if ((tab = aEvent.target).localName == "tab" ||
		     (tab = aEvent.originalTarget).localName == "tab") {
	      // Load in an existing tab.
	      this.getBrowserForTab(tab).loadURI(getShortcutOrURI(url));
	      if (!bgLoad && this.mCurrentTab != tab)
		this.selectedTab = tab;
	    }

Though I'm wondering if this would suffice (should, but not sure, needs
testing):

	    var tab = null;
	    if (aEvent.originalTarget.localName == "tabs") {
	      // We're adding a new tab.
	      tab = this.addTab(getShortcutOrURI(url));
	    }
	    else if (aEvent.originalTarget.localName == "tab") {
	      // Load in an existing tab.
	      tab = aEvent.originalTarget;
	      this.getBrowserForTab(tab).loadURI(getShortcutOrURI(url));
	    }
	    if (tab && !bgLoad && this.mCurrentTab != tab)
	      this.selectedTab = tab;


Regardless, this patch is a hack around a problem in the retargetting code. I
might sr this for the branch as a last minute fix if we can't come up with the
real fix in time.

Comment 7

[[e-mail]]

So this is more event retargeting fallout? :-( No wonder I can't duplicate the
bug, I still have a fix in my tree from the last bug (menulist selection). In
that case the workaround was to use event.originalTarget instead of event.target
so I agree with jag's comment there. Is the document.popupNode problem also
caused by event retargeting?

Comment 8

[[e-mail]]

Whoops, should read the OS more carefully :-[

Comment 9

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4) Gecko/20030529]

Bug not present in v1.4rc1 on W95:
I checked both comment 0 and comment 3.

Based on comment 8, if this bug actually is Macintosh/MacOS_X specific (as I
understand), could you add this in the summary, like bug 94175 ? (Thanks.)

Comment 10

[shliang]

Attachment: cleaned up patch

Comment 11

[shliang]

Attachment: cleaned up more patch

Comment 12

[jag (Peter Annema)]

Comment on attachment 125480 [details] [diff] [review]
cleaned up more patch

sr=jag for 1.4 branch

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 125480 [details] [diff] [review]
cleaned up more patch

a=asa (on behalf of drivers) for checkin to the 1.4 branch.

Comment 14

[jag (Peter Annema)]

This actually seems to be a bug in the event retargetting code.

Here's the problem:

<content>
  <hbox>
    <children/>
  </hbox>
</content>

Say the above represents the basic structure of <tabs/>, which in our code we
use like this:

<tabs>
  <tab/>
  <tab/>
  ...
</tabs>

From the point of view of <tabs/>, an event on <tab/> should have
event.target.localName == "tab".

With this change:

<content>
  <hbox>
    <hbox>
      <children/>
    </hbox>
  </hbox>
</content>

an event on <tab/> now gets us event.target.localName == "tabs"

Ain't that odd?

Now, for 1.4 the above is what we've come up with, but for the trunk we should
fix the above problem.

Comment 15

[jag (Peter Annema)]

Comment on attachment 125480 [details] [diff] [review]
cleaned up more patch

Sorry, will have to minus this. There's no guarantee
document.popupNode.localName is set before the onmousedown handler in
tabbrowser gets called (in fact, I just tested for that and it was null,
resulting in the first context menu "Close Tab" to fail). I think this could be
fixed by just dropping that part of the if check, but will defer to Shuehan.

Comment 16

[[e-mail]]

event.originalTarget should be reliable, because the tab's boxObject will ensure
that the event was originally targetted at itself and not at its content.

I guess that the problem is the same as the modern menulist problem (bug 196755)
- for which I believe my attachment 117071 [details] [diff] [review] is still usable.

Comment 17

[shliang]

Attachment: patch

Comment 18

[jag (Peter Annema)]

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

Comment 19

[[e-mail]]

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

Looks good, and doesn't break anything on Linux.

Comment 20

[[e-mail]]

+            if (this.mCurrentTab != tab  && !bgLoad)
I fixed this spacing when checking in   ^^

Comment 21

[Grace Bush]

Sarah,

Will you check this?  I am able to reproduce exactly what you reported-wrong
tabs get closed per steps in your first comment - and steps in comment #3
thx

Comment 22

[sairuh (rarely reading bugmail)]

tested using 2003.06.13.08-trunk (commercial) bits, OS X 10.2.6, classic theme.

comment 0: i was able to repro this problem *only once*, during the first
session of the build. during that first session, and on subsequent sessions, it
was *no longer* a problem.

comment 3: these worked fine (unable to repro).

i'd say that this is prolly good enough for now (esp the branch, if/when it
lands there).

Grace, are you able to repro the problems consistently/frequently?

Comment 23

[Grace Bush]

thanks Sarah,
I tested on today's branch- just retested on trunk and all is well- could not
reproduce any problems!

Comment 24

[Samir Gehani]

a=adt to land this on the 1.4 branch.  Please add the fixed1.4 keyword once you
land this.

Comment 25

[Samir Gehani]

It appears that the patch for this bug was backed out.  Removing fixed1.4 keyword.

Please don't land unless you get explicit approval from drivers and adt again. 
Thanks.

Comment 26

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.4 branch.

Comment 27

[Samir Gehani]

a=adt to land this on the 1.4 branch by tonight (before 2003-06-19 5am and no
later please).  Please mark this with the fixed1.4 keyword once this lands. 
Thanks.  

Comment 28

[sairuh (rarely reading bugmail)]

checked into 1.4 branch at 20:35 today.

Comment 29

[Grace Bush]

verified on branch build 20030619

Comment 30

[Jo Hermans]

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

Comment 31

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.

Comment 32

[HJ]

I really wonder why you need both onclick=..." and onmousedown="..." set on
<tabs> This small patch should work on all platforms:

-this.mContextTab = document.popupNode;
+this.mContextTab = (aEvent.originalTarget.localName == "tab") ?
eEvent.originalTarget : document.popupNode;

You don't need onmousedown and that method in tabbrowser.xml

Comment 33

[[e-mail]]

HJ, I can't tell where your patch is supposed to apply; updatePopupMenu doesn't
have access to an appropriate event, which is why onmousedown was used (although
oncontextmenu would have worked just as well, I since realized).

Of course if the XBL bug was fixed we could ditch this hack.

Comment 34

[HJ]

I would say remove this line:

onmousedown="this.parentNode.parentNode.parentNode.updateContextTab(event);"

and the corresponding method. onclick has that event and it exactly what should
be used, at least that is how I look at this. After all, you clicked on the tab,
right? You could add a single line like this to method 'onTabClick':

this.mContextTab = (aEvent.originalTarget.localName == "tab") ?
aEvent.originalTarget : document.popupNode;

to replace method updateContextTab. This will prevent the XBL, in case it still
happens, but doesn't break backward compatibility, because that is exactly what
adding new methods does. Well, in case you're an add-on like MultiZilla. 

onmousedown and onclick are almost the same things, so why not use just one of them?

Another way of solving this is to replace 'this' with 'event' for pdatePopupMenu
and to add the line in that method and to replace 'aPopupMenu' with
'this.mContextTab'

Comment 35

[[e-mail]]

> onmousedown and onclick are almost the same things, so why not
> use just one of them?

On some platforms the context menu event is triggered by the right
mouse button down event, not by the right mouse button click event.

> Another way of solving this is to replace 'this' with 'event' for
> updatePopupMenu and to add the line in that method and to replace 
> 'aPopupMenu' with 'this.mContextTab'

Except the event target for updatePopupMenu is the menupopup...

Comment 36

[HJ]

"Except the event target for updatePopupMenu is the menupopup..."

So I noticed. I now know what I was looking for, thanks Neil :-)