Last Comment Bug 532567 - Update details open in a message window as editable HTML
: Update details open in a message window as editable HTML
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: 3.0
: All All
: -- normal (vote)
: Thunderbird 20.0
Assigned To: Mark Banner (:standard8)
:
Mentors:
: 606847 777232 (view as bug list)
Depends on: 263433 532675
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-02 17:31 PST by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2015-09-17 04:46 PDT (History)
6 users (show)
standard8: wanted‑thunderbird+
standard8: blocking‑thunderbird3-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
-


Attachments
The fix (3.03 KB, patch)
2013-01-01 11:45 PST, Mark Banner (:standard8)
mconley: review+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2009-12-02 17:31:13 PST
STR:

1. Go to Preferences.
2. Go to Advanced > Update.
3. Click Show Update History.
4. Click on a Details link.

What happens:
The update details page opens up in a message window as editable HTML, which is definitely what the user didn't originally want.

What should happen:
The update details page should open in a new content tab.
Comment 1 Mark Banner (:standard8) 2009-12-03 08:34:50 PST
Part of the issue for us, is that toolkit is being inconsistent with this text link and isn't ending up using its openURL function in contentAreaUtils.js.

I've got a patch I'm working on for the core issue (in bug 532675). I'm not sure if we'll get it into 1.9.1 or not, if we don't then I have to figure out a way of fixing the binding from within TB.
Comment 2 Phil Ringnalda (:philor) 2009-12-03 13:05:59 PST
I'm not terribly confident about a fix in mail/ - on non-Mac we could probably set browser.chromeURL to something that knows how to deal, since there we've got nothing to lose (the details link, and anything that follows the same path, hits a fallback for not having a browser.chromeURL pref and tries to load chrome://navigator/content/navigator.xul which we don't have), but on OS X we have it set to the compose window so that clicking a mailto link when we're running with no open windows doesn't do something ugly like open a 3pane and then open a compose window from that. I've thought about solving the whole "foo opens a compose window" (where foo certainly includes feed subscription from a browser, and probably more) by switching to having the hidden window know how to deal and using it as a browser.chromeURL dispatcher, but that very much doesn't feel like a stable-branch fix.
Comment 3 Mark Banner (:standard8) 2009-12-03 14:49:52 PST
From the chat amongst drivers we're not going to block on the original issue of opening the log incorrectly as it is unlikely to be frequently especially as most people are unlikely to find this bit of UI.

Finding some sort of generic solution would be nice, but we may just have to go with the specific solution that I'll propose in bug 532675.
Comment 4 Mark Banner (:standard8) 2009-12-21 04:39:19 PST
This is broken UI, so I think it is needed, but due to its hidden state, not critical for .1.
Comment 5 Mark Banner (:standard8) 2010-01-07 04:23:05 PST
Clearing wanted as this is needed.
Comment 6 Phil Ringnalda (:philor) 2010-10-24 13:34:53 PDT
*** Bug 606847 has been marked as a duplicate of this bug. ***
Comment 7 Ray Z. 2010-10-24 13:41:17 PDT
this bug has been found almost 1 year now, why it's still there? Maybe just to change the open "details" link in the browser (system default), simple and easy, isn't it?

Thanks!
Comment 8 Mark Banner (:standard8) 2010-10-25 03:44:25 PDT
(In reply to comment #7)
> this bug has been found almost 1 year now, why it's still there? Maybe just to
> change the open "details" link in the browser (system default), simple and
> easy, isn't it?

No, it isn't that simple. There's a set of interactions going on here that are difficult to work around.
Comment 9 [:Aureliano Buendía] 2012-08-17 03:39:18 PDT
*** Bug 777232 has been marked as a duplicate of this bug. ***
Comment 10 Mark Banner (:standard8) 2013-01-01 11:45:58 PST
Created attachment 696890 [details] [diff] [review]
The fix

With the recent changes in bug 263433, this is actually quite easy to do and provides a really easy way for us to do the right thing.

Once we land this, we can also re-examine other places where we use text-link and have an onclick handler that calls openURL, but I can file a follow-up for those.
Comment 11 Mike Conley (:mconley) - (Away until June 29th) 2013-01-02 11:13:40 PST
Comment on attachment 696890 [details] [diff] [review]
The fix

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

r=me with the following changes. Thanks Mark!

::: mail/components/mailGlue.js
@@ +113,5 @@
> +  _handleLink: function MailGlue__handleLink(aSubject, aData) {
> +    let linkHandled = aSubject.QueryInterface(Ci.nsISupportsPRBool);
> +    if (!linkHandled.data) {
> +      let win = Services.wm.getMostRecentWindow("mail:3pane");
> +      let tabParams = { contentPage: aData, clickHandler: null};

Nit - space before }, for consistency.

@@ +124,5 @@
> +        }
> +      }
> +
> +      // If we didn't have an open 3 pane window, try and open one.
> +      if (!linkHandled.data) {

Instead of checking this again, I think I'd prefer if, after we set linkHandled.data = true on line 123, we return early. Then, after 125, we can assume that both linkHandled.data is false, and that we weren't able to open a tab in a pre-existing 3pane window.
Comment 12 Mark Banner (:standard8) 2013-01-02 12:50:52 PST
Checked in with comments addressed:

https://hg.mozilla.org/comm-central/rev/bc6d82e4d41f

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