Closed Bug 974857 Opened 9 years ago Closed 9 years ago

Anchor links not working when reading HTML e-mails (involving IMAP)

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35
Tracking Flags:
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
firefox-esr31 33+ fixed
thunderbird33 --- wontfix
thunderbird34 --- fixed
thunderbird35 --- fixed
thunderbird_esr31 --- fixed

People

(Reporter: darren.p.kenny, Assigned: bzbarsky)

References

Details

(Keywords: dogfood, regression, Whiteboard: [regression:TB29])

Attachments

(3 files, 1 obsolete file)

Test HTML Links.eml
2.38 KB, message/rfc822
Details
Screenshot 2: Anchor links broken for TB 31 beta IMAP accounts
194.26 KB, image/png
Details
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches
2.90 KB, patch
smaug
: review+
lmandel
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta-
bkerensa
: approval-mozilla-esr31+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1838.2 Safari/537.36

Steps to reproduce:

This is similar to the bug 226130, but that was closed as already fixed in nightlies over a month ago, so wasn't sure whether to re-open or log a new bug... 

To reproduce, compose a new HTML e-mail to yourself, insert a couple of anchor links, and then insert some links to those anchors, and send.

I'm running the following build:

Name 	Thunderbird
Version 	30.0a1
User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Thunderbird/30.0a1
Application Build ID 	20140215030203




Actual results:

When you receive that e-mail you can see that despite it looking correct, the anchor links open a download window instead of jumping to the correct location in the e-mail body. 



Expected results:

It should have simply scrolled the e-mail to the location of the anchor.
Darren, thanks for taking the time to report.

I am not able to reproduce your bug, wfm on WinXP, TB24 and Trunk TB30.0a1 (2014-02-09).

So we need a testcase from you. Pls save an affected msg as .eml file, remove personal data with text editor, then attach here using "add an attachment" link above. Tia.
^^
Flags: needinfo?(darren.p.kenny)
Also, please see if any of these bugs describe your problem/scenario:

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%3Athun%2Cmailn%20anchor
Hmm, just tried to reproduce it with the attached e-mail - when I open it as a saved file, it works...

It would appear that it is failing only when I'm reading the e-mail after receiving it via IMAP, which is how I read my e-mail day-to-day.

Could it be that the full URL generated when reading via IMAP is broken causing it to be interpreted as an anchor in a different document?
Flags: needinfo?(darren.p.kenny)
Attached file Test HTML Links.eml — 

    
    

    
  
The slightly sanitised (for the mailserver) URL that I am getting is:

imap://darren%2Ep%2Ekenny%40gmail%2Ecom@mailserver:993/fetch%3EUID%3E/INBOX%3E288615#Heading_3

Does that look correct?

    
    

    
  
(In reply to Darren Kenny from comment #6)
> The slightly sanitised (for the mailserver) URL that I am getting is:
> 
> imap://darren%2Ep%2Ekenny%40gmail%2Ecom@mailserver:993/fetch%3EUID%3E/INBOX%3E288615#Heading_3
> 
> Does that look correct?

^^
Don't know. Anyone?
Flags: needinfo?
Summary: anchor links not working when reading HTML e-mails → anchor links not working when reading HTML e-mails (involving IMAP)

    
    

    
  
I still see this kind of thing for any of my IMAP mails with a simply anchor/target reference.

It always triggers a launch of Firefox on my Mac - as if FF could possibly read imap URLs ;)
Flags: needinfo?

    
    

    
  
Currently building a outlook compatible email (DEAR GOD OH WILL YOU PLEASE PLEASE KILL ME!?) with anchor links and I ran in the same issue on Windows:
I'm getting a download popup within Thunderbird 31.0, no fancy scrolling to content.

So overall it's platform unrelated.

The end is near: Outlook is actually capable (though slightly broken) of something, while Thunderbird isn't? I guess we're ALL GONNA DIE!

    
    

    
  
Thanks everyone for commenting, I can now reproduce on IMAP account (works for POP/local) on TB31 beta (20140715165256) on WinXP -> confirming.

Same scenario still works for TB 24 IMAP accounts, so it's a regression.

Breaking links in HTML messages is a major bug imho, regardless of the type of link. Anchor links are actually pretty frequent in HTML formatted business communication, advertising, newsletters etc., and we break them all, making TOCs and other such top-links completely useless. I strongly recommend fixing this for TB31. Per comment 9, we might end up being laughed at if we don't...
Severity: normal → major
Status: UNCONFIRMED → NEW

          tracking-thunderbird31:
          --- → ?
