Last Comment Bug 516776 - Make it possible for browser elements to navigate through links/pages
: Make it possible for browser elements to navigate through links/pages
Status: RESOLVED FIXED
[no l10n impact]
: dev-doc-complete
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 3.0rc1
Assigned To: Mark Banner (:standard8, limited time in Dec)
:
:
Mentors:
: 523195 (view as bug list)
Depends on: 523447 527664 529235
Blocks: 346220 487072 490817 526733
  Show dependency treegraph
 
Reported: 2009-09-15 11:39 PDT by Mark Banner (:standard8, limited time in Dec)
Modified: 2010-11-17 07:52 PST (History)
14 users (show)
standard8: blocking‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP (2.77 KB, patch)
2009-10-28 15:37 PDT, Mark Banner (:standard8, limited time in Dec)
no flags Details | Diff | Splinter Review
The fix (20.09 KB, patch)
2009-11-04 07:07 PST, Mark Banner (:standard8, limited time in Dec)
clarkbw: ui‑review+
Details | Diff | Splinter Review
Personas test extension (285.31 KB, application/octet-stream)
2009-11-04 07:27 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details
Google Calendar Tab extension (7.60 KB, application/octet-stream)
2009-11-04 07:38 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details
The fix v2 (21.00 KB, patch)
2009-11-05 03:55 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details | Diff | Splinter Review
The fix v3 (21.25 KB, patch)
2009-11-06 04:55 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details | Diff | Splinter Review
The fix v4 (21.25 KB, patch)
2009-11-06 05:56 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details | Diff | Splinter Review
The fix v4 (the right version) (21.40 KB, patch)
2009-11-06 06:16 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details | Diff | Splinter Review
The fix v5 (21.41 KB, patch)
2009-11-09 05:45 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details | Diff | Splinter Review
Handle window.open() v1 (6.67 KB, patch)
2009-11-09 05:49 PST, Mark Banner (:standard8, limited time in Dec)
philringnalda: review+
Details | Diff | Splinter Review
Main fix v6 (21.58 KB, patch)
2009-11-09 07:18 PST, Mark Banner (:standard8, limited time in Dec)
philringnalda: review+
Details | Diff | Splinter Review
Extension helper function (1.77 KB, patch)
2009-11-09 07:21 PST, Mark Banner (:standard8, limited time in Dec)
philringnalda: review+
Details | Diff | Splinter Review
Google Calendar Tab extension v2 (7.53 KB, application/octet-stream)
2009-11-09 07:37 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details
Personas test extension v2 (285.35 KB, application/octet-stream)
2009-11-09 07:52 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details

Description Mark Banner (:standard8, limited time in Dec) 2009-09-15 11:39:12 PDT
Without special (and complicated) work it is really hard for extensions to make navigation between pages work in browser elements. We need to simplify this to an extend that:

1) Clicking on links, buttons etc will do the expected thing to open.
2) The tab opener can choose what to allow/deny e.g.
2a) Restrict to one page only, redirect everything to external default browser
2b) Restrict to one or more sites, redirecting everything else to external default browser.
Comment 1 Phil Ringnalda (:philor) 2009-10-19 13:16:48 PDT
*** Bug 523195 has been marked as a duplicate of this bug. ***
Comment 2 Dan Mosedale (:dmose) 2009-10-20 12:21:28 PDT
cbeard points out that another case worth taking into consideration is the links inside the add-on manager.  They currently kick the user out to the browser, which is a fairly ugly user-experience.
Comment 3 David Ascher (:davida) 2009-10-20 12:31:49 PDT
Yes -- the personas gallery is another example of interestingness.
Comment 4 Bryan Clark (DevTools PM) [:clarkbw] 2009-10-20 17:22:24 PDT
also browsing the AMO site (in a TBtab) with links opening up the add-ons manager for install as the experience of browsing AMO in a web browser and installing extensions into Thunderbird is very broken.
Comment 5 Mark Banner (:standard8, limited time in Dec) 2009-10-28 15:37:44 PDT
Created attachment 408957 [details] [diff] [review]
WIP

Work in progress patch. This currently allows the tab-opener to register an onclick handler which will they can then use appropriately.

I've currently got an example hooked up to the about: pages, and about:license is the best place to see this working (click on the in-document links without & with patch).

I suspect form handling may be broken hence google login may not work correctly - but I'm investigating that under bug 523447 and will try this out with the google extension tomorrow.
Comment 6 Mark Banner (:standard8, limited time in Dec) 2009-11-04 07:07:07 PST
Created attachment 410238 [details] [diff] [review]
The fix

Ok, totally revised patch which I think covers us for all the cases. Highlights:

* The prefs network.protocol-handler.expose.{about,http,https} are now set to true. This will cause any browser element to load any of those types of link within itself.

* For the message pane, we update contentAreaClick so that if people click on a http(s) link then it will redirect that link to an external browser (after the phishing check which is how we did it before).

* Content Tabs now have to specify which links they don't want to load. We provide two click handlers - default and about. Default will redirect about: and http(s): to external browser. About will just redirect the http(s): ones and let about: load internally. Extensions can provide different handlers when they open the tabs, the handler details also get persisted if the tabs are persisted.

* Chrome Tabs have the same default as content tabs (I caught the migration assistant trying to load an http link internally).

* Due to the change of prefs, this would mean that clicking on the "Browse All add-ons" link in the add-on manager wouldn't work. However I've added an nsIContentHandler implementation that will catch those clicks and redirect to the browser.

The downside here (and hence the UI-review) is that we will now only redirect them to the browser if we're in online mode. Clicking on them in offline mode would still do nothing - which as it turns out is exactly what happens in Firefox, hence I think we can possibly get away with it for now at least (the only other option would be to provide some nasty overlays to overlay the toolkit add-on manager).


I'm still testing this, but I think the majority of it is all in place, and I'll produce some try builds and post a couple of extensions in a while.
Comment 7 Mark Banner (:standard8, limited time in Dec) 2009-11-04 07:08:05 PST
Oh and I should have said, this needs the patch from bug 523447 applied so that forms work in content tabs.
Comment 8 Mark Banner (:standard8, limited time in Dec) 2009-11-04 07:27:22 PST
Created attachment 410241 [details]
Personas test extension

This will work with builds with both the patches applied. It is the personas extension with one minor patch that I'll find a bug to post it on later (and set the bug dependent on this one).

If you go into one of the galleries (go into a category selection on the menu and select the bottom menu option) you will get a tab and can browse around the site. As soon as you click on an external link (e.g. to the mozilla labs site) that will open in your external browser.

You can also use the search form in the normal manner.
Comment 9 Mark Banner (:standard8, limited time in Dec) 2009-11-04 07:38:27 PST
Created attachment 410244 [details]
Google Calendar Tab extension

This is Bryan's google calendar tab extension (from bug 346220 comment 6), you get an additional icon on the tab bar, then you can open the google calendar.

This extension lets you log in, navigate around and interact with google's online calendar. Once you try to go to an external page (e.g. documents, reader etc) then it'll pass you out to the external browser.
Comment 11 Bryan Clark (DevTools PM) [:clarkbw] 2009-11-04 18:11:03 PST
Comment on attachment 410238 [details] [diff] [review]
The fix

This works really well for me so far and I ran with it the whole day.  I noticed one issue where certain links seemed to open a new compose window while they also opened the web page in the browser.  Couldn't dig further into what exactly made some links do this and most links not.
Comment 12 Phil Ringnalda (:philor) 2009-11-05 00:50:12 PST
In the upper right of the google calendar page, the Help, Sync, and New: Whatever We're Advertising To You links all have target="_blank" and all open both a compose window (thanks, browser.chromeURL!) and externally. Dunno what we do about that, or if we can do anything. Even if we have any control over it, I'm not sure what would provide the least surprise. I also really wonder what happens with a frameset.

If you open about:credits via Thunderbird - About - About v - Contributors, and scroll to the bottom, there's a mailto:. Since it's not an about: URI, aboutClickHandler kicks it out to an external load, which is a bit surprising from a mail client. I don't think content is allowed to link to any of our other non-http exposed protocols, so we might want to just expand to allowing about|mailto for internal loads in about:, rather than switching to only explicitly opening our "exposed, but only when we want them to be" schemes externally.

