532567 - Update details open in a message window as editable HTML

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[(no longer active)]

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)]

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)]

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)]

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)]

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)]

Clearing wanted as this is needed.

Comment 7

[Ray Z.]

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)]

(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 10

[Mark Banner (:standard8)]

Attachment: 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) (:⚙️)]

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)]

Checked in with comments addressed:

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