Component: Untriaged → Message Reader UI
Ever confirmed: true
Keywords: regression, 
          
            regressionwindow-wanted
OS: Mac OS X → All
Hardware: x86 → All
Summary: anchor links not working when reading HTML e-mails (involving IMAP) → Anchor links not working when reading HTML e-mails (involving IMAP)

    
    

    
  
Only seen on IMAP accounts (works for POP/local), fails on TB31 beta (20140715165256), tested on WinXP.
Works for IMAP accounts on TB 24 (regression).

STR

1 compose HTML msg having something like this in HTML source:

  <a href="#myanchor">link to myanchor below</a><br>
  <br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br><br>
  <a name="myanchor">myanchor</a><br>

2 Receive and open above msg with TB 31 beta on an IMAP account (for comparison, try POP/local too)
3 Click on "link to myanchor"

Actual Result

-> see attachment 8462709 [details]

- The relative link URL is completed by TB (as seen in status bar), which looks correct and works in TB24:
imap://thomasdxxx@gmail.com@imap.googlemail.com:993/fetch>UID>/INBOX>255#myanchor
- clicking the anchor link opens a dialogue "Opening Inbox_255":
> You have chosen to open Inbox_255
> which is: Firefox HTML document
> from: imap://imap.googlemail.com:993
> What should TB do with this file?
> (o) Open with: Firefox (default)
- Interestingly, the tooltip on "imap://imap.googlemail.com:993" shows a different and clearly mutilated URL:
imap://thomasd%2Ebugzilla%40gmail%2Ecom@imap.googlemail.com:993/fetch%3EUID%3E/
- two observations on the tooltip URL:
a) the inbox referrer is missing: INBOX%3E255#myanchor
b) some parts are escaped, others not, check %40 vs. @

Expected result:

TB should follow the relative anchor link and just scroll the anchor into sight (as we correctly do on TB24 (any account type), and on TB31 pop/local accounts.
Keywords: dogfood

    
    

    
  
In local build:
Last Good: common-central:0456c50549f6, mozilla-central:f356b409d710
First Bad: common-central:0456c50549f6, mozilla-central:4142d03082e1

Triggered by:
4142d03082e1	Boris Zbarsky — Bug 881487. Make anchor scrolls on wyciwyg documents work correctly. r=smaug
Blocks: 881487
Keywords: regressionwindow-wanted

    
    

    
  
So what that bug changed was to call CreateExposableURI.

I guess CreateExposableURI actually does two things.  It will fix up wyciwyg bits correctly.  But it will _also_ nuke the username/password from the URL by default (unless the "browser.fixup.hide_user_pass" preference is set to false).  It does this after cloning the URL, of course.

This url from comment 13:

  imap://thomasdxxx@gmail.com@imap.googlemail.com:993/fetch>UID>/INBOX>255#myanchor

has a userpass, so in theory CreateExposableURL would nuke it.  But the escaping that I see in comment 13 suggests that nsMsgMailNewsUrl::Clone is just broken...

In any case, this highlights an issue with the patch in bug 881487: If the URI of the page includes a userpass, then anchor scrolls will not work.  We should really fix that.  Fundamentally, what we want here (and in the location set code), I think, is a variant of createExposableURI that only fixes up the wyciwyg bits but does not touch the userpass.
Flags: needinfo?(bzbarsky)

    
    

    
  
Wow, Alice & Boris, thanks that's great teamwork... If Boris succeeds to fix this at the same speed, you'll really make my day :)

OT:

(In reply to David H. from comment #9)
> Currently building a outlook compatible email (DEAR GOD OH WILL YOU PLEASE
> PLEASE KILL ME!?)

lol... As a theologian, I can confirm that deliberately seeking to build outlook compatible email is a cardinal sin (involving signs of mental confusion, too), you should repent and consult your psychologist asap... ;)

> The end is near: Outlook is actually capable (though slightly broken) of
> something, while Thunderbird isn't? I guess we're ALL GONNA DIE!

OMG, indeed, you're so right, that's more than SCARY, like the world turning upside down...
lol... ;)

    
  
Whiteboard: [regression:TB29]

    
    

    
  


  
This should fix it.  I tried creating test Thunderbird builds with this patch, but for some reason the Windows and Mac ones don't work (because of calendar stuff, not this patch).  So if someone is using Thunderbird on Linux and seeing this bug and is willing to test whether the problem is fixed, that would be much appreciated.