Groveling around in URL-like strings with or without a regex as a URI parser always makes me nervous, even without it being confusing like |href.lastIndexOf("about:", 0) == -1|, which took me quite a bit of staring at to not get it backward. Can we afford to use newURI(...).schemeIs() instead? The only example I have where the various and sundry regexes would be fooled is https:feed://, which isn't much of an example since nothing will give a useful answer for that, but that doesn't mean there aren't other things we would be misinterpreting, but schemeIs would not. I don't really understand the risks, so I just channel bz saying "oh, don't do that!"
Comment 13 Mark Banner (:standard8, limited time in Dec) 2009-11-05 03:55:28 PST
Created attachment 410473 [details] [diff] [review]
The fix v2

Updated patch. I've taken care of the mailto: issue by seeing if we're exposing the protocol and then letting it load (assuming the protocol isn't http(s)). I think this is reasonable - some pages may have news links (for example) and we'll want those to work as well, though I've a feeling they are broken as well - as this will be an example for extensions, I'd like to get something reasonably good in place.

I'm still thinking about the target="_blank" issue - I think we basically need to detect it is set and redirect those to external browser (i.e. new window) regardless. I'll give it a try later, but need to do some reviews first.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2009-11-05 07:47:25 PST
> I think we basically need to detect it is set

So the basic issue with _blank is that you're stopping the load in an nsIContentHandler, by which point the new window has already been opened, right?  Under what conditions is the nsIContentHandler even used here?  This bug sounds like you generally do want to allow navigation inside the loaded http page...

What happens if page script just calls window.open()?
Comment 15 Mark Banner (:standard8, limited time in Dec) 2009-11-05 08:16:52 PST
(In reply to comment #14)
> What happens if page script just calls window.open()?

Once I tweaked the patch to allow the javascript: protocol, the test page I used (http://www.javascript-coder.com/window-popup/javascript-window-open-example1.html) just did nothing when clicking on the link to call window.open().

So I guess there's something else we are missing.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2009-11-05 08:24:40 PST
I was thinking more like:

<a href="#" onclick="window.open('http://www.google.com'); return false;">Click me</a>
Comment 17 Mark Banner (:standard8, limited time in Dec) 2009-11-05 14:32:26 PST
(In reply to comment #16)
> I was thinking more like:
> 
> <a href="#" onclick="window.open('http://www.google.com'); return false;">Click
> me</a>

That opens a blank compose window and opens "<page url>#" in the external browser.


(I should note that we're primarily using the content tabs for experimenting with extensions in TB 3. The sites that we have using content tabs (within core thunderbird) are under our control hence we can live with this for experimentation purposes I believe, though knowing how to fix it would be nice).
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2009-11-05 14:45:31 PST
> That opens a blank compose window and opens "<page url>#" in the external
> browser.

OK.  And what would you like it to do?

(As an aside... why is it opening "<page url>#" anywhere?  The anchor's onclick handler cancels the event....  Is someone not paying attention to that?

> The sites that we have using content tabs (within core thunderbird) are under
> our control

Last I checked, AMO linked to addon homepages, which are not under your control.
Comment 19 Mark Banner (:standard8, limited time in Dec) 2009-11-06 02:18:36 PST
(In reply to comment #18)
> > That opens a blank compose window and opens "<page url>#" in the external
> > browser.
> 
> OK.  And what would you like it to do?

I think the best thing would be to open the javascript requested page in the external browser.

> (As an aside... why is it opening "<page url>#" anywhere?  The anchor's onclick
> handler cancels the event....  Is someone not paying attention to that?

Possibly, the only thing I can think is that nsIContentHandler is doing the final catch of it and pushing it to the external browser.

> > The sites that we have using content tabs (within core thunderbird) are under
> > our control
> 
> Last I checked, AMO linked to addon homepages, which are not under your
> control.

Right, but my patch allows limiting to specific sites and we're not doing amo by default (those links get pushed to the external browser).
Comment 20 Mark Banner (:standard8, limited time in Dec) 2009-11-06 04:55:15 PST
Created attachment 410765 [details] [diff] [review]
The fix v3

Minor update that fixes the switch from pre to post patch with content tabs open - if there's a tab opened by session restore, we'll now install the default handler if it didn't have one already.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2009-11-06 05:11:52 PST
> Possibly, the only thing I can think is that nsIContentHandler

The code that would trigger the nsIContentHandler shouldn't be happening in this case.  Do you have a click listener installed, though, one that's not checking whether the event's default action has been prevented?

> Right, but my patch allows limiting to specific sites

It does?  Where?  Does it only allow https loads?  Because for http you can't guarantee anything at all here....
Comment 22 Mark Banner (:standard8, limited time in Dec) 2009-11-06 05:56:05 PST
Created attachment 410773 [details] [diff] [review]
The fix v4

Adds checking for trusted events and events which are set to prevent default.
Comment 23 Mark Banner (:standard8, limited time in Dec) 2009-11-06 06:03:58 PST
(In reply to comment #21)
> > Possibly, the only thing I can think is that nsIContentHandler
> 
> The code that would trigger the nsIContentHandler shouldn't be happening in
> this case.  Do you have a click listener installed, though, one that's not
> checking whether the event's default action has been prevented?

Ok, I wasn't checking that - the latest version of the patch does.

> > Right, but my patch allows limiting to specific sites
> 
> It does?  Where?  Does it only allow https loads?  Because for http you can't
> guarantee anything at all here....

By adding their own onclick handlers, extensions can limit which links are loaded in the same content tab versus being opened in an external browser (kinda like the defaultClickHandler in the patch but using a regexp on the link being clicked).

Whilst I realise websites etc can change, or links be hijacked, the changes will generally allow an extensions to limit browsing within Thunderbird to areas of sites or specific sites based on where the links are targeted. Or have I misunderstood what you are saying?
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2009-11-06 06:09:52 PST
> Ok, I wasn't checking that - the latest version of the patch does.

It is?  Where?

> Whilst I realise websites etc can change, or links be hijacked

Right.  That's the key.  If you happen to be on a wireless network, you lose.  If you happen to be on a wired hotel network, you lose.  Seems to me, all you get is a false sense of security.
Comment 25 Mark Banner (:standard8, limited time in Dec) 2009-11-06 06:16:25 PST
Created attachment 410775 [details] [diff] [review]
The fix v4 (the right version)
Comment 26 Mark Banner (:standard8, limited time in Dec) 2009-11-06 06:21:39 PST
(In reply to comment #24)
> > Ok, I wasn't checking that - the latest version of the patch does.
> 
> It is?  Where?

Hopefully there in defaultClickHandler now I've uploaded the right version.

> > Whilst I realise websites etc can change, or links be hijacked
> 
> Right.  That's the key.  If you happen to be on a wireless network, you lose. 
> If you happen to be on a wired hotel network, you lose.  Seems to me, all you
> get is a false sense of security.

So we're not doing web-site limiting on a security basis. We're doing this because we want to allow extensions to experiment with providing web access to things like calendars, add-ons etc from within Thunderbird.

Security is something those extensions should consider, but that's not why we're trying to limit to specific sites. We're implementing the capabilities for limitations so that users can look at specific areas of the web from within Thunderbird, and then "naturally"(?) go out to the browser when they go outside those specific areas.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2009-11-06 06:40:20 PST
> So we're not doing web-site limiting on a security basis.

I see.  The problem here is that there are all sorts of possible weird interactions here that might in fact have security issues.  For example, in the window.open() case... what object does the web page get back?  What can it do with it?
Comment 28 Mark Banner (:standard8, limited time in Dec) 2009-11-06 07:22:52 PST
(In reply to comment #27)
> > So we're not doing web-site limiting on a security basis.
> 
> I see.  The problem here is that there are all sorts of possible weird
> interactions here that might in fact have security issues.  For example, in the
> window.open() case... what object does the web page get back?  What can it do
> with it?

I'll try and have a look at that later, but I'm guessing is something relating to the compose window. If we actually wanted to change that would gecko allow us to or is it too limiting?
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2009-11-06 07:30:39 PST
> If we actually wanted to change that would gecko allow us

Sure.  You have full control over what happens in window opening scenarios.  See nsIWindowProvider.  The one caveat is that you do have to produce an nsIDOMWindow to return.
Comment 30 Mark Banner (:standard8, limited time in Dec) 2009-11-09 05:45:42 PST
Created attachment 411177 [details] [diff] [review]
The fix v5

Minor typo fix for s/event/aEvent/
Comment 31 Mark Banner (:standard8, limited time in Dec) 2009-11-09 05:49:10 PST
Created attachment 411178 [details] [diff] [review]
Handle window.open() v1

Supplemental patch to handle window.open. Here I've basically said we only support window.open opening into a new tab and not a new window - as to fix this any other way I think would require changing browser.chromeURL and I'm concerned that will affect things further than we want, especially for 3.0.
Comment 32 Mark Banner (:standard8, limited time in Dec) 2009-11-09 07:18:40 PST
Created attachment 411185 [details] [diff] [review]
Main fix v6

A couple more improvements to this patch - the browser argument to the click handler functions is now redundant, so I removed it. Also added the true argument to hRefForClickEvent so we don't do weird things and try and handle clicks on form elements when in content tabs.
Comment 33 Mark Banner (:standard8, limited time in Dec) 2009-11-09 07:21:16 PST
Created attachment 411186 [details] [diff] [review]
Extension helper function

Helper function for extensions - allow extensions to provide a regexp (to provide approximate limitation to specific sites) and limit the clicks based on the regexp. This also means we can encourage the re-use of code, which will help if we find we need some extra tweaks.
Comment 34 Mark Banner (:standard8, limited time in Dec) 2009-11-09 07:37:12 PST
Created attachment 411189 [details]
Google Calendar Tab extension v2

New version of the google calendar tab extension updated to use the new extension helper function.

I'll have try server builds going soon for extra help with testing.
Comment 35 Mark Banner (:standard8, limited time in Dec) 2009-11-09 07:52:54 PST
Created attachment 411193 [details]
Personas test extension v2

Personas test extension
Comment 37 Phil Ringnalda (:philor) 2009-11-09 12:04:36 PST
Comment on attachment 411178 [details] [diff] [review]
Handle window.open() v1

>+    let newTab = win.document.getElementById("tabmail")
>+      .openTab("contentTab", {contentPage: "about:blank",
>+                              background: loadInBackground});
>+
>+    newWindow = newTab.browser.docShell
>+      .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+      .getInterface(Components.interfaces.nsIDOMWindow);
>+    try {
>+      if (aURI) {
>+        if (aOpener) {
>+          let location = aOpener.location;
>+          let referrer =
>+            Components.classes["@mozilla.org/network/io-service;1"]
>+            .getService(Components.interfaces.nsIIOService)
>+            .newURI(location, null, null);
>+        }
>+        newWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                 .getInterface(Components.interfaces.nsIWebNavigation)
>+                 .loadURI(aURI.spec, loadflags, referrer, null, null);

Maybe not a record, but that's one densely-packed variety of ways to wrap things with dots.
Comment 38 Phil Ringnalda (:philor) 2009-11-09 12:05:00 PST
Comment on attachment 411185 [details] [diff] [review]
Main fix v6

What, me worry?
Comment 39 Mark Banner (:standard8, limited time in Dec) 2009-11-10 01:49:37 PST
Checked in:
http://hg.mozilla.org/comm-central/rev/2d246c8d3fa4
http://hg.mozilla.org/releases/comm-1.9.1/rev/60c4c14fce24
Comment 40 Mark Banner (:standard8, limited time in Dec) 2009-11-10 04:40:07 PST
Initial documentation that I think is good enough for now and we can expand more later is now available at:

https://developer.mozilla.org/En/Thunderbird_3_for_developers#Content_Browsing
https://developer.mozilla.org/en/Thunderbird/Content_Tabs
Comment 41 Onno Ekker [:nONoNonO UTC+1] 2009-11-17 12:26:41 PST
Too bad keys such as [Alt]-[Left arrow] don't work after following an anchor link. Would be so much easier tyo go back to the top/index...
Comment 42 Mark Banner (:standard8, limited time in Dec) 2009-11-17 12:52:22 PST
(In reply to comment #41)
> Too bad keys such as [Alt]-[Left arrow] don't work after following an anchor
> link. Would be so much easier tyo go back to the top/index...

By which you mean back and forward? This is something we purposely didn't include in this as we decided that as we're providing limited use actually in product, it is up to extension authors to implement these types of functions.

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