User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030719 Mozilla Firebird/0.6 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030719 Mozilla Firebird/0.6 In both a local xml file or one sent with application/xhtml+xml, middle clicking on a link brings up the autoscroll icon instead of opening the link in a background tab. Reproducible: Always Steps to Reproduce: 1. Load an XML file. 2. Middle click on a link Actual Results: Autoscroll icon appears over link. Expected Results: Should open link in background tab. Maybe this bug should depend on bug 212273 if using the All-in-One autoscroll fixes this.

Confirming on W2K with 20030720 build with a fresh profile. If you middle-click a link for the first time the autoscroll icon will appear. If you then middle-click a second time, Firebird will open the link in a new tab as requested.

also on http://www.phoenity.com >>If you then middle-click a second time, Firebird will open the link in a new tab as requested. I only get images

*** Bug 213463 has been marked as a duplicate of this bug. ***

The All-in-One autoscroll is affected by this bug as well. It's not affected in old builds like 0.6. The patch from bug 212273 doesn't fix this for me. Using AiO 0.8 on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030721 Mozilla Firebird/0.6

-> major

Not sure if this should be a separate bug, but autoscroll also causes pretty-printed xml to reappear as plain text. Build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030721 Mozilla Firebird/0.6 Steps to reproduce: 1. Load XML page such as http://bugzilla.mozilla.org/attachment.cgi?id=117944&action=view 2. Middle click on the page Results: All xml formatting and tags dissappear. This happens with built-in and All-in-One autoscroll. All-in-One mouse guestures also cause the same behavior. I assume it's all related.

