Closed Bug 577892 Opened 14 years ago Closed 10 years ago

javascript change of href attribute of link rel="icon" doesnt invalidate the icon

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 33
Iteration:
33.3

People

(Reporter: tomas.hubelbauer, Assigned: Gijs)

References

Details

(Whiteboard: [parity-Chrome][parity-IE])

Attachments

(2 files)

allow icons to change when href attribute is set directly,
6.33 KB, patch
Dolske
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
add test for dynamic favicon changes through altering a favicon's href,
4.39 KB, patch
Dolske
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; cs; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; cs; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6

ive created a test page to reproduce this behavior. youd except icon would change after button click, but it doesnt. the href attribute changes. when i drag a panel into a new firefox window, new icon is already shown, so i guess its problem with user interface rendering.

<html>
  <head>
    <title>Test</title>
    <link rel="icon" href="calm.png"/>
  </head>
  <body>
    <button onclick="document.getElementsByTagName('link')[0].href='hot.png';">Simulate Event</button>
  </body>
</html>

Reproducible: Always

Steps to Reproduce:
try the example code in description
Actual Results:  
icon doesnt invalidate

Expected Results:  
icon should change to a new one
Version: unspecified → 3.6 Branch
Reporter, Firefox 4.0.1 has been released, and it features significant improvements over previous releases. Can you please update to Firefox 4.0.1 or later, and retest your bug? Please also create a fresh profile (
http://support.mozilla.com/kb/Managing+profiles), update your plugins (Flash, Java, Quicktime, Reader, etc) and update your graphics driver and Operating system to the latest versions available. 

If you still continue to see this issue, please comment. If you do not, please close this bug as RESOLVED > WORKSFORME

filter: prefirefox4uncobugs
it still doesnt work.
You can make this work by removing the <link> node from the DOM, and then reinserting it... See updateFavicon() in the source of http://isthetreegreen.com

The browser UI deals with the favicon by listening for "DOMLinkAdded" events from content. There are no events fired (that I know of) when link attributes change. Other than adding a mutation observer (which is bad for perf),I don't think this is currently possible. Perhaps we could special case this by adding a DOMLinkChanged event?
Status: UNCONFIRMED → NEW
Ever confirmed: true
yes i already found out it was possible to create new instance of link node and delete the old one, and started using it. but i thought, it would be nice if it was possible to use the easier and more elegant way, by editing href attribute. like when you change image source with src attribute, but i understand link is not an image and mostly it's used for nonvisual content (stylesheets for example) so it makes sense that icon does not invaldiate immediately.
So this works (testcase: http://gijsk.com/mozilla/testcases/icons.html ). AIUI from bug 481688 these events shouldn't be visible to content either way, so adding another one doesn't seem like a big deal. For the browser part, note that while I've updated some terminology, 'addicon' already really meant 'seticon'. This wasn't the case for feeds and search (note how we don't do anything when these elements get removed, either...), and so I've just added guards so that we don't change behaviour here. I'm happy to remove one or both of those, but this seemed the safest bet.
Attachment #8450547 - Flags: review?(dolske)
Attachment #8450547 - Flags: review?(bzbarsky)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Component: General → Tabbed Browser
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [parity-Chrome][parity-IE]
Version: 3.6 Branch → Trunk
Marco, can you add this?
Iteration: --- → 33.2
Points: --- → 3
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to Iteration 33.2
QA Whiteboard: [qa?]
Flags: needinfo?(mmucci)
Comment on attachment 8450547 [details] [diff] [review]
allow icons to change when href attribute is set directly,

r=me on the DOM bits
Attachment #8450547 - Flags: review?(bzbarsky) → review+
Hi Gijs, can you mark this bug as [qa+] or [qa-].
Iteration: 33.2 → 33.3
Flags: needinfo?(gijskruitbosch+bugs)
There's an automated test, so I don't think this needs QA attention.
QA Whiteboard: [qa?] → [qa-]
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8450547 - Flags: review?(dolske) → review+
Comment on attachment 8450934 [details] [diff] [review]
add test for dynamic favicon changes through altering a favicon's href,

Review of attachment 8450934 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8450934 - Flags: review?(dolske) → review+
folded before landing as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/1829aad143e2
Flags: in-testsuite+
Whiteboard: [parity-Chrome][parity-IE] → [parity-Chrome][parity-IE][fixed-in-fx-team]
Depends on: 1037217
https://hg.mozilla.org/mozilla-central/rev/1829aad143e2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [parity-Chrome][parity-IE][fixed-in-fx-team] → [parity-Chrome][parity-IE]
Target Milestone: --- → Firefox 33
Depends on: 1083895
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: