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

RESOLVED FIXED

Status

()

Firefox
Tabbed Browser
--
trivial
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: wildmyron, Assigned: Martijn Wargers (zombie))

Tracking

({regression})

Trunk
x86
Windows XP
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
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?
Version: unspecified → 2.0 Branch
(Reporter)

Comment 2

12 years ago
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.
Version: 2.0 Branch → Trunk

Comment 3

12 years ago
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]
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061026 Minefield/3.0a1 ID:2006102621 [cairo]

confirmed
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Comment 5

12 years ago
Created attachment 243760 [details] [diff] [review]
Patch

This does it.
Attachment #243760 - Flags: review?(mano)
Looks trivial enough for 1.8.1.1 (but I can't set the flag)

Comment 7

12 years ago
(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 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...
Attachment #243760 - Flags: review?(mano) → review-
(Assignee)

Comment 9

12 years ago
Created attachment 243774 [details] [diff] [review]
patch

Remove timer in onlocation handler.
Not sure why this is set in a setTimeout.
Attachment #243774 - Flags: review?(mano)
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 :-/
Attachment #243774 - Flags: review?(mano) → review-
(Assignee)

Comment 12

12 years ago
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.
Depends on: 203573
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.
(Assignee)

Comment 14

12 years ago
Created attachment 244146 [details] [diff] [review]
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.
Attachment #244146 - Flags: review?(mano)
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
Attachment #244146 - Flags: review?(mano) → review+
(Assignee)

Comment 16

12 years ago
Created attachment 244299 [details] [diff] [review]
patch, adressing comments

So like this? I'm just asking review again, although it is maybe redundant in this case.
Attachment #244299 - Flags: review?(mano)
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.
Attachment #244299 - Flags: review?(mano) → review+
(Assignee)

Comment 18

12 years ago
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.
Assignee: nobody → martijn.martijn
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(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 ?
Blocks: 248612, 326273, 381699
Depends on: 389931
No longer depends on: 203573
Attachment #243774 - Attachment is obsolete: true
Attachment #243760 - Attachment is obsolete: true
(Assignee)

Comment 20

10 years ago
Yes, it might very well now be possible to back out this patch.
Attachment #244146 - Attachment is obsolete: true
Can you look into this ?
Blocks: 420627
You need to log in before you can comment on or make changes to this bug.