Closed Bug 87428 (LinkUI) Opened 23 years ago Closed 23 years ago

No UI HTML <link> element

Categories

(SeaMonkey :: General, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0

People

(Reporter: gerv, Assigned: drbrain-bugzilla)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [Hixie-P2] SPEC: http://www.bath.ac.uk/%7Epy8ieh/internet/discussion/linkspec.txt)

Attachments

(33 files, 2 obsolete files)

(deleted), application/x-xpinstall
Details
(deleted), application/x-xpinstall
Details
(deleted), application/octet-stream
Details
(deleted), application/x-xpinstall
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/plain
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/xml
Details
(deleted), application/x-xpinstall
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), application/xhtml+xml
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), application/x-xpinstall
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), application/octet-stream
Details
(deleted), application/x-xpinstall
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), application/x-xpinstall
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), application/x-xpinstall
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), application/zip
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), application/octet-stream
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/plain
Details
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Continuation bug from bug 2800, which was getting unwieldy. Current implementation: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=39798 GNU Make manual testcase: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=39630 Note: bug 2800 had 60 votes. Newsgroup discussion of the current implementation has been started in netscape.public.mozilla.ui. Let's thrash this out there :-) Gerv
Blocks: html4.01, 62399, 68419
Status: NEW → ASSIGNED
Keywords: mozilla1.0
Priority: -- → P3
Whiteboard: [Hixie-P2] SPEC: http://www.bath.ac.uk/%7Epy8ieh/internet/discussion/linkspec.txt
Target Milestone: --- → mozilla1.0
*** Bug 2800 has been marked as a duplicate of this bug. ***
Blocks: 59118
Current issues for this bug: UI design (to be discussed in n.p.m.ui), this includes which rel types get displayed where. Display of non-HTML4 rel types (n.p.m.ui) Tooltips Localization Icons Implementation of keyboard shortcuts (which keyboard shortcuts is for n.p.m.ui) Lets please keep traffic on this bug to actual implementation details. UI is for the newsgroups because its mostly subjective.
Attached file Skin support added w/icons (deleted) —
This is the latest latest implementation. Skin support has been added. Your best bet is to install from a clean Mozilla.
ack! un-cc myself.
Has anyone been able to install from the attachments? I managed to do it from http://segment7.net/mozilla/linktoolbar/index.html but none of the attachments upgrade it.I think I'm missing a step.
Oops, I screwed up and misunderstood the install script. The right thing to do is to save the original attachment to a local file ending in .xpi and then open it in mozilla. Just clicking on the link fails with a -207 error in the install log (on mac build 2001062008).
Peter, I had to add the following lines to installed-chrome.txt before the toolbar worked: content,install,url,jar:resource:/chrome/linktoolbar.jar!/content/linktoolbar/ locale,install,url,jar:resource:/chrome/linktoolbar.jar!/content/en-us/linktoolb ar/ Does that help?
Just a quick warning: we backed out the wallet toolbar because it would appear and disappear which drove people nuts. If you support autoshow (it's not autohiding, since the functionality is that it shows up whenever it feels like annoying users, not disappears as a convenience to users), you can not use that as your default. I would suggest having another discussion specifically about this, but you will need to run a user study on it. The bug for wallet should be easy to find.
The current toolbar does not handle multiple links for Top, Next, etc. or contents. Also, the title doesn't show up for unrecognized link types.
I've fixed the title for unrecognized link types and added some other improvements. I intentionally didn't support multiple links for certain link types. Currently the last occurance of the same link type is used. I've changed that to use the first occurance instead. Somewhat related, links that have the same REL, HREF, MEDIA, and HREFLANG are treated properly, i.e. only one will be used. They can differ by title, which is intentional. Only the first link encountered will be used. Likewise, in most cases there is only one Table of Contents, Index, and Glossary. So I have changed Indices and Glossaries to Index and Glossary respectively and made them menuitems instead of menus. I mentioned in bug 2800 that I made it easy to switch the toolbar items between button, menuitem, and menu. A planned improvement will make it even simpler...just change the XUL. In other words, maybe we can defer the debate on this until the UI reviewers come around, which will be after we think the toolbar is working good enough. Tooltips are working again. I broke them when I did my rewrite. I'll attach a new JAR or XPI with these and other changes within 24 hours.
Attached file Tim's revised XPI (changes, bugs below) (deleted) —
OK, there's my latest XPI. Either wait until Eric incorporates all or some of the changes on his Link Toolbar page or read the previous discussion on how to install this XPI from the attachment (just clicking on the attachment may work for you). I also added a patch of the changes I made. The diff was made against the contents of linktoolbar.jar in the previously attached XPI, the one labeled "Same as above, only doesn't assume /bin directory (works on Mac)". Here's the command I used to make the patch $ diff -ruN linktoolbar-old linktoolbar On your end either 'patch <linktoolbar.patch' or 'patch -p1 <linktoolbar.patch' should work depending on whether you extracted the contents of linktoolbar.jar into a directory called linktoolbar. Changes: * skin improvements: - fixed Modern horkage, but see bugs below - removed most hard coded styles and use XUL classes properly - toolbar items look like they do in personal toolbar - More menu looks like a bookmark folder - created disabled versions of home, bookmark-folder and bookmark-item icons * handle equivalent links as duplicates. Links are equivalent if they match on REL, HREF, MEDIA, and HREFLANG attributes * tooltips work again and labels in the Miscellaneous menu should work properly now * added toolbar separators, but see bugs * the More menu is disabled if it contains no active items. * added Search menuitem to More menu. Uses REL="Search". * renamed: Authors => Author Contents => Table of Contents SubSections => Subsections Indices => Index (menuitem instead of menu) Glossaries => Glossary (ditto) Alternates => Other Versions The idea is that plural nouns are for menus and singular nouns for menuitems. Known bugs: * Modern theme uses Classic skin for linktoolbar. Help needed. * toolbar separators displayed without vertical bar (border). Help needed. * toolbar only gets updated when the document has completely loaded. It should update incrementally as the document is loaded. * browser is unresponsive for 2 seconds or more while updating the toolbar on the GNU make Manual testcase * HTTP LINK header ignored * Drag & Drop should work like the Personal Toolbar
I've attached a document which classifies the various types of links we should support, and what they do. Tim, is there any possibility of your adding: 1) autogenerating toolbar initialization from XUL 2) Changing the parsing so that if you have one "contents"/"toc" link, say, you get a single menuitem, but if you have multiple items, the menu cascades? If not, I'd be willing to work on both of these changes, but I don't have a CVS installation I can use right now and can't generate diffs, so I'm trying to manually avoid conflicts with other people working on the code.
Am I the only one to find the various "standards" confusing, ambiguous, and sometimes contradictory on the meanings of the link types? Of course, the only standard to date is the HTML recommendation. The rest are just drafts or people's recommendations. We're at least in agreement on the navigation UI: [site homepage], Up, First, Previous, Next, Last. But the specs offer little help for mapping REL values to these. I'm frustrated because the most important type -- a link to the current site's homepage -- has /no/ clear and obvious corresponding REL value. The obvious choice, Home, is deprecated. The next likely candidate, Start, suffers from the vagueness of W3C-speak. Falling back to the IETF draft we learn that Start is overloaded to mean either the site homepage *or* as a synonym of First. Ditto that for Top. I suggest we build a Link Toolbar that makes sense to the user. The goal is to make it easier to navigate. We should be careful to comply with the HTML 4.01 recommendation and should avoid behaving badly with the pages of early adopters of LINK types. We can use the other drafts and recommendations for guidance /when/ they actually provide guidance. That said, thank you, Chris, for putting together that summary on link types. Now we need an unambigous mapping of REL attribute values to our link types. I'll kick start it by asking someone to suggest the proper newsgroup for this discussion, and by providing the current implementation: switch (relAttribute) { case "home": case "start": case "top": case "origin": return "top"; case "up": case "parent": return "up"; case "begin": case "first": return "first"; case "next": case "child": return "next"; case "prev": case "previous": return "prev"; case "end": case "last": return "last"; case "author": case "made": return "author"; case "contents": case "toc": return "toc"; default: return relAttribute; }
> Tim, is there any possibility of your adding: > 1) autogenerating toolbar initialization from XUL Yes. > 2) Changing the parsing so that if you have one "contents"/"toc" link, > say, you get a single menuitem, but if you have multiple items, the > menu cascades? I would rather not, because I'm opposed to that feature. I explain why in n.p.m.ui. I believe Eric liked the idea or, as you say, you might do it yourself. > [...] I don't have a CVS installation I can use right now and can't > generate diffs, so I'm trying to manually avoid conflicts with > other people working on the code I know what you mean. However, you don't need CVS to make a patch. Assuming you have the standard GNU tools just do: $ diff -u old-file new-file >file.patch # or $ diff -ruN old-directory new-directory >file.patch
If you care to study my now-nine-months-old working implementation of the link toolbar: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=14701 you will see just such a switch statement, which I hope may be of some guidance in the new work.
Do I alone notice the irony that this work is nearing completion just in time to miss another Netscape release?
> Am I the only one to find the various "standards" confusing, ambiguous, and > sometimes contradictory on the meanings of the link types? No. I just added LINK support to Bugzilla (bug 87818) and couldn't work out whether to have returning to the Buglist as "Home", "Contents" or something else. Also, there seems no official value to represent "Last" in the HTML 4 spec, as Christopher Hoess points out in his spec. > I'm frustrated because the most important type -- a link to the current > site's homepage -- has /no/ clear and obvious corresponding REL value. I suggest we recognise "Root" or "Origin", which I believe the Links draft suggests. But yes, this is a horrible situation, and needs to be addressed in the draft. Who is drafting it? Gerv
At http://www.w3.org/TR/REC-CSS2/about.html , the "More" menu is selectable, with the sub-options "Table of Contents", "Index", and the "Miscellaneous" menu not disabled. But there doesn't seem to be a way of seeing what is in the Miscellaneous menu, nothing pops up when I mouseover it. Using the latest attachment with Mozilla 0.9.1.
Hmm...at <http://www.w3.org/TR/REC-CSS2/about.html> I see the following in the Misc menu: CSS-properties: properties biblioentry: HTML40 Which reminds me that I need to go find Chris' code that hid certain types (CSS-properties shouldn't show up). Anyhow, it works for me. Can you check your JavaScript console (Tasks -> Tools -> JavaScript Console) for error messages coming from linkToolbarOverlay.js?
Actually this CSS-Properties is the pointer to an additional index (of CCS properties). So it should show up under Misc. I would also vote for displaying multiple index, toc etc entries in some low level menu instead of displaying only the first (or last). So that it would make sense to change the W3C site to use a second rel=index with a TITLE "CSS-Properties".
Okay, so sometimes the Misc menu works (on the CSS2 page), sometimes it doesn't. When it doesn't I get no visual feedback from the More menu at all, i.e. when I scroll down the menu, the menu item I am over does not get highlighted. This seems to be different in the first window I open from other windows (I think). So perhaps it's an XUL bug? Also, when I hover over a button which is disabled (eg First, Previous...) I get a "hand" cursor (the same cursor as for a link). This should be the normal cursor.
> So perhaps it's an XUL bug? I wouldn't totally discount that I may be doing something wrong. I assume those times when it doesn't work, there's no messages in the JavaScript Console or you would have said something about them, right? ;-) > Also, when I hover over a button which is disabled...this should be the > normal cursor. This is fixed in the patch I just attached for localization support.
Given many feel the specs are ambiguous, I'd say "ask the w3c working group". Now I've heard (as in, I don't _know_ but I've been told) that the HTML working group is a relatively closed list. This would suggest someone with membership needs to raise the issue. Who at Netscape has HTML working group membership? Are they cc:'d on this bug? Can they find the time to help? You should try to touch base with them to make sure they know about the problems, and to make sure if its going to be addressed in some future XHTML draft we remain as forward compatible as possible. Speaking of which, if one is going to introduce terms for REL types, consider using the -moz- (-moz-foobar) style used in CSS for vendor specific extensions?
Depends on: 87818
No longer depends on: 87818
Blocks: 87818
No longer blocks: 87818
*** Bug 57399 has been marked as a duplicate of this bug. ***
Blocks: 57399
Gervase, please don't try and make support for REL on anchor elements an essential part of this bug. We already have enough on our plate. Supporting anchors is not without its problems or opponents. It was correctly a seperate enhancement, bug 57399.
No longer blocks: 57399
Tim, That bug was duped against this one because this bug fixes that one. mpt (important UI design guy) says, over in the other bug: "So could this bug be fixed just by treating A REL/REV in the same way we treat LINK REL/REV -- showing those links in the Links Bar, in addition to their usual rendering?" the answer to which is blatantly "yes". The current code does it, and the only issue you have raised with it is a performance one. There is a comment in the newsgroup which, if implemented, will double the speed straight off the bat - and I'm sure that's only the start. If you have conceptual problems with this, then again they need to be hashed out in the newsgroup - but I just can't see why you want to differentiate between <A rel="next" href="next.html">Next</a> and <LINK rel="next" href="next.html" />. These should surely trigger the same browser navigation UI. If drbrain, to whom this bug is assigned, rips <A> support out of the links toolbar, we'll reopen the other bug. Gerv
Following up to various commentary: CSS-Properties should indeed be shown; why they aren't using rel="appendix" for it, I don't know, but the w3c pages do seem to have a few link types that have never appeared in any specification or draft. Re: clarification from the HTML working group: we could ask the question on www- html as well, which some members of the WG read. Both Hixie and dbaron are W3C members and could bring it to the WG for us; I'd suggest Hixie, as he seems to be following the newsgroup discussion. Perhaps we should even consider writing our own draft of what the various values should indicate and see if it could get published as a W3C Note. (Hixie would know more about the feasibility of this.) Any such draft would have to allow leeway for continued extension: people will always be using "rel" values that we don't recognize for human-readable documents, and we'll have to continue to support this. Given that even the W3C seems to feel comfortable making up new values, I don't think we really need to worry about marking values as vendor-defined. I still think that we should not be ignoring "rel" values in the toolbar just because they're in anchors rather than links. I see this toolbar as being a navigation instrument for the entire document, and rel="index" is rel="index", regardless of where it is located in the document. Given Tim T.'s example (in the NG) of a Slashdot page where this significantly affects page load, I do agree that we need some performance help. Our first step should be to make the suggested change to the scope of the getElementByTagName calls (see NG) so that links are only searched for in the head and anchors only in the body; once we see what the performance is for that case, we can discuss other options (DOM mutation, XBL, as discussed in NG.)
> There is a comment in the newsgroup which, if implemented, will double the > speed straight off the bat No, it doesn't. Here's a benchmark: document.getElementsByTagName("LINK") document.getElementsByTagName("A") LINK | A ---------------- 97 ms | 1340 ms 95 ms | 1329 ms 116 ms | 1366 ms 96 ms | 1353 ms 96 ms | 1378 ms HEAD.getElementsByTagName("LINK") BODY.getElementsByTagName("A") LINK | A ---------------- 49 ms | 1339 ms 49 ms | 1388 ms 49 ms | 1359 ms 48 ms | 1357 ms 69 ms | 1322 ms So it retrieves LINK elements on average 50 milliseconds faster. That's good, and it's the correct behavior. I'll switch over to using the correct approach for both link and anchor elements. Here's the sourcecode for the test so you can verify the results: function testRefresh() { profile("iterateLinkElements", iterateLinkElements); profile("iterateAnchorElements", iterateAnchorElements); } function iterateLinkElements() { var document = window._content.document; //var nodeList = document.getElementsByTagName("LINK"); var node = document.firstChild.firstChild; // document -> HTML while (node.tagName != "HEAD") { node = node.nextSibling; } var nodeList = node.getElementsByTagName("LINK"); iterateNodeList(nodeList); } function iterateAnchorElements() { var document = window._content.document; //var nodeList = document.getElementsByTagName("A"); var node = document.firstChild.firstChild; // document -> HTML while (node.tagName != "BODY") { node = node.nextSibling; } var nodeList = node.getElementsByTagName("A"); iterateNodeList(nodeList); } function iterateNodeList(nodeList) { for (var i = 0; i < nodeList.length; i++) nodeList.item(i); } function profile(name, func) { ddump(name + " begin"); var start = new Date(); func(); var finish = new Date(); ddump(name + " end"); ddump(name + ": completed in " + (finish.getTime() - start.getTime()) + " ms")} function ddump(msg) { dump("LinkToolbar::" + msg + "\n"); } To run the benchmark yourself: 1. add the above code to linkToolbarOverlay.js 2. modify linkToolbarOverlay.xul to call testRefresh() instead of refresh() 3. comment/uncomment the relevant lines for building the nodeList using either document.getElementsByTagName or node.getElementsByTagName At this point I'm satisfied there isn't much performance tuning we can do from the JavaScript side of things. Feel free to provide working code that proves me wrong :) I did some research last night looking for ways to improve performance using available APIs. One candidate is the DOM2 Traversal API, specifically NodeIterator with a NodeFilter. Another is the DOM2 Event API, specifically support for the NodeInsertedIntoDocument Mutation Event. Neither of these are working yet. You can read more on the mozilla DOM pages: <http://www.mozilla.org/docs/dom/> The mozilla extension getElementsByAttribute doesn't exist on the content document DOM implementation, only the XUL document DOM implementation.
I won't be able to get to this for a few days... but when I do, I'll need to know which page you used for testing :-) Gerv
Changing title of this bug. Tim's enhancement of adding support for rel= on anchor elements as well as title elements is a worthwhile and useful enhancement and kills two bugs with one patch. We should not waste time building a second UI for these elements, they are highly useful and important to increasing the meaning of links between documents. Why are we not using document.links? This is a collection of all anchor and area elements in the document, and if someone accidentally puts a rel= on an area link all the better (not having rel= on area elements is an oversight in HTML4, IMHO. If we want to be semantic we can add a test.)
Summary: No UI for HTML2 LINK element → No UI HTML "rel=" attribute
Why on earth do we want to go through all the <a> elements as well??? That's what the links sidebar is for. Let's not get carried away here. As the original reporter, I'm putting this back to what I originally asked for, wrote a spec for, and got discussion about. If we want to add <a rel=> (or <a rev=>, <area>, xlink:role, etc.) support then that should be an enhancement added AFTER this bug is fixed. Don't succumb to feature creep!
Summary: No UI HTML "rel=" attribute → No UI HTML <link> element
Regarding the rel attributes -- don't expect the HTML WG to give you anything useful to go on. Do what you think is best. Of course, if you really want me to ask, send me the exact question I should ask and I'll look into it...
Gervase, I got those times running the benchmark against the GNU make manual testcase. However, you can use any heavily linked document you want, because there's no point in comparing your times to my times, unless you want to see who has the faster computer :) Eric Hodel wrote: > Tim's enhancement of adding support for rel= on anchor elements Actually, it was Chris Hoess' enhancement, I just carried it over in my rewrite because I thought it was neat.
> Why on earth do we want to go through all the <a> elements as well??? > That's what the links sidebar is for. So what you are saying is that no-one will use <A rel="foo">, because the Links sidebar isn't in default Mozilla. Are you really saying that <A rel="Next" href="next.html">Next</a> should have different UI for the rel attribute to <LINK rel="Next" href="next.html/> ? > If we want to add <a rel=> (or <a rev=>, <area>, xlink:role, etc.) support > then that should be an enhancement added AFTER this bug is fixed. Why, if the support is there now? Removing it would be more complex than leaving it in! Gerv
Oh, come on. Disabling REL on anchors is as "complex" as commenting out one line.
Ian actually managed to convince me last night on IRC that there's a reason to have different UI for <link rel="foo"> and <a rel="foo">. The first is a page link; the relationship denoted in the link element applies for the whole page. The second is a context link; the relationship denoted applies to the immediate context of the anchor element. Looking carefully at the W3C's homepage, it appears that they're actually making this distinction. If you look at the links brought up by the toolbar (current version, both a and link) on that page, the "Miscellaneous" section has a bunch of links marked "details: news archive" and one "details: press release". Examining the source, these are attached to the anchors at the end of each of the little blurbs on a W3C announcement. It seems more clear if we assume that each of these "details" relationships applies to the little blurb of text rather than the entire page-it doesn't make sense to have five links into different points of the news archive if it's "details" for the entire page, but it is meaningful when viewed in context. I agree that I'd like a better UI than the Links Sidebar. (No insult meant to Eric's design, but I think a lot of people will turn off the sidebar to save screen space and never see anything that appears there.) However, thanks to Hixie's evangelism, I do feel that that is better brainstormed out in bug 57399. Anyway, the matter of turning the code on/off is, as Tim T. pointed out, quite simple, so enabling or disabling it is no big deal.
OK, if in an ideal world, <A rel="foo"> is different to <LINK rel="foo"> then we need to ask ourselves the following questions: 1) Is no UI better than the current UI? That is, is having these links "unflagged", as it were, better than having them in the Misc menu of the Links Toolbar? 2) If different UI is planned, what would it look like and how on earth can we implement it (particularly in the case of the example given)? 3) Why don't the W3C say in what context they expect the two sorts to be used? ;-) It very much affects the sort of UI you are providing, as we have found out. Gerv
Just to clarify, the spec says <link> elements are document links, whereas <a> links are links from certain contextually-sensitive parts of the document. Nowhere does it suggest that <a rel="next"> should be taken and used in some other part of the UI, and in fact I see no reason to do that. There are probably logical things we can do with rel="" links on <a>, but IMHO putting them on the <link> toolbar is not one of them.
There's already a UI for <a rel="foo">: all those elements appear in the page, underlined, in blue type. There is NO currently visible UI for the <link> element because it doesn't correspond to anything in the content.
Just noticed that, as it is (I'm using the version from attachment 40168 [details], "Tim's Revised XPI") only works when the page is served as text/html. If you have XHTML and serve it as text/xml, the toolbar doesn't pick anything up (I'd guess it isn't even activated).
Attached file XHTML w/ LINK elements testcase (deleted) —
That testcase is invalid, Mozilla is behaving exactly as it is supposed to. If you want to serve XHTML as XML and get Mozilla to treat an XHTML file like an HTML file, you need to use the MIME type "application/xhtml+xml"
Attached file Hot XPI Action !!! (deleted) —
Latest XPI with lots-o-good-stuff. Grab it. Love it. Hate it. Changes: * incorporated Neil's Classic icons [Eric] * renamed Author => Authors [Eric] * generate link handling objects automatically from XUL, i.e. no more code like: registerItemForType(new LinkToolbarButton("next")) * Miscellaneous menu removed. Unknown link types appear below the More menu in a submenu for that link type. * refreshing is avoided when the toolbar is hidden via "View -> Show/Hide -> Link Toolbar" (but see bugs below) * minor refactorings to improve readability and reduce duplication * added a few icons for Modern theme (but see bugs below) * added testRefresh() and related functions for performance testing Known bugs: * attempting to switch themes causes the "You have selected a theme that was designed for an earlier version of Mozilla..." error dialog, no theme change * lack First icons, and hover and active icons in Classic * lack appropriate icons in Modern * toolbar is refreshed when collapsed via the twisty. Should avoid refresh when collapsed, then perform an immediate refresh when exposed just like for hiding the toolbar * toolbar separators displayed without vertical bar (border). Help needed. * disabled toolbar items shift (probably border, margin, or padding) * toolbar only gets updated when the document has completely loaded. It should update incrementally as the document is loaded. * browser is unresponsive for 2 seconds or more while updating the toolbar on the GNU make Manual testcase * HTTP LINK header ignored * Drag & Drop should work like the Personal Toolbar Regarding appending unknown link types to the More menu: To preempt the "bug" reports, I intentionally didn't implement the "switcheroo" menu/menuitem as described by fantasai and others. That is, append a menuitem to the More menu for link types that have only one link, and a menu for link types with more than one link. If you disagree agree, feel free to duke it out wherever you think is appropriate, but consider saving the debate till this bug is resolved and you can submit your preferred implementation as an enhancement. If you need to vent, send me a private email full of flamage...I am full of love. ;-) Hackers: The patch requires source from the previous XPI + the patch to get localization working. If you're patching, you'll also need to grab the XPI to get the new images. FYI, I'm about to move a bunch of functions around in linkToolbarOverlay.js to make the code easier to follow. I avoided doing it this time around since such changes make it difficult to tell what's changed when using patch files. If you're doing something that you think might cause big collisions with my changes, drop me an email.
Regarding icons for Modern theme: Chris Hoess made a complete set of icons (normal, disabled, hover) from the original icons I scrounged together. In the Classic skin these have been replaced with Neil's icons. Someone (Chris?) made a few Modern icons, but until Modern has a cohesive set, the icons Chris made are more than adequate. Unless someone comes forward with other icons soon, I'll grab Chris' from an older JAR and incorporate them into the Modern skin.
Thanks for the correction on the MIME type. I fixed my script, noticed it didn't help at all, and then changed my local server to use application/xhtml+xml for .xhtml and saw that my test case still doesn't trigger the toolbar. This is using the newer, "Hot XPI Action!" version of the toolbar. I've created a new attachment which is identical except for the mime type (which may have been a bit overkill, but at least it's small).
The link toolbar isn't showing up because getElementsByTagName("HEAD") is returning null. Changing to getElementsByTagName("head") and getElementsByTagName("link") the link toolbar shows up for the XHTML test case and all other test cases. My explanation is that HTML is case insensitive, whereas XML is case sensitive. Using lowercase "head" and "link" will work because XHTML requires lowercase tag names.
Review This is extremely well-written _code_, and the current toolbar looks very good. A couple of things: Comments. Can we have some, please? :-) Documentation is definitely one of the 1.0 criteria, and a new person coming to this code has no help in understanding it. Each function should say what it does, at the very least, and there should be a block at the top explaining how it all fits together. This is the one glaring problem with it. The rest is nits. // FIXME: we really want to use this: // <snip> It should say in the comments why you can't. If the function doesn't, and will never exist, stop saying it. If there's a bug number, quote it. function getHeadElement() { return window._content.document.getElementsByTagName("HEAD").item(0); } The point of getting the HEAD is to avoid searching the entire document tree. If you search the entire tree to get the HEAD, it rather defeats the object. If there is no document.head object or somesuch, try writing a small bit of code that searches the immediate children of the <HTML> element (which according to your code, is document.firstChild.firstChild. Like in your testIterateAnchorElements function. How much of the CSS file is pinched from other CSS files? If we are using a lot of the same CSS as the Personal Toolbar we should be including its CSS files to avoid duplication, and only putting stuff unique to us in our CSS file. Can it be made to install somewhere more sensible (chrome/content/navigator and parallel friends) or would it need to be part of the main build for that? I am going to keep making the point about making unused items on the More menu disappear, of course. And the one about not having single-item submenus (which is now an issue with the new unrecognised types.) :-) On a related note, is anyone seeing what is probably a XUL bug, where when you open the Sections menu on the Make testcase, which has too many items to fit and so requires menu scroll arrows, the root menu also acquires scroll arrows which don't go away until you reload? For those who want to test a multipage version of the GNU make manual, it's here: http://www.gnu.org/manual/make/html_node/make_toc.html Fix the above, and you will find it easy to acquire r=gerv ;-) Gerv
gervase.markham@univ.ox.ac.uk wrote: > This is extremely well-written _code_, and the current toolbar looks very good. Thanks :) > Comments. Can we have some, please? :-) If what you just said above is correct, then it shouldn't need comments...at least not many. I'm from the XP/Refactoring/Wiki crowd. We tend to TreatCommentsWithSuspicion: <http://c2.com/cgi/wiki?TreatCommentsWithSuspicion> The most sinister of them all are comments that merely say what the function does. These add no value. If there's a section of code that you find difficult to understand, let me know so I can refactor it to make it clearer. Or better yet, refactor it yourself :) > // FIXME: we really want to use this: > // <snip> > It should say in the comments why you can't. If the function > doesn't, and will never exist, stop saying it. If there's a > bug number, quote it. You'll have to give me a specific example and explain your problem with that specific example, because I don't understand what you're trying to say. > function getHeadElement() { > return window._content.document.getElementsByTagName("HEAD").item(0); > } > The point of getting the HEAD is to avoid searching the entire document tree. If > you search the entire tree to get the HEAD, it rather defeats the object. If > there is no document.head object or somesuch, try writing a small bit of code > that searches the immediate children of the <HTML> element (which according to > your code, is document.firstChild.firstChild. Like in your > testIterateAnchorElements function. I appreciate your good intentions, but before you make comments about code performance, please do some profiling yourself. Your comment about getElementsByTagName searching the entire document to retrieve the first occurance is wrong. At first, I did implement a specialized method that walked the document tree. Problem is, I'm not sure you can make the assumption that HEAD is always document.firstChild.firstChild. A comment or processing instruction could be the docment.firstChild or HTML.firstChild. Using getElementsByTagName("head").item(0) will always work (mozilla synthesizes the HEAD element if it doesn't exist) and it only adds about 50ms to the call. In fact, as I started handling the special cases in the tree walker version, it became apparent that there might not be a performance benefit at all. > How much of the CSS file is pinched from other CSS files? The majority of the CSS is for specifying the button icons. A few attachments ago I removed most of the copy-paste-reuse in the CSS and added appropriate CLASS attributes in the XUL. What does remain are styles for disabled bookmarks, because until now there never were any. These have to get approved, because they specifiy color and border properties, which is verboten in derived CSS files (more on derived CSS files below). FYI, there /is/ a comment in the CSS explaining this :) I imagine that at some time in the future the disabled bookmark styles should just get folded into skin/[classic|modern]/communicator/bookmarks/bookmarksToolbar.css. > If we are using a lot of the same CSS as the Personal Toolbar we > should be including its CSS files to avoid duplication, and only > putting stuff unique to us in our CSS file. Nope, definately *must not* do that...here's why: "Your CSS file should never import another skin file if your file is loaded by a XUL overlay" -- <http://www.mozilla.org/xpfe/skins.html> When I came on board, that actually was being done. As I learned more about XUL, I learned it was a no-no. When I removed it, suddenly the Modern horkage was fixed. Funny that :) The correct way to do it is to put the proper CLASS attributes in the XUL overlay, which I did. An overlay inherits all the styles for that which it is overlaying. > Can it be made to install somewhere more sensible (chrome/content/navigator > and parallel friends) or would it need to be part of the main build for > that? I have no idea how the review/acceptance process actually works. I've been imagining the following scenario: 1. develop seperate XPI until complete 2. have UI review. If accepted, proceed to 3., else go back to 1. 3. submit a patch to the main build tree that incorporates the link toolbar into the base chrome packages 4. patch review. If accepted, proceed to 5., else go back to 3. 5. done Someone please post how it really works, and who the reviewer(s) will be. I've been expecting it to be r=hixie, r=mpt, the maintainer(s) of the chrome packages, etc.. Once we add support for LINK HTTP (and META http-equiv) headers, I think it will be Good Enough(TM). Oh, and consistent Modern icons. > I am going to keep making the point about making unused items on the More > menu disappear, of course. And the one about not having single-item > submenus (which is now an issue with the new unrecognised types.) :-) You're free to keep making your point, so long as I'm free to continue ignoring it :) > On a related note, is anyone seeing what is probably a XUL bug, where > when you open the Sections menu on the Make testcase, which has too > many items to fit and so requires menu scroll arrows, the root menu > also acquires scroll arrows which don't go away until you reload? Yes. You can reproduce it by filling up a submenu in a top level personal toolbar bookmark folder with enough bookmarks, i.e. Personal Toolbar: Home Bookmarks | + my-folder + subfolder bookmark1 bookmark2 ... bookmarkN Knowing that it wasn't the link toolbar was good enough for me, so I stopped there and didn't search/file a bug. I know this makes me a bad citizen :)
Attached patch patch to above XPI to fix XHTML bug (deleted) — — Splinter Review
> If what you just said above is correct, then it shouldn't need comments...at > least not many. You do not comment to make unclear code more clear, this is true. You comment so someone who wanted to make a small addition to the Links toolbar (e.g. to make it support <A rel="foo"> ;-) can come along and work out what's going on quickly. It's a large file and it took me 15 minutes just to work out how it fitted together. Multiply that by the number of people who may one day need to understand it. I can't believe I'm having to justify comments. Are you seriously suggesting that uncommented code is in some way clearer than commented code? > You'll have to give me a specific example and explain your problem with that > specific example, because I don't understand what you're trying to say. I'm referring to a particular comment in the code. Read the comment, and what I say will become clear :-) > Your comment about > getElementsByTagName searching the entire document to retrieve the first > occurance is wrong. Why? As the code is written, it will do a getElementsByTagName() on the whole document, because it can't know if you want the first, the second, the xth or the forty-second. Then, you ask for the first one, and it gives it to you and throws the rest away. Either that, or there is some way for getElementsByTagName() to know, when it's being called, that subsequently you are going to only ask for the first one. Which would make it telepathic. There may or may not be a measurable performance benefit; but I stand by my assertion that it does check the whole document. > Someone please post how it really works, and who the reviewer(s) will be. It could work that way if you wanted it to. In this case, UI review can happen now. Summon mpt! As for who reviews it, the answer is "whoever's nearest". I've started already; but you are free to ask anyone you want, and anyone is free to review it and make comments, if you see what I mean. > Once we add support for LINK HTTP (and META http-equiv) headers, I think it > will be Good Enough(TM). Oh, and consistent Modern icons. I agree. In fact, it's Good Enough(TM) now if you fix the icons. META should be easy, right? head.getElementsByTagName("meta") etc. One last point - currently you merely lookup language tags in your dictionary, instead of implementing Hixie's hreflang spec (see the URL box) which copes with x-minbari-warrior-caste and fr-CH-slang or whatever. I've implemented code that does this for the Properties (metadata) window, in metadata.js. Feel free to leverage it. Gerv
> You do not comment to make unclear code more clear, this is true. You comment so > someone who wanted to make a small addition to the Links toolbar (e.g. to make > it support <A rel="foo"> ;-) can come along and work out what's going on > quickly. It's a large file and it took me 15 minutes just to work out how it > fitted together. Yes, I'm aware of that. See the recent patch and the paragraph above labeled "Hackers:" from a few comments ago. Breaking up linkToolbarOverlay.js into a few files and moving around some functions should go a long way towards making it easier to digest. > Are you seriously suggesting > that uncommented code is in some way clearer than commented code? Yes. > Why? As the code is written, it will do a getElementsByTagName() on the whole > document, because it can't know if you want the first, the second, the xth or > the forty-second. Then, you ask for the first one, and it gives it to you and > throws the rest away. > > Either that, or there is some way for getElementsByTagName() to know, when it's > being called, that subsequently you are going to only ask for the first one. > Which would make it telepathic. > > There may or may not be a measurable performance benefit; but I stand by my > assertion that it does check the whole document. getElementsByTagName returns a NodeList. Unlike returning an Array, there's no reason that the NodeList returned has to contain all the matched elements in some internal data structure. Instead, it could defer scanning the document until you start requesting nodes. When you request a node via NodeList.item(n) it only needs to scan the document upto and including the n-th occurance of the matched element. Only something like NodeList.length would require a full scan of the document [1]. I'm fairly certain that the NodeList is implemented using an optimization similar to what I described above. That would explain why I observed little overhead for getElementsByTagName("head").item(0)...no telepathy required. [1] or something like NodeList.item(n), where n >= NodeList.length
>On a related note, is anyone seeing what is probably a XUL bug, where when >you open the Sections menu on the Make testcase, which has too many items to >fit and so requires menu scroll arrows, the root menu also acquires scroll >arrows which don't go away until you reload? Yes, I see that on Windows 2000 with the Bookmarks menu, my Imported IE favourites is longer than the screen and it does what you describe. Very likely a XUL bug.
Attached file All Your XPI Are Belong To Us (deleted) —
Release Every ZIG !! I brought back the HTTP header parsing code originally written by Eric, Chris, Jeffrey Baker, or someone else. It works for META http-equiv="link", but not for real HTTP headers. Getting access to the HTTP headers require some XPConnect voodoo. Therefore getting real LINK headers to work will require one or more of: a) an "aha" moment on my part b) someone responding to my n.p.m.xpcom post with example code c) someone else diving in and making linktoolbar/content/linktoolbar/http.js work for HTTP headers Not handling HTTP headers is a blocker. Not being able to switch themes after linktoolbar.xpi is installed is a blocker. The "2 second delay" bug /isn't/ a blocker, because that's only for handling anchors in BODY...a feature that will eventually be disabled. The remaining bugs aren't blockers...though it would be nice to have icons for First in Classic (Neil, are you there? :-) ). Changes: * META http-equiv="link" works (but real HTTP headers still don't) * Modern has cohesive set of icons * toolbar seperators display properly Known bugs: * attempting to switch themes causes the "You have selected a theme that was designed for an earlier version of Mozilla..." error dialog, no theme change * HTTP LINK header ignored * language name lookup doesn't use hixie's algorithm for non-standard languages * lack First icons, and hover and active icons in Classic * toolbar is refreshed when collapsed via the twisty. Should avoid refresh when collapsed, then perform an immediate refresh when exposed just like for hiding the toolbar * disabled toolbar items shift around in Classic (probably border, margin, or padding) * toolbar only gets updated when the document has completely loaded. It should update incrementally as the document is loaded. * browser is unresponsive for 2 seconds or more while updating the toolbar on the GNU make Manual testcase * Drag & Drop should work like the Personal Toolbar
> Not handling HTTP headers is a blocker. I disagree. It's an RFE on the current code. It shouldn't stop this getting checked in. > Not being able to switch themes after > linktoolbar.xpi is installed is a blocker. I wouldn't worry too much; that may well go away when it's no longer an XPI. :-) > though it would be > nice to have icons for First in Classic (Neil, are you there? :-) ). I'd say that this _is_ a blocker; I doubt you'll pass UI review without the UI being all there. Speaking of which, I'll ping mpt. Gerv
>> Not handling HTTP headers is a blocker. >I disagree. It's an RFE on the current code. It shouldn't stop this getting >checked in. I'm with Gerv. Can anyone point to a site that uses links via HTTP headers? I don't think it is a blocker. Nevertheless I'll be happy to take a crack at updating the header parsing code from lo these many months ago, and post here if I have success.
the bug lordpixel and others mentioned is filed and should be fixed eventually, if you need help finding it, ask on #mozillazine. This bug has to be one of the fastest growing and most actively developed bugs this week. i haven't checked the classic icons lately but i'd expect the first/last buttons to resemble mplayer.exe/mplay32.exe's icons. [don't confuse that app w/ mediaplayer2/wmp (mplayer2) or activemovie(amovie)] As for menu depth, 2 levels is perfectly acceptable (the menubar is not a level). Having a combined menu used to be acceptable but we're moving away from that -- actually we're both moving away from and towards that. people are trying to combine Edit+Search while they are trying to split view>apply {foo}. And people are working to split Window-list from Tasks (yes that's right we're growing menu length, shrinking menu length, increasing top level menu count and decreasing the same). Personally I would prefer a shorter view menu (view is like the context menu, it's getting huge), which means combining view>select {foo}> and view>select {bar}> into view>select bazs>{foo-list}|{bar-list}. But i haven't provided code so my voice shouldn't count for much.
If classic icons are a blocker, I will change classic to use the same icons as modern (except for the More menu). Debating whether HTTP headers is a blocker will be kind of moot if Jeff resolves it. Do we have someone capable of incorporating the linktoolbar into the main chrome? I have a CVS build tree, but my build alway segfaults (probably my system libraries are out of whack), making it difficult to test my changes. Could someone add the "review" keyword?
I don't see how we can get a handle on the current HTTP channel. It used to be that navigator.js had a reference to it, and it got passed to us. We could implement nsIHttpNotify in js or cpp, but I'm not up for it right now. This is how the cookie permission prompter works.
I'll see if I can wrap my head around nsIHttpNotify.
Attached file The 6 missing icons (deleted) —
I don't think the "first" icon is missing in the classic skin. I think its just named start.gif and start-disabled.gif instead (leastways, it is in that last XPI). That means the missing icons for Classic are the 6 "hover" icons, and modern is missing bookmark-icon-disabled.gif. I attached my attempt at doing those icons in the appropriate style. (its a zip file containing 7 gifs). As far as licensing goes - any IP _I've_ created w.r.t. to these 7 icons I place in the public domain. Feel free to copy and relicense as needed. Anything anyone else owns remains theirs, naturally.
Excellent. Thanks for making the rest of the icons, and for locating the First icons...ugh...me dumb. As for Modern, there actually is a disabled bookmark item icon (called bookmark-item-dis.gif) that I've been using. I don't know what it was used for upto now. Hope you don't mind if I leave it unchanged. I'll toss your version into the linktoolbar modern skin folder just in case the "official" one disappears. I should have a new XPI with these icons and a some other changes (sorry, no HTTP headers) in a couple of hours.
Attached file I am full of XPI (deleted) —
The good news: this XPI is the first I feel is Good Enough (TM). I wimped out on HTTP headers and noone came to my rescue on n.p.m.xpcom (thanks for giving it a look, though, Jeff). I'm in agreement that HTTP header support can be a seperate bug. The bad news: this is the last XPI you'll be seeing from me for a week or so. OK, maybe that's good news for some :) I'll be on vacation Jul 4-8 and I need to take care of "life stuff" in the intervening time. I'll still be checking how things are going till around Tue, Jul 3. Now's your chance to make all those changes I didn't want to make. Changing toolbar items between button, menu, and menuitem only requires changes to the XUL. Ditto for adding new top level menus. Unknown link types are hard coded to append to the More menu. It would require some additional code to support class="menubutton-dual" i.e. the button with dropdown menu. BTW, you may need to remove chrome/user-skins.rdf inside your mozilla profile to get the "them switching" error to go away. Before reporting that it's still a bug, try installing the XPI into a clean mozilla tree with a fresh profile. Changes: * added lordpixel's hover icons and Neil's rediscovered First button. * fixed "theme switching" error * added "Document" menubutton for ToC through Index * display HREF for tooltip when title absent * disabled code for handling anchors with REL * more refactoring Known bugs: * HTTP LINK header ignored * language name lookup doesn't use hixie's algorithm for non-standard languages * toolbar is refreshed when collapsed via the twisty. Should avoid refresh when collapsed, then perform an immediate refresh when exposed just like for hiding the toolbar * disabled toolbar items shift around in Classic (probably border, margin, or padding) * toolbar only gets updated when the document has completely loaded. It should update incrementally as the document is loaded. * Drag & Drop should work like the Personal Toolbar
erg...it's getting late. That should have been "theme switching" and "clean mozilla install". And that bit about adding in all sorts of features I disagreed with was sarcasm ;>
I tried to get the HTTP Headers via nsIHTTPChannel.idl XPCOM magic some time ago, but I would always crash. I eventually ran out of time trying to fix it. I'll try to resurrect my past work and see if I can get it to run properly this time (I think the problem was Mozilla's not mine). I should have the code buried in my CVS repository... This should be easy to get into a standard location, but not from an XPI. The task should be as simple as: Move the files to the appropriate location, Adjust the paths on the files Add the new files to the JAR Manifests Build and test, fix as appropriate. I'm not sure what is horked with Modern skin switching. I duplicated skin information from modern.jar's contents.rdf, but apprarently I horked it somehow :(
Unless the code you have in CVS is more recent, I already brought back the HTTP code from a previous JAR/XPI. The code I snagged I placed into content/linktoolbar/http.js. I twiddled with it a bit to get META http-equiv working, but it's mostly the same code. The Modern skin switching isn't horked anymore. By comparing it with other files, I found a contents.rdf file that was missing a skinVersion="1" attribute on one of the elements. Adding that attribute fixed it. As I mentioned, I had to install the XPI into a clean mozilla install with a clean profile, but I later found you can also get it fixed by removing ~/.mozilla/[your-profile]/chrome/user-skins.rdf from an existing profile. You can also install the new XPI over an already installed version, but I had to save the file to my harddrive and open the file up with "File -> Open File". Other people mentioned having to do this above to get the XPI to install. Lastly, I forgot to mention the following change that was also done with the latest XPI: * position "View -> Show/Hide -> LinkToolbar" between "Personal Toolbar" and "Status Bar" menuitems Now, back to paying bills :-<
Adding review and patch keyword per Tim's 2001-07-01 11:40 comment to get reviewers attention. However, this is not always enough to get the necessary reviews. (Especially with code from outside Netscape) The owner of this bug and the patch supplier should contact the individuals they would like to review the fix directly as well.
Keywords: patch, review
Thanks. You should probably remove the "patch" keyword. The attached patches are only for changes to the XPI. We haven't yet had a patch that can be applied against the build tree. At this point only a UI review is in order, I guess.
I thought about not putting it in but the description for the patch keyword is "Tracks contributions from the net community. Fixes/patches found by individuals without direct code check in privileges." This patch is from the net community, and the more keywords the better! :-) While the fix is just an XPI, I'd imagine the check-in/longterm solution will be to include the code directly in the build, so better to let everyone (who wants to) know.
Is it possible 'for the rest of us' to install the latest version (on MacOS 9.1) by just clicking on those attached files in a certain row or do we have to apply the diff-patches manually as well (no idea how this had to be done). Can someone make a current full-installer package? For me only the version from <http://segment7.net/mozilla/linktoolbar/index.html> works and AFAIKS this is really outdated ... Greeting, Michi
You know, the question of "what icon to use for Top" would be answered if bug 32087 gets resolved.
I just installed "I am full of XPI". I'm seeing a problem with the table of contents link. http://www.w3.org/TR/html4/ has it on all the pages, and toc shows up, but doesn't move anywhere. Mac 0.9.2.
Never mind. It seems to have fixed itdelf after a restart. I'm doing it with another skin and it still looks good (Given the disabled icons look off.) Great job Tim.
Finally I managed to install "I am full of XPI" on the Mac (short howto: download with another Browser, rename the resulting file 'showattachment.cgi' to 'showattachment.xpi' and drop it into an open Mozilla window). Well done! One bug and one RfE: 1. Please have a look at the 'More' menu on <http://michael.nahrath.de/dnet/>! <link rel="shortcut icon" ...> breaks all following links (shurely this is related to bug 32087). 2. RfE: Using the 'link' element with e-mail adresses (mailto:) is rather common (typically but not exclusively with the 'rev' attribute). It would be very nice if the icon for these links would change to a mail symbol. More ideas for this feature: An icon for ftp: or file downloads in general and an icon for content that is meant to be printed.
Michael Nahrath, many thanks for explainign how to install the last XPI. It finally worked for me too. And it doesn't break Modern 3! Bug report: Is it just me or can anyone else repro: I have the following link elements: <link rel="alternate stylesheet" type="text/css" href="styles/print.css" title="Print" /> <link rev="made" href="mailto:webmaster@domain.com" /> <link rel="start" href="" title="Start Page" /> <link rev="prev" href="" title="Previous Page" /> <link rel="next" href="" title="Next Page" /> <link rel="contents" href="" title="Contents" /> <link rel="copyright" href="" title="Copyright" /> <link rel="section" href="" title="Section" /> <link rel="help" href="" title="Help" /> <link rel="search" href="" title="Search" /> <link rel="alternate" hreflang="de" href="" title="Seite auf D" /> <link rel="alternate" hreflang="fr" href="" title="Page en f" /> <link rel="alternate" hreflang="en" href="" title="Page in E" /> Bug: the "prev" doesn't show.
| <link rev="prev" href="" title="Previous Page" /> | <link rel="next" href="" title="Next Page" /> | | Bug: the "prev" doesn't show. At least it is not applied to the predefined arrow button. This is not a bug but correct behaviour. The 'rev' attribute ist opposed to the 'rel' attribute, but not in the way you seem to have understood it. You want to use <link rel="prev" href="" title="Previous Page" /> in this case as well. This means "the referenced document is related to to this document in the way that it is the previous document to this". The REVerse relationship was "this document is related to the referenced document in the way that it is the previous document to the other". So <link rev="prev" href="" title="Previous Page" /> was aequivalent to <link rel="next" href="" title="Next page" />. You could even combine them: <link rel="next" rev="prev" href="" title="..." /> See http://www.htmlhelp.com/reference/html40/head/link.html for further explanation. P.S.: The programmers intended to use this Bug mainly for technical implementation details, not for UI, styling and authoring issues. Please open a new thread or post to the existing one in the newsgroup netscape.public.mozilla.ui if you would like more discussion upon the rel/rev thing!
Discovered how to install this on a multiuser linux install: Become root and cd to your mozilla install directory, then: export LD_LIBRARY_PATH=`pwd` export MOZILLA_FIVE_HOME=`pwd` cd chrome unzip linktoolbar.xpi (or whatever you saved the attachment as) rm install.js cat >>installed-chrome.txt <<EOF content,install,url,jar:resource:/chrome/linktoolbar.jar!/content/linktoolbar/ locale,install,url,jar:resource:/chrome/linktoolbar.jar!/locale/en-US/linktoolbar/ skin,install,url,jar:resource:/chrome/linktoolbar.jar!/skin/modern/linktoolbar/ skin,install,url,jar:resource:/chrome/linktoolbar.jar!/skin/classic/linktoolbar/ EOF cd .. ./regchrome Next time you run mozilla you should see the link toolbar in place :) Also, for anyone interested in hacking on this, it's also possible to work with an unzipped copy by making the following changes: 1) make a directory linktoolbar and unzip linktoolbar.jar inside it. 2) Change the four lines added to installed-chrome by removing the string "jar:" and the string ".jar!" on each line. 3) Run regchrome again (or make these changes before running it the first time). From what I've seen so far I'm extremely impressed with the quality of this patch and I hope it gets approved very soon :) Great work guys!
The implementation really is first quality. I used to advocate the magically appearing and disappearing link toolbar, but now I think I prefer the permanent toolbar. It's in your face will remind authors about the LINK element.
Attached file XPI Strikes Back (deleted) —
Hi. As if it weren't obvious, I'm back from vacation. Thanks for the kudos on the Link Toolbar :) It wouldn't be as good as it is without the work of several people who have contributed to this bug. Most everyone can ignore the patches. Unless you've been modifying the code yourself and need to merge in my changes you don't need/want to use the patch. Just follow the procedures posted above for installing the XPI. The attached XPI, XPI Strikes Back, has a couple of changes. Michael uncovered a couple of bugs with <http://michael.nahrath.de/dnet/>. These are fixed. While I was at it, I fixed a couple of small bugs that never made it into the Known Bugs list. Changes: * ignore REL attributes containing "icon" as a word. Whitespace and hyphens are considered word delimiters, so the following are all ignored: - rel="icon" - rel="shortcut icon" - rel="desktop-icon" * fixed bug with unknown link types only being added to first transient submenu in the More menu * other small bug fixes Known Bugs: unchanged from previous list
Security question: Can someone verify that if I do <link rel="next" href="javascript:document.forms[0].submit()">, the javascript will be executed in the document's context (and not, say, the context of the chrome where it would become privileged)? Also, how hard would it be to make the href of the link appear in the status bar when you mouseover the buttons/menuitems? For document-specified links, it's important to be able to see where you're going. (Not to detract in any way from the wonderfulness of the implementation as is, though :) )
All URIs are opened with LoadURI(...), so it runs in which ever context LoadURI has it run. I tested a page with <link rel="next" href="javascript:document.forms[0].submit()"> and a minimal form and it works as expected. If it were running in the chrome context instead of the content document context, I would expect "document" to refer to linkToolbarOverlay.xul and to get a JS error from "document.forms[0].submit()".
Another of my sites shows two more bugs: <http://fakemail.de/> <link rev="Author" href="mailto:webmaster@fakemail.de" title="..."> is not displayed at all. Please note, that using the 'rev'-attribute with the value 'author' and a 'mailto:'-URL is rather common authoring practice! Maybe rev="made" is more widely spread (and _is_ displayed in the 'Document' menu) but in principle this should make no difference. None of both is named in any valid spec. Second issue: I use two rel="home" links in the same document (or one for "home" and one for "start") with different href values and different title values: One for my personal homepage and one for the homepage of the specific project. The current LinkToolbar joins the first of them with the 'Top' button and ignores all others completely. IMHO it should at least show the others (or all) in the 'Document' menu.
Regarding <link rev="Author" href="mailto:webmaster@fakemail.de" title="..."> Remember, REV indicates a reversal in the relationship between document and link. What this actually says is that the document is the author of the person at the email address, rather than the other way around. What you should be using in this case is <link rel="Author" href="mailto:webmaster@fakemail.de" title="..."> which is both accurate and displayed by the toolbar.
I see the logical weakness of <link rev="author" ...>. Thank you for the formulation of your explanation. This is the first time undersand why rev="made" may be sensible. Anyway I believe that it is not a browser's job to decide if the author's provided content is illogical and should thus not be rendered. As long as the used markup is valid in a formal way it should try to show the content.
One definite bug in the implementation (at least, as of "I am full of XPI" - haven't tested Strikes Back but it wasn't mentioned in the changelog) is that the target attribute of <link> is ignored. This is a valid and useful attribute for <link>, see http://www.w3.org/TR/html401/struct/links.html#edef-LINK
I believe rev="made" is popular because the text broswer lynx supports it and has for years. I used to use it in all my pages and I've seen reference to it in guides to writing HTML. Due to widespread use (compared to the <link> element in general, unfortunately) I think its a must to support this idiom. It never made sense it was rev to me either though...
I'm working on a draft document I hope to submit as a W3C note, which I'll submit here for public comment first. (For values of "here" probably equal to a bug filed as a dependency of this bug.) I'd recommend that we support rev="made" as equivalent to rel="made" and rel="author", but not rev="author"; "author" is a clearer value than "made", and that way we can handle the instance where the author is represented by a web page or other resource rather than a mailto: link cleanly (i.e., such a page can have rev="author" pointing to documents creating by the person the page describes). While I agree that we should support "rev" links in general, that shouldn't be part of this patch, as I don't think it's possible to adequately differentiate between the two in this UI; this should be spun off in a separate bug. Probably the first-order solution would be to add cascading menus of both "rel" and "rev" links in the "Go" menu (although I'd rather wait until the dust stops falling from mpt's imminent rearrangement of the menus); if someone comes up with a neat UI implementation for "rev" and we get his OK on it, we can do that instead or as well.
Michael Nahrath wrote: > I use two rel="home" links in the same document...One for my > personal homepage and one for the homepage of the specific project...The > current LinkToolbar joins the first of them with the 'Top' button and > ignores all others completely. My initial reason for contributing code was a strong disagreement with having everything in the toolbar be a menu or a menubutton. The resulting UI is cumbersome and silly. So I implemented a UI that assumed certain link types should have one instance; specifically the link types that make the most sense as singular items. None of the alternatives, including stuffing additional occurances into the Document or More menus, seemed acceptable. There is little disagreement that the current UI is good for the common case. I ask you to consider waiting until this bug is resolved and file a seperate RFE for supporting multiple instances of all link types. I expect much debate over the proper UI for multiple instances of Top, Up, First, Next, et al. It would be a shame to hold up acceptance of the current UI over that debate.
I have to agree with Tim T. The current implementation is quite good for the common case. I'd love to see it reviewed in time to make the next milestone release. We can argue UI and enhancements later. Let's just get it in. Do we have a working spec that defines the meaning/usage of the various rel types? I see some disagreement among people with conflicting ideas of what "rel=foo" means. I wish W3C had spelled this out in the specs long ago. If we have one, should it be submitted to W3C?
I agree that the implementation is good enough to become part of the next Milestone and should get into the tree as soon as possible. Implementing the not-so-common cases may be done later (shurely I will find a lot more ;-) About specs: There are several! I listed all of them (and all current implementations) at <http://www.subotnik.net/html/link.html> Why don't you think that <http://www.w3.org/TR/html4/struct/links.html#edef-LINK> is enough (it even explains the difference betwheen rel and rev)? Most common link types are named in <http://www.ics.uci.edu/~ejw/authoring/draft-ietf-html-relrev-00.txt> but this Internet Draft expired long ago. Fantasai collected all link types and specifications (except the new XHTML-Mod) at <http://fantasai.tripod.com/qref/Appendix/LinkTypes/alphindex.html> Can somebody find out why rel="up" was in the HTML3.0 draft but did not make it into HTML3.2? There must have been reasons and before bothering the W3C with new suggestions one should find out why they disliked it long time ago. About rev="made": kelon@pobox.com wrote: > Remember, REV indicates a reversal in the relationship between document and > link. What this actually says is that the document is the author of the > person at the email address, rather than the other way around. Exchange "author" by "made" and you get a sensible sentence: [...] What this actually says is that the document is _made_ of the person at the email address, rather than the other way around. A bit tricky to use a passive verb as a link type, which turns the logic of direction around by grammar...
Help Wanted. If you have the latest Mozilla source checked out from CVS, can successfully build a working browser, and are willing to try out patches to the build tree, then please send me an email (please don't post to this bug). As I mentioned before, the browsers I build from source all segfault. I'm going to attempt to make a patch to the build tree for the Link Toolbar, but I'd like to test these out instead of posting a bunch of broken patches to this bug.
Michael: the reason I'm working on Yet Another Spec is because the various specs conflict. I'm hoping that by combining them and smoothing out the inconsistencies, we'll have a clear plan for what link types we should support. When I finish the first draft (I'm trying, but don't hold your breath), I'm going to put it into bugzilla and invite comment (especially by yourself, and other people who have been giving thought to these issues); once we've reached a consensus on it, I was going to take it to www-html for exposure outside of Mozilla. Once this is wrapped up, Hixie/dbaron can help me submit for possible publication as a W3C Note; I realize that won't carry any real weight as a standard, but it will help make our implementation public [Note: our implementation should be adjusted to the Note, not the other way around!] and maybe provoke some interest from other browser developers.
Yes, for goodness sake, let's get this in the tree and worry about all the other issues later. It's already of extremely high quality. Henceforth, all comments of the form "it doesn't do X" (as opposed to "it does X incorrectly according to spec/standard Y", and even then it would have to be a serious bug) should be ignored. Checking it in doesn't mean we stop working on it. At least, I hope not :-) Tim: to get this checked in, you need to do the following: a) Do a CVS checkout, or download a tarball of the Mozilla source and convert it to a CVS sandbox. Instructions on http://www.mozilla.org . b) Create a patch using cvs diff -u which, when applied to a Mozilla tree, adds the Links toolbar, with all the files in sane places within the current directory structure. If there are completely new files, cvs add them so that cvs diff will work (after telling us where you are putting them so we can complain if it's the wrong place ;-). c) Post the patch, and ask for review. I could do that, and I'm sure there are several other people who could as well. Given the quality of the code and the fact that I've half-reviewed it once, I don't expect much trouble here. d) Get super-review; probably from blake@netscape.com, CCing reviewers@mozilla.org. See http://www.mozilla.org/hacking/reviewers.html for the procedure. e) Ask someone, e.g. me, to check the patch in. f) Party. :-) If you are working in a CVS tree but want to use a nightly build for testing (i.e. you don't want to have to build Mozilla yourself) you might find http://www.gerv.net/software/patch-manager.html helpful for keeping the two in sync as you test. [Oops - mid air collision. Some of this is redundant. I volunteer to help test patches. :-) . ] Gerv
Gerv: the current version of the linktoolbar contains a number of gifs in the skin/ directories. Is there a mozilla-standard way to handle these in posted patches? (I assume that cvs diff doesn't work too happily with these, even after passing -kb to cvs add...)
> If there are completely new files, cvs add them Unless Tim has checkin priveleges, he can't cvs add files. It may be easier to post a diff for existing files and a tarball of the new files.... (a little harder to review, granted).
The '-N' flag to diff will result in a patch that creates new files when applied. This only works for non-binary files. A tarball containing the images will still be required. No, I don't have CVS checkin privledges, which is OK by me.
personally i like zip's (application/zip) for new binary files, anyone building mozilla proper has zip support. -N is rather useless if you don't have a cvs account unless you forge your CVS/Entries record to include files you added. -kb is very important to remember for graphics ...
As I read the cvs man page, "cvs add" fixes your local repository so that cvs diff works with new files. But, you are right, binary files complicate things. Tim: re: the images. Probably what you'll have to do it create 1 big patch, and one zip to be unzipped in a Mozilla source dir that will put the new files in place, including the images. If you are adding images or other stuff to skins, add the image files to both skins in the zip. Does that make sense? Gerv
I am about to attach a patch (against XPI strikes back) which provides an auto-hiding link toolbar *as an option*. The default is the current behavior of displaying the toolbar all the time. I will post shortly on n.p.m.ui in order to provide a convenient place for flamewars on the desirability and UI of this option; please use this bug for technical issues only. Behavior-wise, I am 100% happy with this implementation, which isn't bad for an evening's work with essentially no prior knowledge of XUL. Elegance-wise, there are a couple of things I'm unsure about: 1) The CSS that does the auto-hiding is in the skins. This is the only place that contains CSS, so I didn't really have any other option. 2) I used oncreate on my new <menupopup> to restore the state of this option which is persist()ed in the "hidden" attribute of the toolbar. This works, but happens every time the popup is opened. My initial thought was that this is very bad because it should be a one-time initialization, but then it occurred to me that the current method means that for most browsing sessions (in which this popup is never opened) the initialization is never performed at all. So it actually *saves* us a miniscule amount of time (it really is miniscule, too - completely unnoticeable when you pop up the menu). 3) I set the "hidden" attribute on the toolbar to "maybe" in this case. This appears to work (including persisting to false), but I'm not sure it's supported to just throw an arbitrary value into an existing attribute. 4) Similarly, I set a new attribute "hasitems" to true or false on the toolbar element to allow matching based on whether there are any items in the toolbar. Is it permissible to just add an arbitrary, undefined attribute to an object? The only other possible issue with this patch is that I had to move away from goToggleToolbar and remove the broadcaster that was being used, because I couldn't figure out a way to fit a broadcaster model to this situation, and goToggleToolbar didn't do what I wanted it to. It turns out that goToggleToolbar seems to do a lot of redundant work, and my replacement (which is handling a more complicated case) is still less lines of code :)
Attached file XPI of autohide option (deleted) —
Sorry for spam... the just-attached XPI is my attempt at putting everything back together into an xpi. It's 100% untested because I don't have the capability to install xpis - use at own risk (but post back here if it worked!)
I installed the XPI with autoshow option (I agree with timeless@mac.com that autohide is a misnomer ;-) ) and it works for me. Nice work. I'll merge it into my stuff and try and incorporate it into the source tree patch I'm working on. Reducing lines of code is a Good Thing (TM)
Handling TARGET attributes isn't as straightforward as, say, making a call like LoadURI(uri, target). Getting TARGET working is low on my list right now. If someone would like to puzzle it out and submit a patch, that would be nice.
Yeah, I noticed that nsIWebNavigation was inexplicably missing a loadURI variant that had a target parameter. I'm thinking of either asking around on IRC or filing a bug asking for one, because it seems really odd not to have that basic functionality. I didn't exactly reduce lines of code per se, because goToggleToolbar still exists in the code - we just aren't using it. All I meant was that my replacement was less lines of code than if I'd cut'n'pasted the whole source to goToggleToolbar into the code. Thanks for taking a look at this :) (Btw, for anyone that's interested, I've received confirmation that the autohide (ok, autoshow, but I called it autohide in the attachment) XPI works okay.
OK, I think I found the necessary info to handle window targets in the nsIDocShell API: nsIDocShell::loadURI <http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#50> nsIDocShell::InternalLoad <http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#95> nsIDocShell::createLoadInfo <http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#125> Of course, having a convienience function, loadURI(uri, target), in nsIWebNavigation would still be a valid RFE.
Tim, nsIDocShell::loadURI and nsIDocShell::InternalLoad are noscript. InternalLoad is so for good reason -- it really should not be used from a script. I'm talking to Rick Potts right now about a function on nsIWebNavigation that will be pretty much like nsIDocShell::loadURI. I need it to do loads from cache, but it would likely work for this as well.... nsIDocShellLoadInfo objects include a target, as you noticed.
The CSS for the autoshow option should be placed in a new file: content/linktoolbar/LinkToolbarOverlay.css (or whatever) The style rule governs content and does not vary from skin to skin. For precedence see sidebarOverlay.css (lxr is your friend ;)
I've filed bug 90722 as a request for a scriptable loadURI method that accepts a target parameter. Tim, would you like to implement Eric's suggestion (which I thoroughly agree with) as part of your merge into the standard build process, or would you like me to provide the (fairly trivial) updated patch to do this? It occurs to me that it would probably be just as hard to integrate a patch if I *did* provide one as for you to just make the change based on Eric's comments.
Stuart, the easiest thing right now would be to just submit patches against the XPI like we've been doing. The patch against the source tree (which I'm about to submit) is based off of XPI Strikes Back. I'm going to concentrate on getting it reviewed and hopefully accepted. If changes to the source tree patch are required to get it accepted, I'll try and fold in the newer features as well.
I've attached a patch to the mozilla source tree and a zip containing the new binary files. Both files assumes that the source is checked out into a directory named 'mozilla'. All new files are in mozilla/extensions/linktoolbar/ or a subdirectory thereof. When it comes time, a cvs add should be relatively painless. Because the patch modifies configure.in you'll need to rerun autoconf, per the Detailed Unix Build instructions (assuming you're building on *nix). Gervase, who volunteered to test the patch, replied that the linktoolbar should be added to xpfe/components instead of extensions. I'll let him post his own comment instead of trying to summarize his position myself. I see it belonging in either location. Pragmatically speaking, the patch works. The JAR files generated in mozilla/chrome are essentially identical regardless of whether the Link Toolbar is an extension or a component. Further testing is appreciated. Please post your results here, including which platform you are on.
extensions/ is for things which are optional in the build. <LINK> support is a key part of our HTML 2 story and should not be optional. If you look at the things currently in extensions/ (venkman, chatzilla, transformiix, xmlterm) you can see that it's not appropriate for the links toolbar to be there. xpfe/components already contains things like the personal toolbar and so on; it's the logical place for the links toolbar. The Links Toolbar should be placed in components, and the code altered so that it works like the Personal Toolbar - using static rather than dynamic overlays, and tying into the current components build process. The skin bits should be put in the relevant bits of the skin directories. Gerv
I disagree on a couple of counts: 1. LINK support is an optional part of the HTML spec. Having the implementation be an optional part of the build is congruous with that. This is really just a pedantic argument, because the Link Toolbar is included in the build by default. You have to explicitly disable it. 2. You only listed example extensions that support your argument. Some other things found in extensions that don't support your position are cookie and wallet. I'm not opposed to putting the Link Toolbar in xpfe/components. However, I disagree that extensions is any less appropriate than components. The work's been done. The patch works.
FWIW, I agree with Gerv about the location. I'd also be very disappointed to see this go in without the autohide option - not just because I wrote it, but because a *lot* of people besides myself seem to want to use it as their preferred mode. This should be trivial if your work is based on "strikes back", because you can probably just replace the files my patch modifies (excluding the skin file patches which should go in a new css file in content anyway) If there's anything I can do to help at this point, let me know (perhaps in private mail to avoid cluttering up the bug).
Whadya mean "cluttering up the bug"? We're talking implementation details fer chrissakes ;) I'm confident that the autoshow feature will make it into the implementation. Either as a revision to this patch, or as a seperate patch once the toolbar is in CVS. Also, as you mentioned in n.p.m.ui, mpt has some reservations about the depth of View -> Show/Hide -> Link Toolbar -> .... Although I'm confident that autoshow will make it in eventually, I'm not so confident that the current implementation won't hold back the Link Toolbar as of XPI Strikes Back, when we had concensus that it was "Good Enough". If you'd like to help, you could work on making a patch to Gerv's satisfaction. I wouldn't mind that at all. That should at least gaurantee r=Gerv. Meanwhile, looks like I'll have to go fishing for another reviewer for the current patch :)
> 1. LINK support is an optional part of the HTML spec. Having the implementation > be an optional part of the build is congruous with that. Having UI for forward-backward-reload is not even part of any HTML spec. But you would not call it "optional" for a browser. > 2. You only listed example extensions that support your argument. Some other > things found in extensions that don't support your position are cookie and > wallet. Where is "Personal Toolbar" and "Status Bar"? "Site Navigator Bar" should be in the same location.
I can imagine, depending on who does your review, that some people might object to the UI on the grounds of *not* having autoshow... anyhow... In terms of location in the build, I'd like to see this put in the "right" place now. Arguing where the "right" place is is valid, but arguing that it should go in extensions because the patch exists and works isn't a valid argument - that logic has led to many messes that always end up never getting fixed. My personal suspicion is that this is how wallet and cookie ended up in extensions and never got moved... I can (just about) live with a checkin without autoshow so long as somebody can reasonably guarantee that the autoshow will get r/sr fairly quickly afterwards, and won't be held up unreasonably hoping for an ideal UI that will never come. But I can't live with putting it in a wrong location, and I've yet to be persuaded that extensions isn't a wrong location.
> Also, as you mentioned in n.p.m.ui, mpt has some reservations about the > depth of View -> Show/Hide -> Link Toolbar -> .... Although I'm confident > that autoshow will make it in eventually, I'm not so confident that the > current implementation won't hold back the Link Toolbar as of XPI Strikes > Back, when we had concensus that it was "Good Enough". If I can get mpt's personal guarantee that he won't oppose the checking in of the current autoshow implementation including UI (I don't know if I can, but I can try ;) ) would you consider adding the autoshow component to the initial patch? I can't guarantee that I'll have time this weekend to work on providing a build system patch (ie, don't stop working on it yourself assuming that I will). But I will try to find time and have a go at it - although any patch I produce will probably include autoshow unless someone persuades me otherwise.
Tim: wallet in Mozilla is a byword for doing everything that it's possible to do, wrongly. And the cookie manager is part of wallet. > The work's been done. The patch works. From this logic comes many evil hacks. If you like, I could drag brendan or someone over here to tell you to put it in components and integrate it properly (removing dynamic overlaying), or we could just skip that step and do it anyway. :-) And you could take the opportunity to integrate auto-hide, although I would say that this is definitely _not_ required for initial checkin. Mozilla is allowed to go through times when bits of it are half-finished - that's what development is. And what are we going to call it in the UI? I like "Site Navigation Bar", but something a bit shorter might be better. We definitely want something better than "Link Toolbar" - your average user will have no clue what that is. Gerv
> And what are we going to call it in the UI? I like "Site Navigation Bar", but > something a bit shorter might be better. We definitely want something better > than "Link Toolbar" - your average user will have no clue what that is. I don't like 'Link Toolbar' either - IE escapees might confuse it with IE's Links Bar (Bill's name for the Personal Toolbar). I'm a bit worried that 'Site Navigation Bar' might be confused with the regular Navigation Toolbar. How about 'Site Links', 'Page Links' or 'Document Links' or something?
> If you like, I could drag brendan or someone over here to tell you to put > it in components and integrate it properly (removing dynamic overlaying) What I would like, if you could, is for you to drag some module reviewers into here to review the patch and draw their own conclusions. An objective second (and third, fourth, etc.) opinion would certainly be helpful. It's certainly ironic if, as you say, wallet is the bad seed of extensions, because I used it as a code example to help me figure out how to incorporate the link toolbar into the build.
The name for this toolbar should include the word "Toolbar" just like "Navigation Toolbar" and "Personal Toolbar". I'll throw the name "Advanced Navigation Toolbar" into the fray. For people worried about long menu labels, here are examples of some other long-ish menu labels in comparison View -> Advanced Navigation Toolbar File -> Open Web Location... Ctrl+Shift+L Search -> Find in This Page... Ctrl+F Tasks -> Privacy and Security Bookmarks -> Manage Bookmarks... Ctrl+B And, of course, there are the generated menu items in "Go" and "Bookmarks" menus that can be very long. Unless there's a limit stated in a UI guideline, I recommend prioritizing clarity over brevity, taking into consideration that sometimes brevity aids clarity.
"Site Navigation Toolbar" -- both shorter and more descriptive than "Advanced...".
> What I would like, if you could, is for you to drag some module reviewers into > here to review the patch and draw their own conclusions. An objective second > (and third, fourth, etc.) opinion would certainly be helpful. I can't offer anyone's time to review this apart from my own, which I have been offering and continue to offer. I'm not sure what you mean by an "objective second opinion" - do you think I have some vested interest (stock options perhaps) in the components/ directory? The people who probably have the final say where it goes are blake and ben - perhaps you should ask them to pop over here and give a ruling. > The name for this toolbar should include the word "Toolbar" just like > "Navigation Toolbar" and "Personal Toolbar". Semi-counter-example: Status Bar. How is a "toolbar" different from a "bar"? Good question. I think we should avoid the word "Advanced"; there is nothing particularly Advanced (advanced compared to what?) about <LINK>-based navigation, and it's a word that I think will scare users. What's it really _for_? It's for navigating between related pages. "Page Navigation Bar"? Regarding possible confusion, we should also think whether, given that it contains Print and Search, Reload and Stop buttons (which are not directly to do with Navigation), whether "Navigation Toolbar" is the right name for the main toolbar. Oh, and how about "Only When Supported" or "When Supported" rather than "Only As Needed"? Gerv
> I'm not sure what you mean by an "objective second opinion" - do you think > I have some vested interest (stock options perhaps) in the components/ > directory? I was contrasting your offer to bring someone in to prove me wrong versus my suggestion to bring someone in to review the patch. I don't attribute malice to any of your actions or opinions. I just disagree with some. I readily admit I may be wrong. I'm just would like a second opinion. A moment ago you were willing to try and bring someone in to prove your point. I assumed you could apply these same connections to attempt and attract another reviewer. > The people who probably have the final say where it goes are blake > and ben - perhaps you should ask them to pop over here and give a ruling. Would you mind trying? I'm sure you have a greater chance of attracting their attention than I do. In the meantime, it would be helpful if someone could test the current patch. Assuming that I need to rework it for extensions, I still need to know that I at least got things right with the first patch. Undoing the changes made by the current patch is as simple as: $ rm -rf configure.in extensions/makefile.win extensions/linktoolbar $ cvs update configure.in extensions/makefile.win $ autoconf $ gmake -f client.mk build
Blocks: 90966
I've got this installed locally and it looks to run rather well. One thing I've noticed is that I can right-click the items in the Document and More menus. The item executes but the menu doesn't close. The Bookmarks menu on the Personal Toolbar doesn't do this, I wonder why these menus do?
>I don't like 'Link Toolbar' either - IE escapees might confuse it with IE's >Links Bar (Bill's name for the Personal Toolbar). I'm a bit worried that 'Site >Navigation Bar' might be confused with the regular Navigation Toolbar. How about >'Site Links', 'Page Links' or 'Document Links' or something? how about: Site-Provided ToolbarSite-Specific Toolbar Site-Customized Navigation Bar Remote Site's Navigation Bar Extra Site Navigation Toolbar Quick Links Toolbar or some variation thereof?
First comments on the latest patch: sounds good all in all. I haven't tested the patch but I began looking at the code. Here are a few points that I would raise, but then again who am I to say whether they are really bad or just a matter of style... 1) What struck me is that in the functions defining an object, for example, function LinkToolbarHandler(), sometimes you define member functions using "this.functionName = function()", and sometimes you define functions (are they member functions? I don't think so) without using this.functionName, like function getLinkType(relAttribute). I am still wondering how a call to "function fctName()" {} inside another function works. IMHO, it would all be much clearer if you used LinkToolbarHandler.prototype = { } instead of the confusing definitions you have at the moment. I trust the current code works, but it just seems evil. 2) Sounds like overkill to me. I'd remove the |else return false| entirely. + if (findWord("icon", this.rel.toLowerCase())) + return true; + else + return false; + + return false; 3)+ if (!this.rel) return true; + if (this.isStylesheet()) return true; + if (this.isIcon()) return true; This could be written |if( !this.rel || this.isStyleSheet() || this.isIcon() ) return true|, but then again it's just a matter of style. The efficiency should be exactly the same. Ok this is just an initial code-only review, I'll comment more as I read the patch further.
> 1) What struck me is that in the functions defining an object...sometimes > you define member functions using "this.functionName = function()", and > sometimes you define functions...like function getLinkType(relAttribute). > I am still wondering how a call to "function fctName()" {} inside another > function works. Think of it as the difference between instance methods and static methods in a class based language. That's not how it works in JavaScript, but that's the basis for how I wrote the functions. Functions defined as "function fctName()" don't need access to any "this.foo" members, so they're sort of like static methods. Defined that way, the function is only in scope in the containing function/class, so it's also a form of encapsulation. > IMHO, it would all be much clearer if you used > LinkToolbarHandler.prototype = { } instead of the confusing definitions you > have at the moment. I trust the current code works, but it just > seems evil. Well, coming from a Java programming background, the Class.prototype = { } sytax seems evil ;-) However, when in Rome, do as the Romans do. For that reason, I've considered rewriting to use the syntax you mention. > 2) Sounds like overkill to me. I'd remove the |else return false| entirely. > + if (findWord("icon", this.rel.toLowerCase())) > + return true; > + else > + return false; > + > + return false; Yup, that's an oversight. I was actually changing it from a guard conditional idiom to a regular if-else idiom, but forgot to remove the last return statement. > 3)+ if (!this.rel) return true; > + if (this.isStylesheet()) return true; > + if (this.isIcon()) return true; > This could be written |if( !this.rel || this.isStyleSheet() || this.isIcon() ) > return true|, but then again it's just a matter of style. > The efficiency should be exactly the same. Agreed. This is partly a style issue, but also a holdover from when I erroneously convinced myself that JavaScript didn't short-circuit conditionals. I say "partly a style issue" because I like to keep unrelated gauard conditionals in seperate if statements. I'll rewrite it as: if (!this.rel) return true; if (this.isStylesheet() || this.isIcon()) return true; Thanks for the review!
What's the status of this feature getting in?
Tim requested super-review some time ago, but seems to have been ignored. I suggest he request it again, and if a response is not received within 48 hours from the selected super-reviewer, to take it up with staff@mozilla.org. Gerv
There was also the issue of whether it should be in extensions/ or components/ - I don't know whether any resolution was ever reached on this, but (last I remember hearing) the only actual existing patch placed it in extensions, which some people thought was wrong. Was this ever resolved?
That's a very good point. It should be in components. If this is to get super-review, a patch needs to appear which does this. It'll probably change enough that it needs reviewing again, as well. We'll get there eventually :-) Gerv
I really wish I had the time and knowledge to do this. I said I'd "try to find time" months ago (with a warning not to depend on me) and needless to say, I couldn't (life is hectic and shows no sign of slowing down). Surely all it would take for someone who knows what they're doing would be to change a few directories, add a few overlay tags, and make sure that the whole thing compiles and works afterwards?
Blocks: 96683
No longer blocks: 96683
I really hope this patch isn't going to end up bitrotting *again* due to such trivial issues. *please*, if there's anyone out there with the time or ability to work on this, consider doing the small amount of work necessary to move the most recent attached patch from extensions/ to components/. I'll still snap it up the moment I get a chance to do so, if nobody already has, but I still don't see that happening in the forseeable future. So consider this pitiful grovelling as bug-advocacy: I know that the second-last thing this bug needs is more comments, but the *very* last thing it needs is to be abandoned to bitrot again for the third time.
The extensions to components issue is in stalemate. I could spend the time to move things to components to satisfy Gerv, but that wouldn't garauntee an sr=. Gervase and I have both requested second opinions and have heard nothing. To be honest, I'm not inclined to spend any more of my time coding until this bug receives some arbitration or reviewer attention. There's also the possibility that the previous bitrot and current neglect means this feature isn't desired except by our small group. When I get a chance this week I'll follow Gerv's suggestion for escalating the review of this bug. In the meantime, if someone wants, they can advocate this bug now if they're as confident as I am that extensions is at least an equally appropriate location for the link toolbar. No coding necessary. In that case, we still need an r= and an sr=.
Attached patch Gerv's patch v.1 (obsolete) (deleted) — — Splinter Review
If you want something doing... The patch I just attached is all the files, with the locations changed to the correct place. I've cvs added them there; they can be removed if the super-reviewer decides they should be elsewhere. I haven't included the binary images; they haven't changed, although they've moved to the relevant skin directories (toolbar subdir in the case of modern) if anyone wants to try this patch out. I've also removed quite a lot of debugging dump() statements and other debugging code which should not go into the tree. Lastly, I changed a few style nits to fit in with the tree style. I've tested it a bit and it does still seem to work. I can't review my own patch, but drbrain and tim wrote most of it. Hmm. Never mind - could one of you review it, please, and make sure it's still the same patch? :-) Gerv
Come on, people - if you are all so keen to see this in the tree, you could at least review my patch. Gerv
More specifically: drbrain or tim, could you please either review this patch or comment that you aren't going to? Thanks :-) Gerv
Are they allowed to review the patch, since it contains their code? Anyway, I talked to db48x on IRC last night and agreed to exchange his review of this patch for my review of his (52730). Adding him to cc to make sure it happens...
> Are they allowed to review the patch, since it contains their code? Well, I reviewed that bit. They need to review the changes I made. > Anyway, I > talked to db48x on IRC last night and agreed to exchange his review of this > patch for my review of his (52730). Adding him to cc to make sure it happens.. OK. Whatever. Just as long as someone reviews it ;-) Gerv
Brought to you by The Reviewers Corp Inc S.A. (TM) Thanks to Boris Zbarsky for helping me out very much :-) *************languageDictionary.js*************** 1. + if (this.getDictionary()[languageCode]) + return this.getDictionary()[languageCode]; should be (for performance and style reasons) var language = this.getDictionary()[languageCode]; if (language) return language; else return ""; 2. + if (this.dictionary == null) should be (for style reasons) if (! this.dictionary) ***********linkToolbarHandler.js************ 1. + for (var i = 0; i < nodeList.length; i++) should be (for performance reasons) var length = nodeList.length; for(var i = 0; i < length; i++) 2. In function this.handle(): + var element = new LinkElementDecorator(element); Use another var name than |element|. Very confusing. 3. + * XXX: would rather add some methods to Element.prototype instead of + * using a decorator, but Element.prototype is no longer exposed + * since the XPCDOM landing, see bug 83433 Bug is fixed. Should we re-do the patch to accomodate this? 4. + function getText(element) { + return condenseWhitespace(getTextRecursive(element)); + } and related functions: A (much?) more efficient way would be to use the Range object which does everything automatically for you. I'm not sure myself how to do it exactly, but it should be easy. I can do that if you so wish. 5. + this.enableParentMenuButton = function() { + if(this.getParentMenuButton()) + this.getParentMenuButton().removeAttribute("disabled"); + } cache this.getParentMenuButton() 6. In this.displayLink and this.isAlreadyAdded: something's really wrong. You assign "true" to getKnownLinks()[linkElement.href] then in isAlreadyAdded() you attempt to use getKnownLinks()[linkElement.href].media and .hreflang. What's the point? Is this on purpose? Maybe add comments to the code if it's on purpose? Should it be getKnownLinks()[linkElement.href] = linkElement; ? Please also cache this.getKnownLinks()[linkElement.href] since a call to that isn't exactly cheap. *************http.js************** 1. global var ioService is initialized in getHTTPHeaders() but overriden with another instance of nsIIOService directly in doGetHTTPHeaders(). What gives? Why the commented out code in doGetHTTPHeaders? 2. badly named http.js file (in our opinion, a too generic name) 3. Rewrite doGetHTTPHeaders(). Bad indentation and excessive unconditional |return|. If possible do less inside the try {}, and more outside. In that function you sometimes return false and sometimes return void, that is inconsistent. 4. in function parseLinkHeader(content) Undeclared variables: contentlen, state, starthref, endhref, startparam, endparam, startparamval, endparamval, curchar 5. comment that some functions in there are not used (getHTTPHeaders, ...) 6. + // XXX: cleanup + var ioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces["nsIIOService"]); + var base = ioService.newURI(window._content.document.base + ? window._content.document.base + : window._content.document.URL, null); Reuse the ioService global var. ************linkToolbarOverlay.js************** 1. + for (var i = 0; i < metas.length; i++) { should be var length = metas.length; for(var i = 0; i < length; i++) 2. + for (var i = 0; i < metas.length; i++) { + if (metas.item(i).hasAttribute("http-equiv")) + if (metas.item(i).getAttribute("http-equiv").toLowerCase() == "link") + links.push(parseLinkHeader(metas.item(i).getAttribute("content"))); should be (again, in our opinion) for (var i = 0; i < length; i++) { var currentMeta = metas[i]; if (currentMeta.hasAttribute("http-equiv") && currentMeta.getAttribute("http-equip").toLowerCase() == "link") links.push(parseLinkHeader(currentMeta.getAttribute("content"))); } 3. + function clicked(event) { + if (event.target.nodeName == "menubutton") return; + if (!event.target.getAttribute("href")) return; + loadURI(event.target.getAttribute("href")); should be (imho) function clicked(event) { if (event.target.nodeName == "menubutton") return; if(event.target.hasAttribute("href")) { loadURI(event.target.getAttribute("href")) } return; } **************linkToolbarOverlay.xul************* 1. <script src="..."> should refer to full chrome URL's, not relative url's 2. No need for language="javascript" in the <script>'s. We don't use that attribute in Mozilla. 3. Investigate use of observers instead of addEventListener(load). I'm not sure at all about this one. It's just a thought I wrote down, maybe one of you knows more about them. 4. Won't <toolbox id="navigator-toolbox"> interfere with the navigator.xul one? Is it on purpose? LinkToolbarOverlay.css is diffed twice, as well as all the gifs. Navigator.xul and linkToolbarOverlay.dtd changes look good. We'll look again when we get a new patch :-) Thanks for doing all this work guys, it's very appreciated!
I can't get this to run right, mozilla keeps crashing in docshell.dll. (the mozilla window opens, with the link toolbar showing, but it crashes as it tries to load a document) Looking at the patch, the modifications to the jar.mn files show that you're putting all the files in with the regular chrome files, but the zip file of images extracts to extentions/linktoolbar/. Personally, I'd prefer all the files to go under components/linktoolbar/[content|skin], with a seperate jar.mn. You can still have the files end up in comm.jar/modern.jar/classic.jar and so on if you like, but it makes rebuilding just this project much easier. Also, the diff is kind of unorganized; some of the files (like languageDictionary.js) are diffed from the directory they're in, whereas some files (like the jar.mn files) are diffed from the mozilla/ directory. It'd make it much much easier if they were all diffed from the same point in the tree. Other than those nits and the things Fabian brought up, I think this is pretty close. (I doubt the crash is truely your fault)
Optimizations should be benchmarked to ensure enough (or any) improvement is made . I disagree on adding temporary variables merely as a style improvement. Avoiding the use of temporary or cache variables improves readability and maintainability ("Refactoring", Fowler). They should be avoided unless needed for a demonstrated optimization. Can we get the "unoptimized" code in the tree and accept optimizations as patches. I write "unoptimized" because I've already done some optimization, backed by benchmarks, that shaved off a couple hundred milliseconds. BTW, how many milliseconds per page load is considered a signifigant performance improvement to justify less concise code? I've seen examples of conflicting style in the tree, so I'm suspicious whenever someone cites style improvements. Is there a style document, or a particular section of code to emulate? Or are these a matter of personal preference? The one convincing style suggestion so far is switching to "Class.prototype = { ... }" for class declarations. There is overwhelming evidence that this is the proper style. > *************languageDictionary.js*************** [...] > 2. In function this.handle(): > + var element = new LinkElementDecorator(element); > Use another var name than |element|. Very confusing. Would renaming it to "htmlElement" make it clear? > 3. [...] > Bug is fixed. Should we re-do the patch to accomodate this? It can always be patched seperately once it's in the tree, yes? > 6. > In this.displayLink and this.isAlreadyAdded: something's really wrong. Agreed. For now, we can remove the following lines, since the conditions are always false: // FIXME: media="screen all" should be equivalent to media="all screen" if (!match(this.getKnownLinks()[linkElement.href].media, linkElement.media)) return false; if (!match(this.getKnownLinks()[linkElement.href].hreflang, linkElement.href)) return false; That will eliminate the confusion. The intended functionality can be added later , and even this botched attempt would have been incomplete. > *************http.js************** [snip] Agreed. http.js is a mess and only META HTTP-EQUIV works. The code might as well be removed until it can be done completely and properly. > ************linkToolbarOverlay.js************** [...] > 3. > + function clicked(event) { > + if (event.target.nodeName == "menubutton") return; > + if (!event.target.getAttribute("href")) return; > + loadURI(event.target.getAttribute("href")); > should be (imho) > function clicked(event) { > if (event.target.nodeName == "menubutton") return; > if(event.target.hasAttribute("href")) { > loadURI(event.target.getAttribute("href")) > } > return; > } The conditionals are examples of a programming idiom called a guard clause. These take the form of simple conditionals at the top of a function that guard entry to the rest of the function. Anyone familiar with the idiom will understand the intent. It makes the rest of the method body easier to read. Normally I follow the guard clauses with a blank line, which unfortunately I neglected to do here. > **************linkToolbarOverlay.xul************* [...] > 4. Won't <toolbox id="navigator-toolbox"> interfere with the navigator.xul one? > Is it on purpose? It's on purpose. It is how XUL overlaying is done.
> > *************languageDictionary.js*************** > [...] > > 2. In function this.handle(): > > + var element = new LinkElementDecorator(element); > > Use another var name than |element|. Very confusing. > > Would renaming it to "htmlElement" make it clear? Erg...brainfart. How about "linkElement"? I had used that originally, but found it redundant with the class name "LinkElementDecorator".
> Looking at the patch, the modifications to the jar.mn files show that you're > putting all the files in with the regular chrome files, but the zip file of > images extracts to extentions/linktoolbar/. Yes, that's my fault - I didn't accompany the patch with a correctly-zipped zip of the images. Would you like me to do that? > Also, the diff is kind of unorganized; some of the files (like > languageDictionary.js) are diffed from the directory they're in, whereas some > files (like the jar.mn files) are diffed from the mozilla/ directory. It'd > make it much much easier if they were all diffed from the same point in the > tree. I'm not quite sure why that has happened - they are all diffed from the mozilla/ directory. Ah, I see - the ones that are new files, and have been cvs added, are the ones that appear to be diffed from the directory they are in. This must be a quirk of CVS. Does it present problems applying the patch? Gerv
Attached patch Patch v.2 (deleted) — — Splinter Review
Attachment #47932 - Attachment is obsolete: true
>> Looking at the patch, the modifications to the jar.mn files show that you're >> putting all the files in with the regular chrome files, but the zip file of >> images extracts to extentions/linktoolbar/. >Yes, that's my fault - I didn't accompany the patch with a correctly-zipped zip >of the images. Would you like me to do that? Yea, that'd be great, thanks. >> Also, the diff is kind of unorganized; some of the files (like >> languageDictionary.js) are diffed from the directory they're in, whereas some >> files (like the jar.mn files) are diffed from the mozilla/ directory. It'd >> make it much much easier if they were all diffed from the same point in the >> tree. >I'm not quite sure why that has happened - they are all diffed from the mozilla/ >directory. Ah, I see - the ones that are new files, and have been cvs added, are >the ones that appear to be diffed from the directory they are in. This must be a >quirk of CVS. Does it present problems applying the patch? That really is weird. It does make it difficult to apply, because all the files that were added end up in mozilla/. It's not really more than a nuisance if you expect it to happen.
Comment on attachment 48698 [details] [diff] [review] Patch v.2 Comments on Gerv's patch v.2 > Index: xpfe/browser/resources/content/linkToolbarHandler.js >+ this.handle = function(linkElement) { >+ // XXX: if you're going to re-enable handling of anchor elements, >+ // you'll want to change this to AnchorElementDecorator >+ var linkElement = new LinkElementDecorator(element); >+ >+ if (linkElement.isIgnored()) return; >+ >+ var relAttributes = linkElement.rel.split(" "); >+ for (var i = 0; i < relAttributes.length; i++) { >+ var linkType = getLinkType(relAttributes[i]); >+ this.getItemForLinkType(linkType).displayLink(linkElement); >+ } >+ } That function is now broken. What Fabian and I meant by our comment about the variable name is that the function argument should be named something different from the variable you assign the new LinkElementDecorator to. The changes you've made don't really help that and break the function (since |new LinkElementDecorator(element);| will now happily fail). > Index: xpfe/browser/resources/content/linkToolbarOverlay.js >+ links.push(parseLinkHeader(metas.item(i).getAttribute("content"))); Since you removed http.js, parseLinkHeader() is no longer present. Please put http.js back (with a better name like http-link.js maybe) or remove this call. >Index: xpfe/browser/jar.mn >+ content/navigator/http.js (resources/content/http.js) Again, be consistent in your removal or not of http.js
Attachment #48698 - Flags: needs-work+
> This must be a quirk of CVS. Does it present problems applying the patch? It should not be a problem if one sets POSIXLY_CORRECT=1 when making the diff and when applying the patch.
You should keep http.js in, because if there are 3 places the information can come from, supporting 2 of the three is better than just 1 of the three. I think the only complaint was that the name doesn't suggest who uses it, or what it does. The name could indicate that it's required to support the http protocol, for all someone unfamiliar with mozilla knows. On the other hand, if you were to stick everything in your own subdirectory of xpfe/components/, you could pretty much call it whatever you want, but that's just my opinion. Also, I keep thinking that the language dictionary stuff has been done before, perhaps there's a scriptable object out there that we could reuse? If not then one should be created. On a different note, are getDictionary, createDictionary etc closures? Just curious as to how JS does that. Can the four regular expressions in findWord() be replaced by a single expression? Maybe /\W$word\W/?
http.js has several problems, six of which were outlined by Fabian just a few comments ago. To his list, I'll add that it's not object oriented, as the rest of the code is. It will take more work then just renaming the file. Work that can be done later, under a seperate bug for adding support for HTTP LINK headers.
Comment on attachment 48698 [details] [diff] [review] Patch v.2 >Optimizations should be benchmarked to ensure enough (or any) improvement is >made. That sounds like a good argument to me. In my comment I suggested using var length = array.length; for(var i = 0; i < length; i++) because Boris told me that JS doesn't inline array.length in a for loop, because the array could grow in the loop. So for a large array, caching the length would avoid a lot of (unnecessary, since the length is constant) calls to array.length. I agree that caching an expression that comes back often just for a matter of style is not good. We should probably cache expressions only in loops or functions called very often. I hope you see what I mean. >Can we get the "unoptimized" code in the tree and accept optimizations as >patches. Sounds good to me, but if this going to add overhead to the page load time (I haven't tested the patch so I can't tell), let's try to quickly work on optim after it's checked in :-) >> 3. >[...] >> Bug is fixed. Should we re-do the patch to accomodate this? >It can always be patched seperately once it's in the tree, yes? Yes. But as usual let's not forget it. >Agreed. http.js is a mess and only META HTTP-EQUIV works. The code might as >well be removed until it can be done completely and properly. OK. >The conditionals are examples of a programming idiom called a guard clause. >[...] >Normally I follow the guard clauses with a blank line, which unfortunately I >neglected to do here. You didn't, I removed the blank line when commenting on it. The rest looks fine. Address Boris' new comments and we'll almost be there!
Contradicting myself somewhat about having optimizations be a seperate bug I did a benchmark. Using a test document containing 127 REL links (the GNU make manual test case modified to have LINK elements instead of anchors) I tested the following optimization: var length = nodeList.length; for (var i = 0; i < length; i++) { /* ... */ } Average Page Load (milliseconds) Before After 936ms 926ms I loaded the test case 6 times, and through out the first page load. The optimization saves an average of 10 milliseconds in this case. Things to consider: 1. My machine: 750 MHz Athlon, 256 MB RAM, Linux 2. 127 REL links is a large number of links 3. Is 10 milliseconds a signifigant performance improvement I'm being a bit pedantic. In this case, the optimization is simple, the use of a temporary hardly obscures the code, and doing it demonstratably saves time. One thing to note, nodeList is /not/ an array. It's an XPCOM wrapped C++ object implementing the DOM NodeList interface. I suspect the implementation caches its length so all we're paying for here is the method invocations. Side Note: In LinkToolbarHandler.js, this code clears out all the link toolbar buttons and menuitems. It loops using an associative array: this.clearAllItems = function() { for (var linkType in this.items) this.items[linkType].clear(); this.hasItems = false; } This may be another candidate for optimization, though doing so will be more complex than just adding a temporary.
>One thing to note, nodeList is /not/ an array. It's an XPCOM wrapped C++ object >implementing the DOM NodeList interface. I suspect the implementation caches >its length so all we're paying for here is the method invocations. As far as I know, a NodeList, as defined by the W3C, is live, i.e. its length can vary during the execution of the program. This is why I said the length of the NodeList is recomputed each time (though it's probably just a call to get a data member of the objct). Maybe someone knows exactly what's going on?
Attached patch Patch v.3 (deleted) — — Splinter Review
Patch v.3 attached. Setting POSIXLY_CORRECT=1 didn't seem to have any effect on the diff produced. Any further comments should be of the form: "This really can't go into the tree as it is, because the following code is evil, bad and wrong, or doesn't work at all: ..." Gerv
Comment on attachment 48807 [details] [diff] [review] Patch v.3 Alright. This really can't go into the tree as it is, because the following code doesn't work at all (except the overall file location thing, which may be just evil): >Index: xpfe/browser/resources/content/linkToolbarOverlay.xul >+<?xml-stylesheet href="chrome://linktoolbar/skin/linkToolbarOverlay.css" type="text/css"?> You mean href="chrome://navigator/skin/toolbar/linkToolbarOverlay.css" right? Either that, or fix your jar.mn files.... Which seem to be inconsistent, by the way. More on this later. >+ <script type="application/x-javascript" src="chrome://navigator/content/LinkToolbarHandler.js" /> >+ <script type="application/x-javascript" src="chrome://navigator/content/LinkToolbarItem.js" /> >+ <script type="application/x-javascript" src="chrome://navigator/content/LanguageDictionary.js" /> The first "l" in the filename should be lowercased in all of those. Otherwise the scripts don't get loaded and bad things happen. >Index: xpfe/browser/jar.mn >+ content/navigator/languageDictionary.js (resources/content/languageDictionary.js) >+ content/navigator/linkToolbarHandler.js (resources/content/linkToolbarHandler.js) >+ content/navigator/linkToolbarItem.js (resources/content/linkToolbarItem.js) >+ content/navigator/linkToolbarOverlay.js (resources/content/linkToolbarOverlay.js) >+ content/navigator/linkToolbarOverlay.xul (resources/content/linkToolbarOverlay.xul) >+ locale/en-US/navigator/linkToolbar.dtd (resources/locale/en-US/linkToolbar.dtd) So. Question. Wasn't this stuff going to move to xpfe/components/linktoolbar ? It seems to be living in xpfe/browser/resources/content at the moment with Gerv's current patch... Don't you want to put this stuff in components/ and edit components/jar.mn to include it in comm.jar? >Index: themes/classic/jar.mn >+ skin/classic/navigator/linkToolbarOverlay.css (navigator/linkToolbarOverlay.css) >Index: themes/modern/jar.mn >+ skin/modern/navigator/toolbar/linkToolbarOverlay.css (navigator/toolbar/linkToolbarOverlay.css) These put the css file in different places in the jar. Please fix that. The path inside the jar should be the same for the CSS file and the images in both themes. > Index: themes/modern/navigator/toolbar/linkToolbarOverlay.css All the image urls in this file are broken with the current file locations. Once you have the jar.mn's fixed, please update the chrome:// urls in here.
Attachment #48807 - Flags: needs-work+
Gerv, when you do get this into the tree please file a bug on me for the autohide support, and I'll update my patch to the new locations. Also, FWIW: Anyone testing this who's ever had the xpi installed in the same installation directory (on Linux at least): I've found that regchrome will *NOT* uninstall the xpi even if you remove the relevant lines from installed-chrome.txt first. The only way to do it is either to edit all-*.rdf and remove the linktoolbar sections, or (at your own risk) blow away all-*.rdf completely (as well as removing the lines from installed-chrome.txt) and run regchrome again.
bz: fair comments :-( I think I probably wasn't testing the current version, but an old version - this is why it all still seemed to work. >Index: xpfe/browser/jar.mn > So. Question. Wasn't this stuff going to move to > xpfe/components/linktoolbar ? > It seems to be living in xpfe/browser/resources/content at the moment with > Gerv's current patch... Hmm. I appear to have misremembered the substance of the argument about this. However, I would now argue (and perhaps this is a change from my previous position; I'm not afraid to admit it when I'm wrong :-) that its closest counterpart in the tree currently is the Personal Toolbar, and so it should live wherever that lives - which is in navigator/, where I put it. This was what was going through my head as I was making the patch, at any rate. The links toolbar is chrome specific to the browser window, so should be somewhere in xpfe/browser, certainly. > These put the css file in different places in the jar. Please fix that. > The path inside the jar should be the same for the CSS file and the > images in both themes. If you examine the current versions of jar.mn for the two themes, you will see I am merely continuing a trend. All the toolbar stuff in the Modern theme seems to be in the toolbar directory; but Classic doesn't have a toolbar directory. Are you merely offended by the aesthetics, or do you think something will break? > > Index: themes/modern/navigator/toolbar/linkToolbarOverlay.css > > All the image urls in this file are broken with the current file locations. > Once you have the jar.mn's fixed, please update the chrome:// urls in here. Serves me right for not testing with Modern. I fixed the other stuff - thanks. Gerv
> >+ <script type="application/x-javascript" src="chrome://navigator/content/LinkToolbarHandler.js" /> > >+ <script type="application/x-javascript" src="chrome://navigator/content/LinkToolbarItem.js" /> > >+ <script type="application/x-javascript" src="chrome://navigator/content/LanguageDictionary.js" /> > > The first "l" in the filename should be lowercased in all of those. > Otherwise the scripts don't get loaded and bad things happen. For what it's worth, the files themselves should have been left with leading uppercase characters. The filenames, not the URLs that should be corrected. I explained the convention several weeks back when someone previously commented on them. The filename matches the classname that's defined within that file, a convention used by all Java programmers. So, for example, the class LanguageDictionary is defined in LanguageDictionary.js.
> All the toolbar stuff in the Modern theme seems to > be in the toolbar directory; but Classic doesn't have a toolbar directory. > Are you merely offended by the aesthetics, or do you think something will > break? I looked at this. All the stuff in the toolbar dir in modern.jar is images that are only used by and referenced by modern. The CSS files do _not_ live in the toolbar dir. Your patch v3 put the link toolbar CSS file in different locations inside modern.jar and classic.jar. Thus different chrome:// urls would be necessary to reference that same CSS file, which we can't do. So as far as I can tell, you need to put the CSS in the same place in both jars. The images should follow the theme's convention -- inside toolbar/ in modern and just on their own in classic. Then have the two different CSS files refer to the images with different chrome:// urls > For what it's worth, the files themselves should have been left with leading > uppercase characters. That would be fine too. As long as the names are consistent. :) Gerv, attach a new patch once you have all the chrome:// paths straightened out?
When in Rome, do what the Romans do. I can't see another example of leading uppercase in Mozilla js, so lowercase it is. Does this next patch pass muster? Gerv
Attached patch Patch v.4 (deleted) — — Splinter Review
Comment on attachment 48902 [details] [diff] [review] Patch v.4 >Index: themes/modern/navigator/linkToolbarOverlay.css >+.button-toolbar.bookmark-item[disabled="true"]:hover:active >+{ >+ color : GrayText; >+ text-decoration : none; >+ list-style-image : url("chrome://communicator/skin/toolbar/bookmarks/bookmark-item-dis.gif"); >+ list-style-image : url("chrome://communicator/skin/toolbar/bookmarks/bookmark-item-dis.gif"); >+.menuitem-iconic.bookmark-item[disabled="true"] >+{ >+ list-style-image : url("chrome://communicator/skin/toolbar/bookmarks/bookmark-item-dis.gif"); You mean communicator/skin/bookmarks, not communicator/skin/toolbar/bookmarks, right?
Drat - I messed up the search and replace. I'll fix it after lunch. Gerv
Attached patch Patch v.5 (deleted) — — Splinter Review
OK. More comments on file locations: > + list-style-image : > url("chrome://communicator/skin/bookmarks/bookmark-folder-disabled.gif"); OK. > Index: themes/modern/communicator/bookmarks/bookmark-folder-disabled.gif Fine. in classic/jar.mn > + skin/classic/navigator/bookmark-folder-disabled.gif > (navigator/bookmark-folder-disabled.gif) er? That's not where that file lives according to the rest of the patch... same for modern/jar.mn Same for bookmark-item-disabled.gif Please attach an updated image archive; the seem to have moved... Also, please _do_ make the one perf change that has been proved to have a measurable benefit: "1. + for (var i = 0; i < metas.length; i++) { should be var length = metas.length; for(var i = 0; i < length; i++)" 10ms off load time is a definite improvement. Hyatt did a complete style system rewrite to shave off about 150ms.... Other perf stuff should be handled in a bug filed immediately after this is checked in.
Attached patch Patch v.6 (deleted) — — Splinter Review
<sigh> Where's the documentation on this stuff? I've changed what you said. I hope your review is comprehensive. I'm building now, as well as testing doing a file copy, to see if there's anything else. But, in the mean time, is it OK now? :-) Gerv
OK. I've been testing this now that it actually works. Looks good. One thing: There is a themes/modern/communicator/bookmarks/bookmark-item-dis.gif We are adding a themes/modern/communicator/bookmarks/bookmark-item-disabled.gif The linkToolbarOverlay.css for modern only references bookmark-item-dis.gif Is there a reason to add bookmark-item-disabled.gif? (It's needed for classic, but modern seems to have it in place already.) Sort that out and r=bzbarsky, assuming you and Tim are happy with it.
Comment on attachment 48968 [details] [diff] [review] Patch v.6 Patch v.6 doesn't contain the optimization var length = nodelist.length; for(var i = 0; i < length; i++) I'd really like to see that one in. Once that is done, I can, yes baby, say r=fabian :) Excellent work!
This patch can't be checked in as-is for security reasons: As a test, gerv says that a <link> to about:cache loads. It shouldn't. Ditto for file urls. (Although javascript:window.open("about:cache") doesn't, for some reason. This may just be hitting js security checks which are more strict, I guess.) CheckLoadUri needs to be called, and quick tests with gerv on irc show that the following untested pseudocode should work: var ssm = Compoents.classes[scriptsecuritymanager_contractid].getService(). QueryInterface(Components.interfaces.nsIScriptSecurityManager); try { ssm.CheckLoadUriStr(sourceURL, dest, 0); loadURI(dest); } catch (e) {} mstoltz? jesse? does that sound right?
One other code comment... I've been browsing with this and just ran into a problem with: +function getLinkElements() { + return getHeadElement().getElementsByTagName("link"); +} for XML documents getHeadElement() can well return null. Then this code throws an exception. So please check that the head element exists before calling its methods.
Should we even be doing anything with documents that aren't HTML? Afterall, a LINK element in your XML document might not have anything to do with the HTML LINK element.
For XML, to be safe, you'd need to be looking at localName == "link" in the XHTML namespace.
Will this patch bitrot with the <toolbarbutton> changes?
The copyright headers need to be changed to the MPL/GPL/LGPL triple license before this patch can be accepted: <http://www.mozilla.org/MPL/relicensing-faq.html> Portions are copyright Eric Hodel. So, Eric, do we have your permission to relicense the code? Please say "Yes" :)
You have my permission to relicense to MPL/GPL/LGPL. (I prefer BSDL, but...)
Mine also, obviously. If no-one else claims they contributed to this code, then we are away. Gerv
> for XML documents getHeadElement() can well return null Same thing for image documents... as well as text/css or text/javascript content which we show in the browser without creating a <head> We _do_ have a <head> for viewsource content loaded in the main browser, but that could change. So please make sure that you check that the head element is non-null
I give my permission to incorporate my auto-hide patch under the tri-license. (it doesn't seem like this will be in the initial version anyway, so I'll end up adding it myself, but it can't hurt to give extra permission) How about making getHeadElement() return {getElementsByTagName: function() {return []}} if there is no head element? :)
Do you need permission to relicense the images I made for the classic theme a while back? (I don't even know if you're still using them)
FWIW, Eric was the only person who copyrighted his contributed code. I assume it means he's the only one who needs to grant permission, though I'm certainly unqualified to make that judgement. The FAQ lists relicensing@mozilla.org as the email address to contact for granting permission, so perhaps they accept questions as well. They'll be more qualified to answer than the rest of us. > How about making getHeadElement() return > > {getElementsByTagName: function() {return []}} I'm not even sure what that code does. Does it temporarily redefine getElementsByTagName? If that is so, it can't return an empty array, it must return an empty nodeList. Near as I can tell, there's nothing in the DOM API for constructing your own nodeList. Besides, it'd be nasty hack, which I suspect is what you meant by the smiley emoticon. I also noticed, in addition to checking if getHeadElement returns null, getHeadElement should check if getElementsByTagName returns an empty nodeList before accessing item(0), i.e.: function getHeadElement() { var nodeList = window._content.document.getElementsByTagName("head"); return nodeList.length == 1 ? nodeList.item(0) : null; } The code works without the check, but it's bad form to request an index that's known to be out of bounds in some cases. Also, we shouldn't be doing anything with documents containing multiple HEAD elements, hence the explicit check for length == 1.
I'm going to wait until the toolbarbutton dust settles. Yes, the patch will need to be fixed. Comments on that are welcome. Could someone expand on Heikki's last comment, which I don't understand? > I assume it means he's the only one who needs to grant permission It doesn't mean that. People automatically have copyright on their contributions. > Image permissions Yes. If you could click the link to send the preformatted mail on the FAQ page, that would he helpful. > relicensing@mozilla.org That would be me :-) Gerv
> Could someone expand on Heikki's last comment, which I don't understand? Sure. For XML, in which random nodes could be called <link>, you need to check the following for a node in an xml doc: 1) the namespaceURI property is the XHTML namespace URI (http://www.w3.org/1999/xhtml) 2) the localName property is "link"
Last time I checked plain text and images were wrapped in HTML and therefore have an HTML DOM tree.
> Last time I checked plain text and images were wrapped in HTML and therefore > have an HTML DOM tree. Yep. And that tree has no <head> element. The document looks like this: <html><body><p><img/></p></body></html> or <html><body><pre>text</pre></body></html>
The example just given DOES have a head element, even though it is not explicit, it is implied. For example, this document has a head and body element, and also validates as HTML 4.01 strict: <title>A document</title> <p>hi That's it! document.getElementsByTagName('head') will return a valid node list with one item.
Tim: As I understand Javascript (and I could be wrong, because I hadn't tested that particular code) it would return an Object with only one property - getElementsByTagName - with a value that is a function that returns the empty list. It was taking advantage of the fact that JavaScript is more-or-less untyped, and the only thing that ever gets called on the result of getHeadElement is getElementsByTagName(). And you're right about the reason it wouldn't work - I didn't realize that it returned a NodeList rather than an array. And yes, it would be far too hacky to actually do that in practice :)
> For example, this document has a head and body element Jeffrey, there is a subtle difference here. The HTML you provide goes through the parser when we load it in Mozilla and the parser creates and inserts HEAD and BODY nodes. Image documents and the like are created explicitly. There is never any HTML source involved -- the appropriate code creates nodes by hand and inserts them in the document. As a result those documents do _not_ have a HEAD node in the DOM.
Apparently, <head> is an instance of HTMLHeadElement even when it appears in an XHTML document parsed as XML. I'm working on a version that doesn't differentiate between HTML and XHTML but relies on instanceof HTMLHeadElement.
I rewrote doRefresh() to access linkToolbarHandler.handle() directly. This implementation works with text/html and XHTML parsed as XML. It does minimal tree walking. (But I haven't benchmarked it.) Thanks to jag and bz.
Comment on attachment 50279 [details] linkToolbarOverlay.js that works with XHTML parsed as XML The first version had a bug in it. :-(
Attachment #50279 - Attachment is obsolete: true
Attached patch Patch v.7 (deleted) — — Splinter Review
OK, so the new patch has the menubuttons eliminated, the security problems fixed, Heikki's XML document change, the relicensing changes, and smells minty fresh too :-) Looking for re-review (as far as I remember, the big changes are in linkToolbarOverlay.js and linkToolbarOverlay.xul; there are tiny ones in linkToolbarHandler.js). Let's go, people, before they change XUL again! Gerv
window.content.location.href is currently not a secure way to get the source URL. A page could set a getter on window.location to make it return the object {'href':'chrome://navigator/content/navigator.xul'}. I don't know what the right way is, or if there even is one at this point. See bug 36946. You should also make sure that a link pointing to a data: URL yields a page with the same principal as the linking page. To check whether data urls inherit principal correctly, make sure - the data: page gets a security error when it tries to access Components.classes. - the data: page can open a URL at the site that linked to it, and then read data in that window. - the data: page can't open a URL at *another* site and do the same thing.
I talked this problem through with Mitch; he says that he will look into it early next week, but that it shouldn't hold up getting it checked into the tree (as long as it's not on the 0.9.4 branch ;-). He mentioned something about using XPConnect to get the Document object and getting the current URL from there... Review! Review! :-) Gerv
> He mentioned something about using XPConnect to get the Document object Nope. No good. All the idl-ized document interfaces we have are the DOM ones -- there are no scriptable internal interfaces. This means that your only options for getting the URL as nsIDOMNSDocument::location.href or nsIDOMHTMLDocument::URL The latter suffers from the problem that it's only defined for HTML documents. The proper solution, of course, is to idl-ize nsIDocument and not expose it to unpriveleged scripts... But that's not happening near-term. Gerv, I'll try to take a look at this patch this weekend (most likely tomorrow morning).
+ var ssm = + Components.classes["@mozilla.org/scriptsecuritymanager;1"].getService(). + QueryInterface(Components.interfaces.nsIScriptSecurityManager); This should probably be wrapped in a try/catch... at the very least, the QI can fail. I got a warning for line 262 in LinkToolbarItem.js -- "Anonymous function does not always return a value" Other thank those, looks good to me. Fix those and get review from the security people.
I'll put it inside the try/catch that I use the returned value in on the next line - easy. The security people are fine with this at the moment; I did some tests over the phone with Mitch. Gerv
Attachment #51333 - Flags: review+
Comment on attachment 51333 [details] [diff] [review] Patch v.7 Gerv says he'll add 'return true;' to all the functions that need it to quiet that warning. With that, r=bzbarsky
One can also use var ifrq = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor); var uri = ifrq.getInterface(Components.interfaces.nsIWebNavication).currentURI; to get at the current URI if you have full XPConnect priveleges. This is not 100% bullet proof either, but it's less likely to be wrong than using location.href
Please use the method that jst just mentioned for getting the document URL. It's a bit safer.
Comment on attachment 51333 [details] [diff] [review] Patch v.7 * Brackets and spacing in modern's linkToolbar.css are not using the conventions used in other files throughout the modern theme. Please make it look like other files. * You are setting type attribute on some menuitems (in linkToolbarItem.js and linkToolbarOverlay.xul). This attribute is reserved for XUL, so please qualify it with another namespace (perhaps rdf?). * The function names in linkToolbarOverlay.js are somewhat generic ("clicked", "clear", "doRefresh"), and should be qualified by putting them on an object prototype encapsulating link Toolbar functionality * Many of the new Javascript files create fake js "classes" by adding functions to the object inside its constructor. This should be done more efficiently using object prototypes. * On this line: + switch(document.getElementById("link-" + linkType).tagName) { ...you should use localName instead of tagName. * The CSS styles you added for disabled bookmark-items in linkToolbarOverlay.css should be moved into bookmarksToolbar.css * In linkToolbarOverlay.css, GrayText is not the right color for disabled text in the Modern theme. Use #9399AB instead. * In linkToolbarOverlay.css, you don't need to specify text-decoration or color in the rules for toolbarbutton.bookmark-item[disabled="true"][container="true"], etc, since they will pick that up from the rule above. * Do not import chrome://global/skin/toolbar.css from linkToolbarOverlay.xul * Your image file names aren't using the naming conventions prevalent in the Modern theme. Rename all the buttons to say -hov instead of -hover and -dis instead of -disabled. * In the modern theme, please move all the button images into navigator/btn1. * Rename linkToolbarOverlay.css to linkToolbar.css to be consistent with personalToolbar.css.
Attachment #51333 - Flags: needs-work+
Grr. :-) OK, I've done all of this except: * The function names in linkToolbarOverlay.js are somewhat generic... and * Many of the new Javascript files create fake js "classes".. which are beyond my JS-fu. If Hewitt requires either of these for super-review, one of the original patch authors is going to have to step in here. The alternative is this patch sitting here for _another_ milestone. :-( Gerv
Hewitt reformatted the relevant files (thanks, hewitt!) and says: sr=hewitt. Checked in. My apologies to Eric and Tim; in my rush, I forgot to credit them in my checkin comment. However, as their names are all over the source, those in the know will know. :-) Congratulations to all involved. Any problems with this feature should be filed in a _separate_, short, manageable bug, probably CCing the three of us. We have a commitment to make it hidden by default for 0.9.5; if someone can come up with working autoshow code really quickly (in _another_ bug ;-) we might take that, otherwise it'll just be turned off by default, which would be a shame. Who has autoshow code? :-) Gerv
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
OK, so I suck even more. Thanks and well done to Chris, too - and anyone else I may have forgotten. Everyone over to bug 102832! <hides in corner.> Gerv
Depends on: 102899
Depends on: 102886
Removing dependancy on bug 102899. This bug is fixed. A fixed bug cannot depend on on an open bug. Bug 102899 is an enhancement related to the functionality introduced by the fixing of this bug.
No longer depends on: 102886, 102899
Sorry, I got bug 102899 and bug 102832 mixed up. In any case, this bug is fixed can't depend on an open bug. Bug 102899 is a problem with the current implementation. It doesn't block this bug. Apologies for the spam everyone. The laws of probability state that one of these days I'll make a useful contribution to Bugzilla!
Depends on: 102896
Depends on: 102915
Depends on: 102832
Depends on: 102895
Depends on: 102897
No longer depends on: 102896
No longer depends on: 102895
Removing dependancies (again). Bug 103053 is now used to track Link Toolbar improvements.
No longer depends on: 102832, 102897, 102915
This checkin is being blamed for the large degradation in performance yesterday. See <news://news.mozilla.org/3BBBFBD0.8CA40B45@netscape.com>. If this is really the case, I'm going to lobby for this being backed out until the performance ramifications are understood.
It's already been backed out and I'm working on a fix that would at least eliminate the performance penalty when the toolbar is disabled. See bug 103082 for more details.
Now that I had a chance to look at it as a live implmentation from a UI design perspective I find this toolbar very unsuitable to aid navigation because it looks like part of the chrome rather then part of the page. This is really a geek feature for now and very confusing looking to a consumer. It seems very confusing to have two essentailly to similar looking layers of navigation and is sure to get confused with history (which is already over the head for most of Netscape's customers) I believe that a much better implimentaton would be to think about how to make it visually part of the page rather then part of the chrome (like e.g a frame the drops down or a sidebar panel aka sitemap).
German, I agree with your statement about the UI, but why don't we get the backend working and not be backing things out every three seconds before we worry about what percentage of the population can use it. We will need the same backend code in a sidebar panel or a toolbar.
German, this is exactly how a UI for 'link rel="..."' and 'link rev="..."' should look like. Please go back to the very start and read the basic article <http://www.euronet.nl/~tekelenb/WWW/LINK/> if you don't understand the benefit of a standardisized site navigation!
This bug is _DONE_. We have a UI for the <link> element, if you dislike the UI, then please file a new bug for that (against this component) and have it block http://bugzilla.mozilla.org/show_bug.cgi?id=103053
Status: RESOLVED → VERIFIED
German, an RFE has been filed to move the toolbar into the frame.
Blocks: 103053
*** Bug 68419 has been marked as a duplicate of this bug. ***
Shouldn't links in this form also be supported by the toolbar? <a rel="next" href="foo.html" title="Next document">Next</a> ie. a link in the body rather then the head of the document. Also the language is displayed as English if the link has hreflang="en" but not hreflang="en-gb", which is valid for UK English.
This is not the correct bug to make these comments in :-) Both of these issues are known. The language one depends on switching over to use the decent language-parsing code I wrote for Page Info, and the <a href=> one was (as you will find out if you read this bug) removed for performance and semantic reasons. I personally am in favour of putting it in, but it is a perf. hit. Gerv
> and the <a href=> one was (as you will find out if you read this bug) > removed for performance and semantic reasons. I personally am in favour > of putting it in, but it is a perf. hit. Someone could create a seperate bug for this enhancement. At a minimum it would be marked WONTFIX for perf reasons, providing a convienient target for the dupes that are bound to come in. However, bug 102992 may solve the perf problems and make the enhancement possible, in which case we would resume the debate on semantics in the new bug. I'm opposed to the enhancement, so I'm only providing friendly advice...someone else will have to create the bug if they want it ;-)
Component: User Interface Design → Browser-General
Product: Browser → Seamonkey
Alias: LinkUI
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: