Closed Bug 516776 Opened 15 years ago Closed 15 years ago

Make it possible for browser elements to navigate through links/pages

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [no l10n impact])

Attachments

(5 files, 9 obsolete files)

6.67 KB, patch
philor
: review+
Details | Diff | Splinter Review
21.58 KB, patch
philor
: review+
Details | Diff | Splinter Review
1.77 KB, patch
philor
: review+
Details | Diff | Splinter Review
7.53 KB, application/octet-stream
Details
285.35 KB, application/octet-stream
Details
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.
Flags: blocking-thunderbird3+
Whiteboard: [no l10n impact]
Blocks: 490817
No longer blocks: 480533
Blocks: 487072
Whiteboard: [no l10n impact] → [no l10n impact][needs investigation onclick versus other methods][part patch ETA Wed]
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.
Yes -- the personas gallery is another example of interestingness.
Depends on: 523447
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.
Attached patch WIP (obsolete) — Splinter Review
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.
Whiteboard: [no l10n impact][needs investigation onclick versus other methods][part patch ETA Wed] → [no l10n impact][needs investigation onclick versus other methods][patch ETA Thursday]
Attached patch The fix (obsolete) — Splinter Review
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.
Attachment #408957 - Attachment is obsolete: true
Attachment #410238 - Flags: ui-review?(clarkbw)
Attachment #410238 - Flags: review?(philringnalda)
Oh and I should have said, this needs the patch from bug 523447 applied so that forms work in content tabs.
Whiteboard: [no l10n impact][needs investigation onclick versus other methods][patch ETA Thursday] → [no l10n impact][has patch][needs review philor,clarkbw]
Keywords: dev-doc-needed
Attached file Personas test extension (obsolete) —
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.
Attachment #410241 - Attachment is patch: false
Attachment #410241 - Attachment mime type: text/plain → application/octet-stream
Attached file Google Calendar Tab extension (obsolete) —
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 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.
Attachment #410238 - Flags: ui-review?(clarkbw) → ui-review+
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!"
Blocks: 526733
Attached patch The fix v2 (obsolete) — Splinter Review
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.
Attachment #410238 - Attachment is obsolete: true
Attachment #410473 - Flags: review?(philringnalda)
Attachment #410238 - Flags: review?(philringnalda)
Whiteboard: [no l10n impact][has patch][needs review philor,clarkbw] → [no l10n impact][has patch][needs review philor]
> 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()?
(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.
I was thinking more like:

<a href="#" onclick="window.open('http://www.google.com'); return false;">Click me</a>
(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).
> 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.
(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).
Attached patch The fix v3 (obsolete) — Splinter Review
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.
Attachment #410473 - Attachment is obsolete: true
Attachment #410765 - Flags: review?(philringnalda)
Attachment #410473 - Flags: review?(philringnalda)
> 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....
Attached patch The fix v4 (obsolete) — Splinter Review
Adds checking for trusted events and events which are set to prevent default.
Attachment #410765 - Attachment is obsolete: true
Attachment #410773 - Flags: review?(philringnalda)
Attachment #410765 - Flags: review?(philringnalda)
(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?
> 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.
Attached patch The fix v4 (the right version) (obsolete) — Splinter Review
Attachment #410773 - Attachment is obsolete: true
Attachment #410773 - Flags: review?(philringnalda)
(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.
> 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?
(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?
> 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.
Attached patch The fix v5 (obsolete) — Splinter Review
Minor typo fix for s/event/aEvent/
Attachment #410775 - Attachment is obsolete: true
Attachment #411177 - Flags: review?(philringnalda)
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.
Attachment #411178 - Flags: review?(philringnalda)
Attached patch Main fix v6Splinter Review
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.
Attachment #411177 - Attachment is obsolete: true
Attachment #411185 - Flags: review?(philringnalda)
Attachment #411177 - Flags: review?(philringnalda)
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.
Attachment #411186 - Flags: review?(philringnalda)
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.
Attachment #410244 - Attachment is obsolete: true
Personas test extension
Attachment #410241 - Attachment is obsolete: true
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.
Attachment #411178 - Flags: review?(philringnalda) → review+
Comment on attachment 411185 [details] [diff] [review]
Main fix v6

What, me worry?
Attachment #411185 - Flags: review?(philringnalda) → review+
Attachment #411186 - Flags: review?(philringnalda) → review+
Whiteboard: [no l10n impact][has patch][needs review philor] → [no l10n impact][has patch][ready to land]
Checked in:
http://hg.mozilla.org/comm-central/rev/2d246c8d3fa4
http://hg.mozilla.org/releases/comm-1.9.1/rev/60c4c14fce24
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [no l10n impact][has patch][ready to land] → [no l10n impact]
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
Depends on: 527664
Depends on: 529235
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...
(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.
You need to log in before you can comment on or make changes to this bug.