The reason for this bug is pretty trivial: XHTML has a lowercase "a" tags, where HTML has uppercase "A" tags. I ran into a similar issue with MozGest's trails code. From browser's isLink method ( http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/browser.xml#527 ) 527 if (node.nodeName == "A" || node.nodeName == "INPUT" || node.nodeName == "TEXTAREA" || 528 node.nodeName == "AREA") { 529 return true; 530 } Example fix: var tagName = node.nodeName.toUpperCase(); if (tagName == "A" || tagName == "INPUT" || tagName == "TEXTAREA" || tagName == "AREA") { return true; } I'd suggest someone smarter than me makes a nice patch file from the above so this bug can get the has-patch keyword.

Patch based on the code in comment 7. Untested by me (since I don't have a 3-button mouse) except to say that it didn't bust my Windows build when I recomplied with it. I basically put it out there to give people a chance to run with it and test it.

Works fine with the test url as well as a local xml file on WinXP. I just edited browser.xml inside toolkit.jar.

*** Bug 218682 has been marked as a duplicate of this bug. ***

The provided URL is no longer served as XML, and thus can't be used for testing. Attaching a simple testcase.

Comment on attachment 131033 [details] [diff] [review] possible patch Setting a requestee might speed up the review process... Blake, please have a look. It's short, simple and it works.

Comment on attachment 131033 [details] [diff] [review] possible patch Dean, can you take a look at this. This is pretty visible and since Blake seems to be pretty inactive ATM I thought I might ask you.

Comment on attachment 131033 [details] [diff] [review] possible patch Sure, I'm fine with this.

I'm not allowed to request flags, but I'd suggest asking either alecf@flett.org or jag@tty.nl for sr, as they're mentioned on the super-reviewers list for XPFE.

Jens, this bug does not affect XPFE-applications. It only affects apps using the new toolkit, so we don't need super-review at the moment. I will ask Dean to check this in. And Jens, if you want additional permissions, go to http://www.gerv.net/hacking/before-you-mail-gerv.html look for the "I'd like my permissions upgraded on bugzilla.mozilla.org."-entry and see if you fulfill all the necessary requirements.

pmsyyz@yahoo.com, stop marking bugs as blocking0.8+. This is a developer-only option. If you continue doing this, your account will be suspended for bugzilla abuse.

checked in

Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031024 Firebird/0.7+

This patch is not enough to solve the real problem. The test needs to verify that the element is an (X)HTML element as well. Therefore we need to test the namespaceURI as well as the localName. This is a bit tricky since the namespaceURI is null when no namespace is available and this happens in both HTML documents as well as in some XML+CSS documents. However there is hope here. One can use instanceof HTMLElement to ensure that it is an HTML element instead. var tagName = node.localName.toUpperCase(); var xhtmlNs = "http://www.w3.org/1999/xhtml"; if ((node.namespaceURI == xhtmlNs || node instanceof HTMLElement) && (tagName == "A" || tagName == "INPUT" || tagName == "TEXTAREA" || tagName == "AREA") ) { return true; }

>This patch is not enough to solve the real problem. The test needs to verify >that the element is an (X)HTML element as well. Just so we all understand, am I correct in assuming that this is because someone could write a new XML language Foo which defines an <A> element that isn't a link and then create a DTD which is XHTML+Foo, possibly creating a situation where middle-clicking on the <A> element results in no scrolling?

Yes. I should have made that clear. Just because the name is "a" or "A", it does not mean that the element is an HTML link.

If you start to get so anal, then the whole line: var tagName = node.localName.toUpperCase(); is incorrect for checking when it comes to XML. Besides it should have been .toLowerCase() just to be consistent with HTML -> XHTML tendencies.

I also don't see why <input> and <textarea> are considered as links? The proper way of doing this would be: if (!node) return false; for (var i = 0; i < document.links.length; i++) { if (document.links[i] == node) return true; } return this.isLink(node.parentNode); This should apply to both HTML, XHTML, and should not pose any problems with any other XML language. However it might be a performance hit on pages that have hundreds of links on them. Someone test this?

Dean, despite your checkin this is still not fully fixed. We have suggestions for a better patch in comment 20, comment 23 and comment 24. Please take a look at it.

INPUT and TEXTAREA were already handled in the existing code. The idea in comment 20 sounds close. Don't we have a constant for xhtmlNs somewhere that we can use, instead of re-declaring it? Actually, looking at pageInfo.js and metaData.js, we don't. Anyone want to write and test a patch?

The only thing that isLink method is used for is to determine wether to start autoscroll or not: http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/browser.xml#638 Which explains why <input> and <textarea> are included (we don't want to autoscroll when middle clicked inside a textarea) so the name of the method doesn't really reflect it's function and purpose, and should be changed as well.

> we don't want to autoscroll when middle clicked inside a textarea. I do. I either middle-click to open a link in a new tab, or to use autoscroll. Well, I don't want to open textareas in a new tab, so why shouldn't it call autoscroll? I don't want to move around textareas before starting autoscroll.

so which would you expect to scroll? the contents of textarea? or contents of whole viewport? Middle clicking is used for pasting text in X-Windows and is much more important than scrolling.

It should scroll the contents of whole viewport, so that I don't have to move the cursor outside the textarea to autoscroll the page. This is the behaviour of the All-in-One extension.

So this needs to check for middlemouse.paste being true as well. It is on by default in Linux, but you can turn it on for other platforms.

This patch is based on the solution proposed by Erik in comment 20. It also incorporates Steffen's suggestion that autoscroll occur when middle-clicking form elements, but only when middlemouse.paste is not enabled as alanjstr pointed out. It does not address the issue where autoscroll disables XML prettyprint, but that seems to be a separate bug.

Comment on attachment 137660 [details] [diff] [review] Proposed patch Asking dean for review since he did the last version of this. I believe this patch addresses all the issues raised with the previous one, and it's pretty straightforward. Is this too late to land for 0.8?

Just to add some more confusion; In All-in-One extension, I use the following test code that seems to work flawlessly: if (nodeNameLC == "a" || nodeNameLC == "area" || domNode.hasAttributeNS(xlinkNS, "href")) return domNode; where nodeNameLC = domNode.nodeName.toLowerCase(); and const xlinkNS = "http://www.w3.org/1999/xlink";

Comment on attachment 137660 [details] [diff] [review] Proposed patch 1. The method name should be changed. It doesn't reflect it's function, since <input> and <textarea> are not links, and makes the analysing the code a quite confusing task. name it something like isAutoscrollBlocker() ? can you come up with a better name? 2. toLowerCase() should be used instead of toUpperCase() in the spirit of XHTML. 3. The variable should be named xhtmlNS, not xhtmlNs to be consistent with the way these are named in DOM. === On the being anal note, this code is still incorrect as it disregards the case of the element name in XML. If in future XHTML 6.0 for example has 2 different elements <a> and <A>, and only one of them is a link, the code will behave incorrectly.

Comment on attachment 137660 [details] [diff] [review] Proposed patch I agree with your nits alexey, I can work those in easily. I'm not completely sure how to implement your last point, however. The example you provided earlier would certainly work for links, but not for textareas and inputs (if it is indeed decided that clicking them should trigger scrolling when paste is disabled). I just reviewed the in-progress XHTML 2.0 spec and you're right, this function isn't particularly future proof. For example, in XHTML 2.0 pretty much everything can have an href attribute and function as a link. Would some kind permutation of Mar's code be best? Something along the lines of if ((node.namespaceURI == xhtmlNs || node instanceof HTMLElement) && (tagName == "input" || tagName == "textarea" || node.hasAttribute("href"))) Finally, is the namespace check correct? What if the document is XHTML 1.1 or greater? Thanks for your comments so far everyone, they've been insightful.

Yes, all current XHTML specs use the same namespace. XHTML 2 however will have a different one. So I guess we can not worry for now about scenario I descrived in the code above. But to fix it, the best way would be to have separate relaxed check for HTML tag soup, and a strict one for XHTML/XML nodes. if(node.namespaceURI == xhtmlNS) { if (node == "a" || node == "area" || (middleMousePaste && (node == "input" || node == "textarea")) return true; } else if (node instanceof HTMLElement) { var tagName = node.nodeName.toLowerCase(); if (tagName == "a" || tagName == "area" || (middleMousePaste && (tagName == "input" || tagName == "textarea")) return true; }

*** Bug 220756 has been marked as a duplicate of this bug. ***

we're almost at RC status, this really is a good thing but I don't think we have enough timeframe to work out the nits before an RC

Back from the holidays with a patch that (hopefully) addresses the remaining issues people raised. If the markup is valid XHTML 1.0/1.1, it will check specifically for lowercase "a" tags, otherwise either case is acceptable (this is essentially the code Alexey posted with minor fixups). While it's true that this isn't exactly future-proof for new versions of XHTML, there isn't really a way I can think of to do that until the standards are actually ratified. This is definitely better than the previous code, since at least it works for the testcase and everything else I could find to try it out on.

Comment on attachment 138308 [details] [diff] [review] Patch v1.1 Could you please have a look at this, Pierre?

Comment on attachment 138308 [details] [diff] [review] Patch v1.1 looks good except for 33 trailing space characters in 2 lines. Once XHTML 2 spec comes out, it will have a different NS string, and we'll need rename this variable to xhtml1NS and add xhtml2NS varaible. But that will become an issue years from now.

Comment on attachment 138308 [details] [diff] [review] Patch v1.1 I haven't looked through this patch in detail yet, but I have a couple of comments/nits... - <method name="isLink"> + <method name="isAutoscrollBlocker"> <parameter name="node"/> <body> <![CDATA[ if (!node) return false; Since you're changing the meaning of the function, shouldn't it now return true if there's no node? + if(prefs.getPrefType("middlemouse.paste") == prefs.PREF_BOOL){ + middleMousePaste = prefs.getBoolPref("middlemouse.paste"); + } You don't need "{" or "}". + if(node.namespaceURI == xhtmlNS) { + if(node.nodeName == "a" || node.nodeName == "area" || + (middleMousePaste && (node.nodeName == "input" || node.nodeName == "textarea"))) You can combine this into a single "if". + else if(node instanceof HTMLElement) { And then you don't need the "else" after the "return". >+ var xhtmlNS = "http://www.w3.org/1999/xhtml"; This could be a "const" declared at the beginning of the method. >+ if(node.namespaceURI == xhtmlNS) { You need spaces between "if" and "(" all through this patch. + var tagNameLC = node.nodeName.toLowerCase(); I like the original var name of tagName instead of tagNameLC.

Thanks for getting to this so quickly. I'll fix up those nits. One thing though...changing the initial check to return true rather than false causes auto-scroll to not work *at all*, so that's probably not the right solution. I don't believe I've really changed the meaning of the method, per se, so much as made it correct. It was not really checking whether the target node was a link, but rather whether clicking it should prevent autoscroll from happening. Is the problem that the new name is also unclear/inaccurate? Everything else I understand, and I'll fix that up.

Comment on attachment 138308 [details] [diff] [review] Patch v1.1 the meaning of function was not changed, just the name was corrected. No return changes needed. The 'else' is still needed, we don't want the second group of checks to apply to XHTML, those are for HTML tag soup only. I think grouping into that separate xhtmlNS 'if' check simplifies things and improves code readability. Putting it together would make it messy and harder to understand: if (node.namespaceURI == xhtmlNS && (node.nodeName == "a" || node.nodeName == "area" || (middleMousePaste && (node.nodeName == "input" || node.nodeName == "textarea")))) Also the checks inside are exactly the same except for the case handling, and this can be clearly seen when grouped as per patch, but harder to see if everything ti shoved together. I agree with all other nits. Jon, could you please rename the xhtmlNS const into xhtml1NS just to be ready for coming of new XHTML 2 Name Space.

> The 'else' is still needed, we don't want the second group of checks to apply > to XHTML, those are for HTML tag soup only. I think I see that now, yes.

Here's yet another patch that I think addresses the various points raised in comments. All of Dean's nits, minus changing the value returned at the top of the function and eliminating the "else" statement are addressed. I also changed a variable name from xhtmlNS to xhtml1NS in (distant) anticipation of a new XHTML spec. Finally, I overlooked the fact that the work of getting a handle to the pref service was already done, so that redundant code was cleaned up. I'd ask Dean for the review, but am I right in thinking you are not a Firebird peer? I'll ping someone else for the time being I guess.

That was just what I was going to say. I used to be one of the chosen few, but I haven't been very involved lately. But I can still give an opinion. The patch looks pretty good. The only thing that I see is the indenting of the try/catch block is off.

hmmmm readability of the first 'if' has definitely suffered.

Autoscroll should also be blocked when clicking on the scrollbar, at least if the pref middlemouse.scrollbarPosition is set to true. If that pref is set to true, middleclicking on the scrollbar directly scrolls to the clicked position, not just one page up or down. Autoscroll shouldn't kick in afterwards. The AiO extension does this already.

That's so not this bug. Try bug 218686 (currently marked as a dupe but I don't think it should be).

Comment on attachment 138663 [details] [diff] [review] Patch v1.2 Blake, other than Dean's nit about the indenting on the try/catch, this looks pretty good to me

Comment on attachment 138663 [details] [diff] [review] Patch v1.2 >+ var middleMousePaste; >+ >+ try { >+ middleMousePaste = this.mPrefs.getBoolPref("middlemouse.paste"); >+ } >+ catch(ex) { >+ } Seems like middleMousePaste should have a default boolean value, or we should set it in the catch. Also, this indentation needs to be fixed. >+ >+ if (node.namespaceURI == xhtml1NS && (node.nodeName == "a" || node.nodeName == "area" || >+ (middleMousePaste && (node.nodeName == "input" || node.nodeName == "textarea")))) >+ return true; >+ >+ else if (node instanceof HTMLElement) { >+ var tagName = node.nodeName.toLowerCase(); >+ >+ if (tagName == "a" || tagName == "area" || >+ (middleMousePaste && (tagName == "input" || tagName == "textarea"))) >+ return true; Maybe I'm missing something but...why can't we just say something like: if (node.namespaceURI == xhtml1NS || node instanceof HTMLElement) { var tag = node.nodeName; if (node instanceof HTMLElement) tag = tag.toLowerCase(); if (tagName == "a" || tagName == "area") return true; if (middleMousePaste && (tagName == "input" || tagName == "textarea")) return true; } i.e. don't duplicate the code, just lowercase the tag in the case of an html element (I also broke out the middleMousePaste case for easier readability). Is this valid or am I missing something? If it is, please post a new patch for review.

I agree that's a bit easier to read. The code was actually originally more like that, but on Dean's advice I combined all the if statements. I have no problem creating another patch later this afternoon though, whichever one you think makes it clearer what's going on is fine with me.

> if (node.namespaceURI == xhtml1NS || node instanceof HTMLElement) { > var tag = node.nodeName; > if (node instanceof HTMLElement) > tag = tag.toLowerCase(); Don't we need to be case-sensitive for xml?

Are (In reply to comment #55) > Don't we need to be case-sensitive for xml? We are doing this using case sensitive tag name comparison. We only get in here if we have an (X)HTML element and we only change the case if it is an HTML element. Are you afraid of elements created as createElementNS(xhtml1NS, "A") or when do you think there is a problem?

> Don't we need to be case-sensitive for xml? yes, instead it should be: if (node.namespaceURI != xhtml1NS) tag = tag.toLowerCase();

and actually we don't need the initial check. it should be: if (node instanceof HTMLElement) { var tag = node.nodeName; if (node.namespaceURI != xhtml1NS) tag = tag.toLowerCase();

God willing, this is the final verison of the patch *knocks on wood*. I've incorporated Blake's review comments and Alexey's suggestion as to how to further simplify the if statement.

This latest patch looks good to me. Alexey Chernyak, you're clearly the expert here--could you sign off on it too?

> + if (tagName == "a" || tagName == "area") > + return true; It should check for an href attribute so named anchors (or other non-link <a> elements) don't block autoscroll.

Nice catch there, Jesse! You always had an eye for hidden quirks. Waddya know, this aint last version afterall. :o)

Reassigning to jhenry@ccs.neu.edu.

Ack! So much work one one little patch :) I guess that's why we have the review process though, to keep newbies like me from gumming up the works with half-baked ideas. This patch makes sure only anchors with href attributes are considered blockers.

pfft, shows how much I know. Jesse, Alexey: how's this one look to you?

Comment on attachment 141715 [details] [diff] [review] Next patch well, for the final touch it should be: if ((tagName == "a" || tagName == "area") && node.hasAttribute("href")) return true; the href= attribute is not REQUIRED on <area> either. While it is unlikely that someone would use <area> just for named reference, that's the way to do it for completeness sake.

Alexey's change is correct -- <area> does not act as a link unless it has an href attribute. I'm ok with the patch with alexey's change.

Here's the requested change.

Comment on attachment 141772 [details] [diff] [review] Another day, another patch This may finally be ready to go... (famous last words).

Fix checked in (I fixed one more small nit to only declare the NS in the scope it's needed). Thanks for the patch, Jon!!

From what Erik already said in comment 20: If this line: var tagName = node.nodeName; would be changed in: var tagName = node.localName; then it would also work for links who have a prefix (most likely <html:a href...) See upcoming testcase, which doesn't work now.

Martijn, this has been addressed in patch for bug 218686.

Bug still exists in Firefox 20040411, in an XML page transformed to XHTML (left-clicking the links works, so I don't think it's duff code on my part) http://purl.org/mooquackwooftweetmeow/weblog#029 - middle-click "mint"

Greg, as I just said above, this has been addressed in patch for bug 218686.

Has anyone checked this situation with XLink? (specification at http://www.w3.org/TR/xlink/) It's just that the error still occurs with links on http://www.mozilla.org/university/hof.xml Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

http://www.mozilla.org/university/hof.xml - WFM Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051224 Firefox/1.5

Alexey- You get a new tab if you middle click on a link (for say Epiphany) with tabs set to open new links in a new tab? Maybe it's only a Windows problem then.