Closed
Bug 420605
Opened 16 years ago
Closed 12 years ago
fragment links have wrong favicon and description in history entry
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: ori, Assigned: justin.lebar+bug)
References
()
Details
(Keywords: polish)
Attachments
(5 files, 12 obsolete files)
Using today's nightly build. When navigating to a page fragment, a new history item is created. Instead of having the page's favicon, it has the default favicon. Instead of having the page's title for a description, it has the filename part of the URL. To reproduce: 1) Visit http://en.wikipedia.org/wiki/Mozilla_Firefox 2) Click on "Release history" in the table of contents. *) Now at http://en.wikipedia.org/wiki/Mozilla_Firefox#Release_history 3) Open the history sidebar, sort by last visited. *) The last entry is Mozilla_Firefox, and it has a default favicon, instead of "Mozilla Firefox" and the Wikipedia favicon. Note that the back/forward history menu displays the correct description for the fragment.
Comment 1•16 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008030307 Minefield/3.0b4pre I see this too. I see this at least since end May and branch has no icons at all in history so it is not a regression.
Component: History → Places
OS: Linux → All
QA Contact: history → places
Hardware: PC → All
Comment 2•16 years ago
|
||
(In reply to comment #1) > branch has no icons at > all in history I meant only default favicons.
Comment 3•15 years ago
|
||
Currently I see there's two history entries after reproducing steps: one for "Mozilla Firefox - Wikipedia etc" with Wikipedia icon, and one for Mozilla_Firefox#Release_history, named Mozilla_Firefox, without favicon. Also in back/forward menu Mozilla_Firefox#Release_history has no favicon. IMO anchor links must not only have favicons, but they should be distinguished adding them anchor link name in history (in this example, Mozilla_Firefox#Release_history instead of Mozilla_Firefox). Anyway, confirmed. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052406 Minefield/3.0pre
Comment 4•15 years ago
|
||
This is a proof of concept for the icons; It basically works by shadowing the fragment entry in places with an entry for the page. This seems to work pretty well although it may still need a visit entry to keep the place entry alive for longer. The second part is The first part is a proof of concept for a problem that history has in displaying a suitable icon on visiting a fragment of a document - three guesses as to the bug number. The second part is just some early ideas for the titles but, as it already seems to work quite well, is included for further experimentation. I wasn't that keen on just showing #fragment but then it occurred to me that I could attempt to use information from the document, for example: Bug 420605 – fragment links have wrong favicon and description in history entry [Description] All-in-One Sidebar :: Firefox Extras [Avaliações] Web Server Statistics for Central Web Server [Redirection Report] and just use #fragment as a fallback Bristol University | A-Z index | Full [#w] There may be various problems with this - it doesn't seem to handle javascript changes very well - so the option is off by default.
Comment 5•15 years ago
|
||
(In reply to comment #4) > The second part is The first part is a proof of concept for a problem that > history has in displaying a suitable icon on visiting a fragment of a document > - three guesses as to the bug number. Sorry, I don't know how that 'paragraph' got back in.
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 6•15 years ago
|
||
Comment 7•15 years ago
|
||
This is an interesting workaround, but you think you can implement it in Firefox C++ source code?
Comment 8•15 years ago
|
||
(In reply to comment #4) > There may be various problems with this - it doesn't seem to handle javascript > changes very well - so the option is off by default. This turned out to be bug 438288
Attachment #323539 -
Attachment is obsolete: true
Comment 9•14 years ago
|
||
I have been sitting on this for a while so as to see the fate of the HTML5 (IE8) hashchange event (bug 385434); My preferred solution is to try an event listener for this (proposed) event and to set the icon and title in the database to the current ones. In the meantime the first part of this patch catches the hash change, in the onLocationChange of the progress listener, by comparing any Location containing a '#' with the previous location; The second part is what the event handler would do if it was available. For nightly testers, I will upload a new extension containing this code (version 0.4) to amo later today.
Attachment #324751 -
Attachment is obsolete: true
Comment 10•14 years ago
|
||
hashchange has now been implemented, so possibly something like this. I don't believe that I need the try block anymore but it is useful for testing
Attachment #367740 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #386967 -
Flags: review?(gavin.sharp)
Comment 11•14 years ago
|
||
The extension has been updated (0.5) on amo to reflect this latest change. [Nit^H^Hote for self: despite other handlers using 'evt' or 'event', change parameter name to 'aEvent']
Comment 12•14 years ago
|
||
Comment 13•14 years ago
|
||
john, can you ask review from gavin or another browser peer for this?
Updated•14 years ago
|
Severity: minor → normal
Comment 14•14 years ago
|
||
Hmm! I don't get a hashchange for this case but I do get a history entry; I am not sure if it is valid, but unraveling the specs is proving a bit hard...
Assignee | ||
Comment 15•14 years ago
|
||
re comment 14: Interesting. I get neither a hashchange nor a history entry for that testcase. At least on my browser, it appears that setting location.hash='#' actually sets location.hash to '', as illustrated by the testcase I just uploaded. I'm running the latest Shiretoko nightly on Linux. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1pre) Gecko/20090708 Shiretoko/3.5.1pre
Assignee | ||
Comment 16•14 years ago
|
||
Oops. I meant to test this on the latest trunk nightly. But I get the same results as above: Neither a hashchange nor a history entry for either of the testcases in comment 14 and comment 15. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090708 Minefield/3.6a1pre
Comment 17•14 years ago
|
||
This looks like a places bug, not a tabbrowser one. Why don't you fix it in places code?
Comment 18•14 years ago
|
||
(In reply to comment #17) > This looks like a places bug, not a tabbrowser one. Why don't you fix it in > places code? It seems to me to be a hybrid bug; Storing the favicons in the places database is currently done by tabbrowser so adding the fragment code here seems right; Storing the title is currently done in the docshell - as is firing the hashchange - but I am not sure that it should also store the title for fragments, so I put it here mostly for convenience. I would be happy if we just do the icon here and punt the title somewhere else. But where?
Comment 19•14 years ago
|
||
Rather than storing any extra data, why don't we use foo's title and favicon for foo#bar?
Comment 20•14 years ago
|
||
(In reply to comment #18) > Storing the title is currently done in the docshell - as is firing the > hashchange - but I am not sure that it should also store the title for > fragments, so I put it here mostly for convenience. > > I would be happy if we just do the icon here and punt the title somewhere else. > But where? Having found the appropriate code for session history, the docShell probably _is_ the correct place; Presumably something like: 7895 /* Set the title for the SH entry for this target url. so that 7896 * SH menus in go/back/forward buttons won't be empty for this. 7897 */ 7898 if (mSessionHistory) { 7899 PRInt32 index = -1; 7900 mSessionHistory->GetIndex(&index); 7901 nsCOMPtr<nsIHistoryEntry> hEntry; 7902 mSessionHistory->GetEntryAtIndex(index, PR_FALSE, 7903 getter_AddRefs(hEntry)); 7904 NS_ENSURE_TRUE(hEntry, NS_ERROR_FAILURE); 7905 nsCOMPtr<nsISHEntry> shEntry(do_QueryInterface(hEntry)); 7906 if (shEntry) 7907 shEntry->SetTitle(mTitle); 7908 } 7909 + /* Set the title for the Global History entry for this target url. */ + if (mGlobalHistory) { + mGlobalHistory->SetPageTitle(mCurrentURI, mTitle); + } + 7910 if (doHashchange) { But I would be out of my depth working here.
Comment 21•14 years ago
|
||
(In reply to comment #19) > Rather than storing any extra data, why don't we use foo's title and favicon > for foo#bar? This was the approach in comment #4; The problem is you may never have visited foo (you may have got to foo#bar from foo#ey) which then mean that you have to create an entry for foo which (a) can disappear as there is no visit for it and (b) creates a lot of database queries to track it.
Attachment #386967 -
Attachment is obsolete: true
Attachment #386967 -
Flags: review?(gavin.sharp)
Comment 22•14 years ago
|
||
(In reply to comment #21) > This was the approach in comment #4; The problem is you may never have visited > foo (you may have got to foo#bar from foo#ey) Well, that doesn't mean that you haven't visited foo before foo#ey. Also, if you visit foo#bar directly without a hash change, we store the favicon and the title. So the only remaining edge case is really visiting foo#ey directly (i.e. not foo) and then foo#bar. That's neither dramatic nor very common, and it doesn't justify the bloat your patch would add, imho.
Updated•14 years ago
|
Attachment #387858 -
Attachment is obsolete: true
Comment 23•14 years ago
|
||
Move handler to browser.js The bad news is that is not so easy to find the relevant browser. The good news is that the code is now a good match for the "icon" code in the DOMLinkAdded handler.
Comment 24•14 years ago
|
||
Let my slightly rephrase an earlier question: Why does Places store random respectively no data for foo#bar, instead of using foo's title and favicon? Wouldn't it be nicer if that worked out of the box?
Comment 25•14 years ago
|
||
(In reply to comment #20) > Having found the appropriate code for session history, the docShell probably > _is_ the correct place; Presumably something like: ... > 7909 > + /* Set the title for the Global History entry for this target url. */ > + if (mGlobalHistory) { > + mGlobalHistory->SetPageTitle(mCurrentURI, mTitle); > + } > + > 7910 if (doHashchange) { > > But I would be out of my depth working here. I am afraid that I have punted this to bug 503832 - so it will now need a c++ programmer. (In reply to comment #24) > Let my slightly rephrase an earlier question: Why does Places store random > respectively no data for foo#bar, instead of using foo's title and favicon? > Wouldn't it be nicer if that worked out of the box? As used here places is just an implementation of nsIGlobalHistory2 so it is not reasonable for it to do anything but store and retrieve titles as requested by the browser; Note that Seamonkey 1.1.17 also exhibits the same behaviour as this bug - use STR from comment #0 and then Go/History - which convinces me it is a Core bug. By analogy the icons are stored by the browser (on DOMLinkAdded in browser.js via setIcon in tabbrowser which calls the favicon service) and so the patch does the same on hashchange for the fragment URLs; Note that hashchange is fired just after the session history title is updated and hopefully where the global title will be.
Comment 26•14 years ago
|
||
Check aEvent.isTrusted setAndLoadFaviconForPage checks isFailedIcon so don't do it here.
Attachment #388194 -
Attachment is obsolete: true
Comment 27•14 years ago
|
||
(In reply to comment #25) > As used here places is just an implementation of nsIGlobalHistory2 so it is not > reasonable for it to do anything but store and retrieve titles as requested by > the browser I haven't checked if nsIGlobalHistory2 is explicit about this. Maybe the interface isn't sane, or maybe Places could interpret it more laxy. > Note that Seamonkey 1.1.17 also exhibits the same behaviour as > this bug - use STR from comment #0 and then Go/History - which convinces me it > is a Core bug. The fact that multiple consumers suffer from this actually seems to support that either the interface or the implementation should be fixed. Btw, without being convinced that you're using the right approach, note that this structure is unnecessarily complicated: const hashChangeHandler = { handleEvent: function (aEvent) { switch (aEvent.type) { case "hashchange": this.onHashChange(aEvent); break; } }, onHashChange: function(aEvent) { FOO } }; Use this instead: function hashChangeHandler(aEvent) { FOO }
Comment 28•14 years ago
|
||
(In reply to comment #27) > I haven't checked if nsIGlobalHistory2 is explicit about this. Maybe the > interface isn't sane, or maybe Places could interpret it more laxy. s/laxy/laxly/
Comment 29•14 years ago
|
||
Comment on attachment 389111 [details] [diff] [review] patch v4 >+ var browser = gBrowser.getBrowserForDocument(aEvent.target.document); >+ if (!browser) >+ return How about: if (aEvent.target != content) return; var browser = gBrowser.selectedBrowser;
Comment 30•14 years ago
|
||
(In reply to comment #27) > Btw, without being convinced that you're using the right approach, note that > this structure is unnecessarily complicated: Yes I think I must have been using 'this' at some stage which made it convenient. (In reply to comment #29) > How about: > > if (aEvent.target != content) > return; > > var browser = gBrowser.selectedBrowser; I'm pretty sure that the event could fire on any browser so need to identify which one it was.
Comment 31•14 years ago
|
||
It could, but even if you care about that case, you should still check the 'content' shortcut.
Comment 32•14 years ago
|
||
Attachment #389111 -
Attachment is obsolete: true
Attachment #389119 -
Attachment is obsolete: true
Comment 33•14 years ago
|
||
Attachment #390209 -
Attachment is obsolete: true
Updated•14 years ago
|
Assignee: nobody → john.p.baker
Comment 34•14 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Comment 35•13 years ago
|
||
Apart from ruining the aesthetics of the affected history entries, this bug also causes problems for add-ons that query history and rely on description + favicon being correct (e.g. BarTab). So it'd be quite useful if we could decide whether John's patch was the correct way to proceed, especially with the docshell part (bug 503832) being fixed now (or soon to be fixed again ;)). I have attached a patch against mozilla-central containing John's bugfix and a test case exercising the fix. Dietrich suggested Gavin as a reviewer in comment 13, so I'm submitting it to him. Cheers!
Attachment #431922 -
Flags: review?(gavin.sharp)
Comment 36•13 years ago
|
||
Here's an updated version of the patch containing John's bugfix and my test case. The only difference between this and the previous version of the patch concerns bug 544097. Since that landed, tests are no longer supposed to use localhost:8888 but mochi.test:8888. Would be cool if we can reach a verdict whether this bugfix can go in or not. I certainly wouldn't mind seeing this (and bug 503832) fixed for the Firefox 3.6.x line. ;)
Attachment #431922 -
Attachment is obsolete: true
Attachment #433336 -
Flags: review?(gavin.sharp)
Attachment #431922 -
Flags: review?(gavin.sharp)
Comment 37•13 years ago
|
||
It seems wasteful to have favicon entries for every fragment navigated to, and it kind of sucks to have to add this kind of code at this level (in the app vs. in the core). Couldn't we instead change the favicon service to ignore the fragment when responding to queries, and just return the base page's favicon? That means we lose support for AJAXy pages that dynamically change their favicon based on the fragment, but I'm not sure that's really worth supporting, and doing the simpler thing now doesn't really preclude us from doing more later AFAICT.
Comment 38•13 years ago
|
||
(In reply to comment #37) > Couldn't we instead change the favicon service to ignore the fragment when > responding to queries, and just return the base page's favicon? That means we > lose support for AJAXy pages that dynamically change their favicon based on the > fragment, but I'm not sure that's really worth supporting, and doing the > simpler thing now doesn't really preclude us from doing more later AFAICT. This shouldn't be too hard to do, and I agree that it is the better approach.
Comment 39•13 years ago
|
||
I'd be fine with that, and I certainly agree with the sentiment of rather not having this code in the app. Note that changing the favicon in JavaScript after having changed window.location.hash will actually store the right favicon right now (tested on FF 3.6) since that explicitly triggers a store in the favicon service. So as Gavin said, that "feature" would be lost. I personally don't feel strongly about that, so I'm ok with ditching it in the process. Gavin's suggestion would basically boil down to the favicon service normalising URIs to the base URI before both storing and querying. The normalisation when storing is necessary because one might visit http://host/page#fragment and move on to other fragments before ever visiting http://host/page. I'll work on a patch for this.
Comment 40•13 years ago
|
||
(In reply to comment #37) > It seems wasteful to have favicon entries for every fragment navigated to, and > it kind of sucks to have to add this kind of code at this level (in the app vs. > in the core). There is already a history entry and the favicon is probably already stored so there isn't actually much extra stored - a favicon_id instead of a null. > Couldn't we instead change the favicon service to ignore the fragment when > responding to queries, and just return the base page's favicon? That means we > lose support for AJAXy pages that dynamically change their favicon based on the > fragment, but I'm not sure that's really worth supporting, and doing the > simpler thing now doesn't really preclude us from doing more later AFAICT. Some of the earlier experiments (see proof of concept attachments) on this bug tried that approach; The trouble is that there may not be an entry for the base page so that, on visit to http://host/page#fragment, they had to create an entry for http://host/page which would be lost on browser shutdown as there was no visit. [This can also then delete the favicon data from the database as there may now be no reference to it] I also suspect that the queries for history that include the favicon will become very hairy such that the right decision becomes to store the favicon relation when you know what it is - rather than trying to figure it out later.
Updated•13 years ago
|
Attachment #433336 -
Flags: review?(gavin.sharp)
Comment 41•12 years ago
|
||
Here is another test case which fails in all Firefox versions (including 4.0): http://jsbin.com/ulaxo4
Assignee | ||
Comment 42•12 years ago
|
||
Now that we've fixed bug 655270, we should be able to use the same approach to fixing this bug: On a hash load, call into the favicon service and tell it that the new favicon is the same as the old page's favicon. John, do you mind if I pick up this bug?
Assignee | ||
Comment 43•12 years ago
|
||
(In reply to comment #37) > It seems wasteful to have favicon entries for every fragment navigated to, Maybe, but in bug 655270, we made it so each pushState causes a new favicon entry. I don't think this is a lot different. > and it kind of sucks to have to add this kind of code at this level (in the > app vs. in the core). I can move it into docshell if you'd prefer.
Comment 44•12 years ago
|
||
Yes, having this code in docshell would be better.
Assignee | ||
Comment 45•12 years ago
|
||
Attachment #534310 -
Flags: review?(gavin.sharp)
Comment 46•12 years ago
|
||
Comment on attachment 534310 [details] [diff] [review] Patch v1 - Send notification from docshell >diff --git a/docshell/test/browser/browser_bug420605.js b/docshell/test/browser/browser_bug420605.js >+ gBrowser.selectedBrowser.addEventListener( >+ "DOMContentLoaded", onPageLoad, true); Probably a good idea to remove this listener at some point (even though you're removing the tab it's added on). Looks great to me, but I'm not a docshell peer.
Attachment #534310 -
Flags: review?(gavin.sharp) → feedback+
Assignee | ||
Comment 47•12 years ago
|
||
Comment on attachment 534310 [details] [diff] [review] Patch v1 - Send notification from docshell Jonas, do you want to review the docshell bits?
Attachment #534310 -
Flags: review?(jonas)
Assignee | ||
Updated•12 years ago
|
Attachment #391288 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #433336 -
Attachment is obsolete: true
Updated•12 years ago
|
Assignee: john.p.baker → justin.lebar+bug
Attachment #534310 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 48•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4ae5181b22f7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 49•12 years ago
|
||
(In reply to comment #48) > http://hg.mozilla.org/mozilla-central/rev/4ae5181b22f7 Thanks, Justin, for finishing this up!
Updated•12 years ago
|
Target Milestone: --- → Firefox 7
Updated•12 years ago
|
Component: Bookmarks & History → Document Navigation
Product: Firefox → Core
QA Contact: bookmarks → docshell
Target Milestone: Firefox 7 → mozilla7
You need to log in
before you can comment on or make changes to this bug.
Description
•