358202 - rss icon status in url bar is unchanged when drag&drop opens hovered over tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Arie Paap [:wildmyron]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061026 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061026 Minefield/3.0a1

This seems to be fallout from bug 248612

When dragging a selection (or a link) from one tab with the rss icon displayed in the url bar to another tab without the rss icon, the icon is displayed until the mouse is released.

Reproducible: Always

Steps to Reproduce:
1. Have multiple tabs open, some with rss feeds available
2. Select text
3. Drag selection and mouse over other tabs

Actual Results:  
The status of the rss icon is unchanged

Expected Results:  
Status should change according to the open tab

Comment 1

[Adam Guthrie]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061025 Minefield/3.0a1

This works for me on Linux. I even tested with the security icon, and that worked too. Windows only?

Comment 2

[Arie Paap [:wildmyron]]

Thanks for checking.

AFAICT, only the feed icon is affected on Windows. The security icon, url, favicon, navigation buttons and status bar all work.

Changing to trunk as that is where I meant to put this.

Comment 3

[Brian Toms]

Bug appears for me.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061026 Minefield/3.0a1 ID:2006102604 [cairo]

Comment 4

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061026 Minefield/3.0a1 ID:2006102621 [cairo]

confirmed

Comment 5

[Michael Ventnor]

Attachment: Patch

This does it.

Comment 6

[Peter van der Woude [:Peter6]]

Looks trivial enough for 1.8.1.1 (but I can't set the flag)

Comment 7

[Michael Ventnor]

(In reply to comment #6)
> Looks trivial enough for 1.8.1.1 (but I can't set the flag)
> 

Peter, the patch for bug 248612 didn't go on branch AFAICT so this wouldn't either.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 243760 [details] [diff] [review]
Patch

General comment: toolkit bindings cannot depend on browser/ code, at all. XULBrowserWindow is only available here becuase browser.js sets window.XULBrowserWindow in its startup method (plus, asyncUpdateUI isn't part of any API).

I'm not sure what's the cause of this bug, we're supposed to run onLocationChange here...

Comment 9

[Martijn Wargers (dead)]

Attachment: patch

Remove timer in onlocation handler.
Not sure why this is set in a setTimeout.

Comment 10

[Asaf Romano (gone)]

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

Probably due to Tp impact... I would review a hack which calls asyncUpdateUI direcly if we're in the middle of a dragging event :-/

Comment 11

[Asaf Romano (gone)]

directly, too.

Comment 12

[Martijn Wargers (dead)]

Well, the patch for bug 248612 also impacted Tp for a certain platform, but it was still decided to check it in.
I think all these setTimeout calls are a hack themselves to 'cheat' on Tp numbers. But are those really making a difference in pageload as perceived by users?

Anyway, this wouldn't be a problem if timers would fire on windows while performing a drag operation, so I'm making this dependent on bug 203573.

Comment 13

[Asaf Romano (gone)]

So, I think that in this particular case, setTimeout is the right thing.

I think that the workaround in comment 10 is worth doing, for now. Otherwise we should back the drag-patch out until bug 203573 is fixed.

Comment 14

[Martijn Wargers (dead)]

Attachment: patch

Maybe this then?
When aRequest is null, then update directly. This only happens when a tab gets switched, not when a different url gets loaded.

Comment 15

[Asaf Romano (gone)]

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

>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.726
>diff -u -8 -p -r1.726 browser.js
>--- browser/base/content/browser.js	24 Oct 2006 16:14:22 -0000	1.726
>+++ browser/base/content/browser.js	31 Oct 2006 00:02:56 -0000
>@@ -3847,18 +3847,22 @@ nsBrowserStatusHandler.prototype =
>       // Close the Find toolbar if we're in old-style TAF mode
>       gFindBar.closeFindBar();
>     }
> 
>     //fix bug 253793 - turn off highlight when page changes
>     if (document.getElementById("highlight").checked)
>       document.getElementById("highlight").removeAttribute("checked");
> 
>-    var self = this;
>-    setTimeout(function() { self.asyncUpdateUI(); }, 0);
>+    if (aRequest) {

Add a comment explaining this, and refer to this bug.

>+      var self = this;
>+      setTimeout(function() { self.asyncUpdateUI(); }, 0);
>+    } else {

nit: make this
 }
 else
>+      this.asyncUpdateUI();
>+    }
>   },

r=mano

Comment 16

[Martijn Wargers (dead)]

Attachment: patch, adressing comments

So like this? I'm just asking review again, although it is maybe redundant in this case.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 244299 [details] [diff] [review]
patch, adressing comments

>Index: browser/base/content/browser.js
>===================================================================
>+    // See bug 358202, when tabs are switched during a drag operation,
>+    // timers don't fire on windows (bug 203573)
>+    if (aRequest) {
>+      var self = this;
>+      setTimeout(function() { self.asyncUpdateUI(); }, 0);
>+    } 
>+    else {
>+      this.asyncUpdateUI();
>+    }

Remove the braces around the else block.

r=mano.

Comment 18

[Martijn Wargers (dead)]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.730; previous revision: 1.729
done

I removed the braces around the else block, before I checked it in
->fixed.

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> Anyway, this wouldn't be a problem if timers would fire on windows while
> performing a drag operation, so I'm making this dependent on bug 203573.

Should this patch be backed out, now that bug 389931 (which fixed bug 203573) is fixed ?

Comment 20

[Martijn Wargers (dead)]

Yes, it might very well now be possible to back out this patch.

Comment 21

[Serge Gautherie (:sgautherie)]

Can you look into this ?