359183 - Clicking a link to an anchor opens a browser window instead of scrolling.

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[sandoz]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20060601 Firefox/2.0 (Ubuntu-edgy)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20060601 Firefox/2.0 (Ubuntu-edgy)

When I view a RSS news item in the RSS windows and click on a link to an anchor (e.g. <a href="#bottom">bottom</a>), Thunderbird opens a browser window/tab (e.g. Firefox) and shows the news website.

Suggested behaviour:
When clicking on a link to an anchor, Thunderbird should scroll to that anchor instead of invoking a new browser windows (or tab).
When clicking on a external link, thunderbird should open a new browser window/tab like it does now.

Reproducible: Always

Steps to Reproduce:
1. Subscribe to an rss feed (like http://ati.amd.com/online/rss/atilinuxdriver.rss)
2. View one of the news items.
3. Click on a link to an anchor (like "Web Content")

Actual Results:  
Opens a new browser window/tab.

Expected Results:  
Scroll in the Thunderbird RSS message window.

Comment 1

[Nikolay Shopik]

no reporter = no bug

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> no reporter = no bug
> 

Reopening, there are enough steps here that this should be able to be tested.

Comment 3

[Riccardo]

I confirm the bug, and I add: when I come back from a tab to the main window the feed reloads, so I loose the line I was reading

Comment 4

[melchior blausand]

Confirm.
Regularly keeps me from clicking>reading into the body of my favorite german rss-digest nachdenkseiten.de

Comment 5

[alta88]

Attachment: Hinweise des Tages.eml

Example message with lots of anchors. In a feed folder, going to the web page also tests lots of anchors.

Comment 6

[alta88]

Attachment: anchor.patch

This makes clicking anchor links work in mailnews messages and web pages in messagepane/content tab. Jorg K, if this isn't your purview, please direct to magnus.

The test message works in local folders and when copied to imap. It likely also fixes Bug 1184811.

Bug 974857 has some backstory on anchors in Tb.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8785724 [details] [diff] [review]
anchor.patch

(In reply to alta88 from comment #6)
> Jorg K, if this isn't your purview, please direct to Magnus.
I'd have to read up on it. I can still do that if Magnus doesn't get to it.

Comment 9

[alta88]

An alternate way to do this would be to go through document.links and adjust the anchors in onMsgLoaded or such. This would prevent the http url from showing in the status bar on mouseover.  The url could also be added to history to show visited state.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8785724 [details] [diff] [review]
anchor.patch

I'm taking this review after all. Code looks good and I'll test it in the next few hours.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8785724 [details] [diff] [review]
anchor.patch

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

I have some questions. Also, unrelated, how do you set the Content-Base: header to set the website information? I used an editor to create a test message.

About name vs. id, see http://stackoverflow.com/questions/484719/html-anchors-with-name-or-id:
There it says that an <a> can't even carry a name attribute in HTML5.

::: mail/base/content/contentAreaClick.js
@@ +34,5 @@
> +      // We have a ref fragment which may reference a node in this document.
> +      // Ensure html in mail anchors and web page anchors work as expected.
> +      let anchorId = target.hash.replace("#", "");
> +      // Return if an id (html5) or name attribute value doesn't match the ref.
> +      let selector = "#" + anchorId + ", [name='" + anchorId + "']";

Should we also test for id=?
I know websites, where the reference is done via id, for example:
www.henk.com.au/upcoming, look for |tg2016| in the page source and you'll see:
<a href="upcoming#tg2016">
<a id="tg2016"></a>

@@ +45,5 @@
> +
> +      // Finally, if the document url is a message url, and the anchor href is
> +      // http, it needs to be adjusted so docShell finds the node.
> +      let messageURI = makeURI(target.rootNode.URL);
> +      if (messageURI instanceof Components.interfaces.nsIMsgMailNewsUrl)

Is there an else? Can there be a case where messageURI is not an instance of Components.interfaces.nsIMsgMailNewsUrl? What happens then? We still return null?

@@ +47,5 @@
> +      // http, it needs to be adjusted so docShell finds the node.
> +      let messageURI = makeURI(target.rootNode.URL);
> +      if (messageURI instanceof Components.interfaces.nsIMsgMailNewsUrl)
> +      {
> +        if (href.startsWith("http"))

Is there an else? What happens if we find a ftp:// URL or some other protocol?

Comment 12

[alta88]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #11)
> Comment on attachment 8785724 [details] [diff] [review]
> anchor.patch
> 
> Review of attachment 8785724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some questions. Also, unrelated, how do you set the Content-Base:
> header to set the website information? I used an editor to create a test
> message.

Content-Base is set for feeds internally to mean the rss/atom spec permalink url. It could theoretically exist in an email and be anything, but in Tb15 I made any feed message carry a feed flag, so there is a way to distinguish.

> 
> About name vs. id, see
> http://stackoverflow.com/questions/484719/html-anchors-with-name-or-id:
> There it says that an <a> can't even carry a name attribute in HTML5.
> 

Right, I think pre html5 should also be supported. The selector string below will find both forms.

> ::: mail/base/content/contentAreaClick.js
> @@ +34,5 @@
> > +      // We have a ref fragment which may reference a node in this document.
> > +      // Ensure html in mail anchors and web page anchors work as expected.
> > +      let anchorId = target.hash.replace("#", "");
> > +      // Return if an id (html5) or name attribute value doesn't match the ref.
> > +      let selector = "#" + anchorId + ", [name='" + anchorId + "']";
> 
> Should we also test for id=?

Yes of course, and we are, as that's html5. #tg2016 is the form for |id| attrs for querySelector()..

> I know websites, where the reference is done via id, for example:
> www.henk.com.au/upcoming, look for |tg2016| in the page source and you'll
> see:
> <a href="upcoming#tg2016">
> <a id="tg2016"></a>
> 
> @@ +45,5 @@
> > +
> > +      // Finally, if the document url is a message url, and the anchor href is
> > +      // http, it needs to be adjusted so docShell finds the node.
> > +      let messageURI = makeURI(target.rootNode.URL);
> > +      if (messageURI instanceof Components.interfaces.nsIMsgMailNewsUrl)
> 
> Is there an else? Can there be a case where messageURI is not an instance of
> Components.interfaces.nsIMsgMailNewsUrl? What happens then? We still return
> null?
> 

It will be decided later, and there is no change from how it currently works. This test is only so the url can be rewritten and make docShell skip to the id/name in the same page, ie not to open externally.

> @@ +47,5 @@
> > +      // http, it needs to be adjusted so docShell finds the node.
> > +      let messageURI = makeURI(target.rootNode.URL);
> > +      if (messageURI instanceof Components.interfaces.nsIMsgMailNewsUrl)
> > +      {
> > +        if (href.startsWith("http"))
> 
> Is there an else? What happens if we find a ftp:// URL or some other
> protocol?

There shouldn't be any change, as per above, ie contentAreaClick() would still return true.

All that said, I'm actually thinking the better way to do it would be comment 9. It proposes rewriting all such links at load time, rather than individually at click time.  Any opinion one way or another?

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Another test message.

Can you check why my example doesn't work. Please click on "Tony Gould" under the image. It will open the website where it shouldn't. Or I created a bad example.

(In reply to alta88 from comment #12)
> Right, I think pre html5 should also be supported. The selector string below
> will find both forms.
I will try that once my example works.

> > Is there an else? Can there be a case where messageURI is not an instance of
> > Components.interfaces.nsIMsgMailNewsUrl? What happens then? We still return
> > null?

> It will be decided later, and there is no change from how it currently
> works.

Hmm, I don't follow you here. You unconditionally return null whether or not your modified the URL. Without your patch the flow falls out of the if/else if/else and returns href, never null.

I believed your comment:
// Return null to allow the click to go to the anchor in this docShell.
but you return null even if you don't modify the URL and you clearly don't go to any anchor in the docShell.

> All that said, I'm actually thinking the better way to do it would be
> comment 9. It proposes rewriting all such links at load time, rather than
> individually at click time.  Any opinion one way or another?

Yes, I think rewriting message content at load time is fiendishly difficult.

Comment 14

[alta88]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #13)> > All that said, I'm actually thinking the better way to do it would be
> > comment 9. It proposes rewriting all such links at load time, rather than
> > individually at click time.  Any opinion one way or another?
> 
> Yes, I think rewriting message content at load time is fiendishly difficult.

Rather than going into why the procedural null/href return makes no difference, I will note that your example proves that the click method is a fail - it fails on the img (since there's no href) and on the BOLD name (since the element is <strong>, with no href, a child of <a>).  It would work if you clicked CD/DVD text..

So the href has to be changed before the click event is processed (or else walk the dom tree up to find the <a> - I don't want to do that).

Why do you think changing a dom document's link node href's is difficult? I can't see that at all - we do a similar and harder thing for image fitting. The gecko parser even gives us an iterable called links and all we do is test for the link node having a hash property and updating the href with the real document url (the msgmailnewsurl).

Further, it's bad ux to show an http link to an anchor if it's not an http document.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #14)
> Rather than going into why the procedural null/href return makes no
> difference, I will note that your example proves that the click method is a
> fail - it fails on the img (since there's no href) and on the BOLD name
> (since the element is <strong>, with no href, a child of <a>).  It would
> work if you clicked CD/DVD text..
I'm glad I contributed something ;-)
The funny thing is, when you click on the CD/DVD text once, the image and the bold text become sensitive, too, and they even show the mailbox link.

> Why do you think changing a dom document's link node href's is difficult? I
> can't see that at all - we do a similar and harder thing for image fitting.
> The gecko parser even gives us an iterable called links and all we do is
> test for the link node having a hash property and updating the href with the
> real document url (the msgmailnewsurl).
I'm happy to stand corrected. I've been on this project only for 1.5 years, and I'm doing OK. I don't know many things still.

Comment 16

[alta88]

I'd say much better than just OK..

Comment 17

[alta88]

Attachment: anchor2.patch

I skipped adding a visited anchor in mail to history, although an argument can be made for it, primarily since Back doesn't work in messagepane (though it could) so as not to make it falsely too browserlike.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: Patch with debug.

This works, but are you sure it works as expected? My debug shows:
http://www.nachdenkseiten.de/?p=34756=====
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20message%202?number=7#h01=====
=== return 3

I think/hope the logic in contentAreaClick.js can be further simplified. If you find a nsIMsgMailNewsUrl you can be pretty sure you've inserted it (although we can exclude the case where evil people send messages with those).

IMHO, comparing href with baseURI will always be unequal, so you take the 3rd return and don't return null as the comment suggests.

Am I missing something?

Comment 19

[alta88]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #18)
> Created attachment 8791667 [details] [diff] [review]
> Patch with debug.
> 
> This works, but are you sure it works as expected? My debug shows:
> http://www.nachdenkseiten.de/?p=34756=====
> mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.
> testing/Mail/Local%20Folders/AAA%20Test%20message%202?number=7#h01=====
> === return 3
> 

if it scrolls to the anchor it works as expected.  that is the right form for an anchor in a mailnews url.  what's the specific issue?

> I think/hope the logic in contentAreaClick.js can be further simplified. If
> you find a nsIMsgMailNewsUrl you can be pretty sure you've inserted it
> (although we can exclude the case where evil people send messages with
> those).
> 

right, that's why it early returns (same behavior as current) if it's not http. all that needs to be tested there now is a web page load - use the first test message and double click it to load the web page in message pane to test.

> IMHO, comparing href with baseURI will always be unequal, so you take the
> 3rd return and don't return null as the comment suggests.
> 
> Am I missing something?

not sure why you say that. it's an extra check so if an href happens to have an anchor, which also happens to exist, but the href's url is spoofed to be different - in that case we go external.  i believe the same would happen in a web page (load link rather than scroll to anchor), look here:
https://dxr.mozilla.org/comm-central/rev/8a494adbc5cced90a4edf0c98cffde906bf7f3ae/mozilla/docshell/base/nsDocShell.cpp#10120

Comment 20

[alta88]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #18)
> Created attachment 8791667 [details] [diff] [review]
> Patch with debug.
> 
> This works, but are you sure it works as expected? My debug shows:
> http://www.nachdenkseiten.de/?p=34756=====
> mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.
> testing/Mail/Local%20Folders/AAA%20Test%20message%202?number=7#h01=====
> === return 3
> 

ah, i see what you mean, the test should be for rootNode.URL to early return for mailnews uris...

Comment 21

[alta88]

Attachment: anchor2.patch

update.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8791700 [details] [diff] [review]
anchor2.patch

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

I think the early return for mailnews URLs is appropriate. I'm just not sure why we need the code that follows. I'm new here, please explain.

::: mail/base/content/contentAreaClick.js
@@ +28,5 @@
>        if (target.hasAttribute("href"))
>          href = target.href;
> +
> +      if (!target.hash || !target.rootNode.URL.startsWith("http"))
> +        return href;

I think this needs a comment along the lines of:

If we're clicking on a link in a mail message, the root URL will be a 'MsgMailewsURL' (like mailbox:// or imap://). For those we already replaced anchors in mailWindowOverlay.js, so let's return early here.

That said, why do I need the code that follows? That's about a web page that's showing in a tab, right? I never worked out when TB opens its own tab and when it launches the browser. Can you please give me some instructions on how to test this.

Comment 23

[alta88]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #22)
> Comment on attachment 8791700 [details] [diff] [review]
> anchor2.patch
> 
> Review of attachment 8791700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the early return for mailnews URLs is appropriate. I'm just not sure
> why we need the code that follows. I'm new here, please explain.
> 
> ::: mail/base/content/contentAreaClick.js
> @@ +28,5 @@
> >        if (target.hasAttribute("href"))
> >          href = target.href;
> > +
> > +      if (!target.hash || !target.rootNode.URL.startsWith("http"))
> > +        return href;
> 
> I think this needs a comment along the lines of:
> 
> If we're clicking on a link in a mail message, the root URL will be a
> 'MsgMailewsURL' (like mailbox:// or imap://). For those we already replaced
> anchors in mailWindowOverlay.js, so let's return early here.
> 

sure.

> That said, why do I need the code that follows? That's about a web page
> that's showing in a tab, right? I never worked out when TB opens its own tab
> and when it launches the browser. Can you please give me some instructions
> on how to test this.

if the 1)anchor test or 2)specIgnoringRef test is removed, the code path/behavior is different:
1) if the anchor does not exist, on click nothing happens, as opposed to opening externally.
2) there is a noticeable delay as the link is passed to docShell and it decides what to do, which eventually is to open externally; however, the visited links processing will not run (via openLinkExternally() implemented recently).

this can be seen if you use the Hinweise eml, set Message->When Opening Feed..->Toggle.. and then double clicking the item to load the web page instead of summary.  then, you can use Dom Inspector to change the <a> node's href:
1) the anchor ref to be invalid.
2) the url part (not the ref) to something else.

opening a specific web page in a content tab is difficult in native Tb.  my extension TotalMessage lets you open any embedded link in a content tab and do back/forward.  but the native issue here is loading a web page in messagepane, which can be tested as above.

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #23)
> set Message->When Opening Feed..->Toggle.. 
Sorry, I looked for 15 minutes and couldn't find it
https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1929
not even in a feed folder "news.mozilla.com" I attached myself to for the cache test.
Should be somewhere between "Edit as new" and "Tag" but it's not there.

Comment 25

[alta88]

"news.mozilla.com" is a newsgroup, no message there can be a feed.  if you paste the eml into any local folder, its feed flag should trigger the right menu item to appear.. you may need to create the first line "From -" line. or you can import the .eml which should add that line.

you can also create a Feed account and subscribe to https://www.nachdenkseiten.de/?feed=atom.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8791700 [details] [diff] [review]
anchor2.patch

OK, I finally tested that. You need a *feed* folder, not "any local folder" ;-)

Now, as already agreed, let's have some comments:

       // If we're clicking on a link in a mail message, the root URL
       // will be a 'MsgMailewsURL' (like mailbox:// or imap://).
       // For those we already replaced anchors in mailWindowOverlay.js,
       // so let's return early here.
+      if (!target.hash || !target.rootNode.URL.startsWith("http"))
+        return href;

       // The following code is needed if a (feed) message is opened as
       // web page. I this case there is no preprocessing in
       // mailWindowOverlay.js and we adjust the href here.

That said, if I load my Tony Gould message, I still have to click onto the "CD/DVD" and not the image and not the bold, to make TB scroll to the section. You said that you didn't want to fix this, and I can see that the bug is in the category "Feed Reader", so perhaps Feeds don't do that, or do they? Anyway, the UX is a little confusing, since the user gets displayed a link in the status bar, however, whether the shown link opens internally or externally depends on the internals of the HTML.

Comment 27

[alta88]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #26)
> Comment on attachment 8791700 [details] [diff] [review]
> anchor2.patch
> 
> OK, I finally tested that. You need a *feed* folder, not "any local folder"
> ;-)
> 

no, it should (and does for me) work if that message is copied to Local Folders, Inbox for example..

> Now, as already agreed, let's have some comments:
> 
>        // If we're clicking on a link in a mail message, the root URL
>        // will be a 'MsgMailewsURL' (like mailbox:// or imap://).
>        // For those we already replaced anchors in mailWindowOverlay.js,
>        // so let's return early here.
> +      if (!target.hash || !target.rootNode.URL.startsWith("http"))
> +        return href;
> 
>        // The following code is needed if a (feed) message is opened as
>        // web page. I this case there is no preprocessing in
>        // mailWindowOverlay.js and we adjust the href here.
> 

except that's not what we're doing; we're merely doing some validation tests and returning null to let the click go to docShell.  it's already commented for what it does.

> That said, if I load my Tony Gould message, I still have to click onto the
> "CD/DVD" and not the image and not the bold, to make TB scroll to the
> section. You said that you didn't want to fix this, and I can see that the
> bug is in the category "Feed Reader", so perhaps Feeds don't do that, or do
> they? Anyway, the UX is a little confusing, since the user gets displayed a
> link in the status bar, however, whether the shown link opens internally or
> externally depends on the internals of the HTML.

no, it should (and does for me) work with your message with click on any of image/bold text/regular text.

let's make sure we're testing the latest patch.

Comment 28

[alta88]

Attachment: anchor2.patch

udpate.

Comment 29

[alta88]

(In reply to alta88 from comment #27)
> (In reply to Jorg K (GMT+2, never had any PTO during summer) from comment
> #26)
> > Comment on attachment 8791700 [details] [diff] [review]
> > anchor2.patch
> > 
> > OK, I finally tested that. You need a *feed* folder, not "any local folder"
> > ;-)
> > 
> 
> no, it should (and does for me) work if that message is copied to Local
> Folders, Inbox for example..
> 

as per the feed user guide[1], make sure you select the message first.

[1] https://support.mozilla.org/en-US/kb/how-subscribe-news-feeds-and-blogs

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #27)
> > That said, if I load my Tony Gould message, I still have to click onto the
> > "CD/DVD" and not the image and not the bold, to make TB scroll to the
> > section. You said that you didn't want to fix this, and I can see that the
> > bug is in the category "Feed Reader", so perhaps Feeds don't do that, or do
> > they? Anyway, the UX is a little confusing, since the user gets displayed a
> > link in the status bar, however, whether the shown link opens internally or
> > externally depends on the internals of the HTML.
Maybe we misunderstood each other. I used the "Tony Gould" message, drag it into a feed folder, let it open as web page in a tab and then click on the image/bold/CD+DVD. Only the click on the CD+DVD scrolls, which is the same problem discussed in comment #15.
Of course Tony Gould works fine if displayed as a message with the links replaced.

> no, it should (and does for me) work with your message with click on any of
> image/bold text/regular text.
Not when opened as a web page. Am I missing something?

> let's make sure we're testing the latest patch.
Yes, I was using your latest patch, only with a few debugs. I now refreshed with the latest commented patch supplied here and it still doesn't work.

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #23)
> my extension TotalMessage lets you open any embedded link in a content tab
> and do back/forward.
Just out of interest, where can I find that? You have three add-ons on AMO and this is not one of them.

Comment 32

[alta88]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #30)
> (In reply to alta88 from comment #27)
> > > That said, if I load my Tony Gould message, I still have to click onto the
> > > "CD/DVD" and not the image and not the bold, to make TB scroll to the
> > > section. You said that you didn't want to fix this, and I can see that the
> > > bug is in the category "Feed Reader", so perhaps Feeds don't do that, or do
> > > they? Anyway, the UX is a little confusing, since the user gets displayed a
> > > link in the status bar, however, whether the shown link opens internally or
> > > externally depends on the internals of the HTML.
> Maybe we misunderstood each other. I used the "Tony Gould" message, drag it
> into a feed folder, let it open as web page in a tab and then click on the
> image/bold/CD+DVD. Only the click on the CD+DVD scrolls, which is the same
> problem discussed in comment #15.
> Of course Tony Gould works fine if displayed as a message with the links
> replaced.

Now I see what you're doing, normally an email cannot have its Content-Base loaded, but it can if it's moved to a feed folder, due to keeping pre Tb15 message feedness compatibility and not checking the flag there..

So the solution is to skip all this code for testing links and images, which was designed for mailnews urls, and just let docShell take care of any click for web pages. Jorg, thanks for pushing on it ;)

Comment 33

[alta88]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #31)
> (In reply to alta88 from comment #23)
> > my extension TotalMessage lets you open any embedded link in a content tab
> > and do back/forward.
> Just out of interest, where can I find that? You have three add-ons on AMO
> and this is not one of them.

https://totalmessage.mozdev.org/

Comment 34

[alta88]

Attachment: anchor2.patch

updated.

Comment 35

[alta88]

Attachment: anchor2.patch

Comment 36

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #32)
> Jorg, thanks for pushing on it ;)
My pleasure, thanks for the patience ;-)

(In reply to alta88 from comment #33)
> https://totalmessage.mozdev.org/
Since that wasn't advertised in Google, I installed "WebApp Tabs". This BMO comment is typed in TB. I must say that I don't like it much since it opens everything in TB, but it's lacking some features of a real browser tab, especially "middle mouse click" to open a link in a new tab. I'll try yours later.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8792202 [details] [diff] [review]
anchor2.patch

YES! r=jorgk.

Lucky I know this Tony Gould guy, he helped us achieve a better solution ;-)

Comment 38

[rsx11m]

http://hg.mozilla.org/comm-central/rev/32bf9c43bef8

Comment 39

[alta88]

Unfortunately, allowing all http links results in undesired opening of certain links in a content tab rather than externally, patch upcoming for this regression.

Comment 40

[alta88]

Attachment: anchorFollowup.patch

Comment 41

[alta88]

Attachment: anchorFollowup.patch

Comment 42

[alta88]

Attachment: anchorFollowup.patch

argh, fix empty file.

Comment 43

[Jorg K (CEST = GMT+2)]

Comment on attachment 8792317 [details] [diff] [review]
anchorFollowup.patch

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

I'm going out for a few hours now, back tonight ;-)

::: mail/base/content/contentAreaClick.js
@@ +84,5 @@
> +  while (linkNode && !(linkNode instanceof HTMLAnchorElement))
> +    linkNode = linkNode.parentNode;
> +
> +  // It's an anchor.
> +  if (linkNode && linkNode.href && linkNode.hash)

Unfortunately that still doesn't work.
Load Tony Gould as web page.
I changed the Facebook link at www.henk.com.au/upcoming to
https://www.facebook.com/henk.music.promotion#01
TB still opens it in a tab :-(

Yes, the also bold Youtube link
"Tony Gould on the Art of Creative Music - Trailer" opens externally as it should because it doesn't have a #.

I think you just have to check that the link goes to somewhere on the same page. (No warranty on that.)

Comment 44

[alta88]

Yes, we have to put that check back.  The test for existence of the ref isn't required, as the behavior in a browser is to do nothing, and we should match that rather than opening externally.

Comment 45

[alta88]

Attachment: anchorFollowup.patch

Comment 46

[Jorg K (CEST = GMT+2)]

Comment on attachment 8792322 [details] [diff] [review]
anchorFollowup.patch

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

::: mail/base/content/contentAreaClick.js
@@ +77,5 @@
> + *
> + * @param HTMLElement aTargetNode - the element node.
> + * @return                        - true if anchor or anchor child, else false.
> + */
> +function isAnchorOrAnchorChild(aTargetNode)

I think the function name is not right now. Perhaps something like: "Is anchor on page" or "Is anchor matching URL" or something similar. Also, we could pass in the URL we're looking for, right? target.rootNode.URL.

Comment 47

[alta88]

i really feel it's unimportant and tangential to the point.  and we don't need to pass in any url as it's derived from the node we do pass in. if you require it for r, then best to attach such a stylistic thing written as you want it.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: I think this is clearer ;-)

Comment 49

[Jorg K (CEST = GMT+2)]

Attachment: I think this is clearer ;-) (v2)

(Fixed comment.)

Comment 50

[Jorg K (CEST = GMT+2)]

Comment on attachment 8792330 [details] [diff] [review]
I think this is clearer ;-) (v2)