Olli, I tried writing a testcase for this, but getting Firefox to load a URI with a userpass is rocket science involving window-modal dialogs.  :(

        Attachment #8484732 -
        Flags: review?(bugs)

    
  
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

So this works as long as browser.fixup.hide_user_pass is true, right?
Otherwise wyciwyg might contain userpass, I think

        Attachment #8484732 -
        Flags: review?(bugs) → review+

    
    

    
  
On Ubuntu 14.04.1 x86_64 the new build works correctly.

    
    

    
  
> So this works as long as browser.fixup.hide_user_pass is true, right?

I think it works either way.

> Otherwise wyciwyg might contain userpass, I think

Sure, but the point is that so would our fixed-up URI.  All I'm doing is trying to compare our incoming URI to both the fixed-up and non-fixed-up versions of mCurrentURI, because maybe our caller did fixup and maybe not.

James, thanks for checking that!

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/bda5f3a0f8a2
Target Milestone: --- → Thunderbird 35.0

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

I don't think it makes sense to backport this to Firefox Aurora/Beta, but we should probably make sure we land it in Thunderbird 31.x

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
   Breaks Thunderbird functionality.  Or should I be requesting
   approval‑comm‑esr31?
User impact if declined: Clicking links in emails in Thunderbird is all borked
Fix Landed on Version: 35
Risk to taking this patch (and alternatives if risky): I think this should be
   fairly low risk.  The alternative is to do nothing and let this ride the trains.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

        Attachment #8484732 -
        Flags: approval-mozilla-esr31?

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/bda5f3a0f8a2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

[Approval Request Comment]
Regression caused by (bug #): Bug 881487
User impact if declined: Breaks all anchor links in html-formatted emails (esp. business mail with TOCs, Top-Links etc.), which is a frequent, annoying, and ridiculous failure scenario for an HTML mail reader
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): "fairly low" according to patch author in comment 26

        Attachment #8484732 -
        Flags: approval-comm-esr31?

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

This is a mozilla-central change, and as such is approved by the Firefox release drivers. Bz's already put in a fair approval request, I don't have anything to add to it.

        Attachment #8484732 -
        Flags: approval-comm-esr31?

    
    

    
  
Pleeeez fix this bug, thank you! (Plea from a common (but assiduous) Thunderbird out there...

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #26)
> Comment on attachment 8484732 [details] [diff] [review]
> Compare attempted anchor traversals to both the actual page URI and the
> exposable URI, and do an anchor scroll if either one matches
> 
> I don't think it makes sense to backport this to Firefox Aurora/Beta, but we
> should probably make sure we land it in Thunderbird 31.x
> 
> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR
> consideration:
>    Breaks Thunderbird functionality.  

Common example is mail digests and annoucement type messages.

    
    

    
  
(In reply to Wayne Mery (:wsmwk) from comment #31)

> Common example is mail digests and annoucement type messages.

This is precisely what brought me here. I receive digest emails from Yahoo Groups and am suffering this bug since updating to 31. Which basically makes 31 no use to me...

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

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

This request does not meet landing criteria 
https://wiki.mozilla.org/Release_Management/ESR_Landing_Process#What_should_land_on_mozilla-esr24.2Fmozilla-esr31

        Attachment #8484732 -
        Flags: approval-mozilla-esr31? → approval-mozilla-esr31-

    
    

    
  
OK, so what's needed to get this landed in a Gecko that would enable Thunderbird to have this bug fixed?  Which particular ESR hoop did I fail to jump through?  Getting this landed on Firefox Aurora and Beta first?
Flags: needinfo?(bkerensa)

    
    

    
  
bz - I don't want you to spend any more time on this. I'm setting ni on myself to follow up and figure out exactly how and where this needs to land next week.
Flags: needinfo?(lmandel)

    
    

    
  
Just clarified with Lukas that ideally the Thunderbird Drivers would speak up on this but if they don't then we can take it ESR31 for them. Going to NI Mark Banner since he is the Release Driver for Thunderbird.
Flags: needinfo?(bkerensa) → needinfo?(standard8)

    
    

    
  
Yes, Thunderbird drivers would very much like this fixed, as it is a regression in the 31.x series compared to the 24.x series.
Flags: needinfo?(standard8)

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

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

Based on feedback from Thunderbird Drivers going to take this

        Attachment #8484732 -
        Flags: approval-mozilla-esr31- → approval-mozilla-esr31+
Component: Message Reader UI → Document Navigation
Product: Thunderbird → Core
Target Milestone: Thunderbird 35.0 → mozilla35
Version: 29 → Trunk

    
    

    
  
So a fixed version of Thunderbird will be released in January?
That's a bit of a joke.

    
    

    
  
If this went into ESR31 then a new TB version of 31.2 with the fix should be out in some weeks.

    
    

    
  
How come it appeared in 32.0.2 Release Notes as fixed?

"Fixed an issue where anchor links would not work in HTML emails (Bug 974857)"

    
    

    
  
Which release are you talking about? There is no TB 32.0.2, and I see it mentioned neither in TB 31.1.[01] nor in FF 32.0.[23].

    
    

    
  
Because the fix was included in tb31.1.2.

    
    

    
  
Correct, here it is: http://hg.mozilla.org/releases/mozilla-esr31/graph/200330?revcount=12 (last one)

This didn't land for SM 2.29.1, though, which is built from mozilla-release. Thus, SeaMonkey users will have to wait until 2.32 (rv: 35.0) to see the fix, unless this lands on the other channels as well.

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

Per discussion in today's SeaMonkey status meeting, requesting approval for m-a and m-b (as possible) so that this fix will be included in the upcoming SM 2.30 beta(s) and release, or at least for 2.31 already.

http://logs.glob.uno/?c=mozilla%23seamonkey&s=30+Sep+2014&e=30+Sep+2014#c626671

Approval Request Comment
[Feature/regressing bug #]: bug 881487
[User impact if declined]: SeaMonkey users will continue to experience this issue until their 2.32 release while it seems to be working fine on TB 31.x
[Describe test coverage new/current, TBPL]: Seems to work fine for c-c and esr31
[Risks and why]: was acceptable for ESR 31, thus assuming it's low enough
[String/UUID change made/needed]: none afaict

For the case that confirmation from a SM official is needed, ni?-ing IanN and Callek.

        Attachment #8484732 -
        Flags: approval-mozilla-beta?

        Attachment #8484732 -
        Flags: approval-mozilla-aurora?
Flags: needinfo?(iann_bugzilla)
Flags: needinfo?(bugspam.Callek)

    
    

    
  
I'd *love* this in aurora/beta, but I certainly can't make the a+ call.
Flags: needinfo?(iann_bugzilla)
Flags: needinfo?(bugspam.Callek)

    
    

    
  
Thanks, I'll take this as an SM-driver confirmation in the intend of comment #37. ;-)

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

I'm happy to take this on Aurora. Given that we're at the end of the Beta cycle, I'm going to ask Sylvestre to make the call for Beta.

        Attachment #8484732 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Justin, I guess this code is used by Firefox itself, right?
We are late in the beta cycle, if I can avoid it, I would like to avoid taking it.
What would be the impact on SM if we don't take it? (I don't know much about SM release schedule)
Flags: needinfo?(bugspam.Callek)

    
    

    
  
(In reply to Sylvestre Ledru [:sylvestre] from comment #53)
> Justin, I guess this code is used by Firefox itself, right?
> We are late in the beta cycle, if I can avoid it, I would like to avoid
> taking it.
> What would be the impact on SM if we don't take it? (I don't know much about
> SM release schedule)

I can neither confirm nor deny its a codepath/file used by Firefox. However I can confirm this affects our users of e-mail (would also affect Thunderbird if they did another release on what is currently beta this cycle, but thats not planned at present).

Basically when there is an "HTML e-mail" (as opposed to plaintext e-mail) in a users inbox, with a link, the link will not work to open a page, without this patch.
Flags: needinfo?(bugspam.Callek)

    
    

    
  
Comment on attachment 8484732 [details] [diff] [review]
Compare attempted anchor traversals to both the actual page URI and the exposable URI, and do an anchor scroll if either one matches

Justin, I am a bit concerned to take it for Firefox that late. Can't you cherry-pick this patch for SM?

        Attachment #8484732 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta-

    
    

    
  
> I can neither confirm nor deny its a codepath/file used by Firefox.

This code is totally used by Firefox all the time.

    
    

    
  
(In reply to Sylvestre Ledru [:sylvestre] from comment #55)
> Comment on attachment 8484732 [details] [diff] [review]
> Compare attempted anchor traversals to both the actual page URI and the
> exposable URI, and do an anchor scroll if either one matches
> 
> Justin, I am a bit concerned to take it for Firefox that late. Can't you
> cherry-pick this patch for SM?

Understood, and thanks. We'll just relnote it for now, imo.

          You need to log in
          before you can comment on or make changes to this bug.