Damn comment. It should be:

If we've loaded a web page url, and the element's or its ancestor's href
points to an anchor on the page, don't open link externally.

Comment 51

[alta88]

Comment on attachment 8792330 [details] [diff] [review]
I think this is clearer ;-) (v2)


it's odd to have a function name with a very broad meaning, while having it perform a very specific test, and having a very specific comment.  but, as i said, it's not important and functionally is no different, so f+ and r+.  you can do the landing etc. on the updated patch.

Comment 52

[Jorg K (CEST = GMT+2)]

Look, it's 1:30 AM here, so I'll land it in the morning. Give it a few minutes thought and submit a patch with a better name. I'm not sure I like the slightly twisted logic: True means, don't do it. We've been through so many loops here that one doesn't matter to make it perfect ;-)

But you must admit that you're name wasn't right either: isAnchorOrAnchorChild.

No, the anchor is the other guy being pointed to by the node you click on, so the honest name would have been: isLinkPointingToAnchorOnPageOrChildThereof ;-) How about: isLinkPointingToAnchorOnPage?

Comment 53

[Jorg K (CEST = GMT+2)]

Attachment: Final offer ;-)

Comment 54

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/2a22881408f8
(landed with more tweaks to the comments.)

Comment 55

[alta88]

(In reply to Jorg K (GMT+2, can't work due to bug 1304183) from comment #52)
> 
> But you must admit that you're name wasn't right either:
> isAnchorOrAnchorChild.
> 

nope.  (and it's "your").

> No, the anchor is the other guy being pointed to by the node you click on,
> so the honest name would have been:
> isLinkPointingToAnchorOnPageOrChildThereof ;-) How about:
> isLinkPointingToAnchorOnPage?

incorrect, and just for the record: the element with an href containing a hash ref it its url is an instance of HTMLAnchorElement - ie the anchor.  the anchor *target* is the node with the id/name of the anchor element's hash property.

;)

Comment 56

[Jorg K (CEST = GMT+2)]

Well, I got inspired by the commit message of both the patches you submitted:
  Clicking a link to an anchor opens a browser window instead of scrolling.

So, this talks about a link (pointing) to an anchor. It doesn't say "anchor to an anchor target".

I think the nomenclature is very unfortunate here and both are <a> elements and both the #part of the link and the target are commonly referred to as "anchor", just like you did ;-)

Comment 57

[alta88]

the bug title was chosen by the OP, and carried forward into the patch..

Comment 58

[rsx11m]

You guys have interesting problems... the main thing is that it works now and that the name of the function reasonably reflects what that function is supposed to do. :-D

Comment 59

[Stefan Sitter [:ssitter]]

Using Thunderbird 51.0a2 (20160922004009) I get many errors like the following while reading newsgroup messages:

SyntaxError: '#595, [name='595']' is not a valid selector
  OnMsgParsed chrome://messenger/content/mailWindowOverlay.js:3379:10
  messageHeaderSink.onEndMsgDownload chrome://messenger/content/msgHdrViewOverlay.js:731:7

In this example the message contains a link to http://searchfox.org/mozilla-central/source/dom/media/GraphDriver.cpp#595

Is this a known regression?

Comment 60

[alta88]

yes, please open a new bug, cc me, and attach the source or point to a particular newsgroup and/or message.

Comment 61

[Stefan Sitter [:ssitter]]

Created Bug 1304795.