324642 - Show pings in the status bar when hovering over a link

Status: RESOLVED WONTFIX

(Core :: DOM: Navigation, enhancement)

Description

[Darin Fisher]

Show pings in the status bar when hovering over a link

The WhatWG spec for <a ping> recommends that the browser show users what pings will occur when they click on an anchor/area element.  An easy way to do this is to append some text to the status bar that normally just shows the HREF that will be followed.  Unfortunately, the API between docshell and browser front-end, nsIWebBrowserChrome::SetStatus, does not give us the flexibility to implement this UI change at the browser front-end.  So, without changing that embedding API, the next best thing is to have nsWebShell::OnOverLink format the text to include the pings before calling nsIWebBrowserChrome::SetStatus.

This is a suboptimal solution, but I think it's worth it for the purposes of initial UI.

Comment 1

[Darin Fisher]

Attachment: v1 patch

Comment 2

[Darin Fisher]

The result of this patch is status bar UI that looks like this:

 http://link.that.i.want.to.follow.com/foo/bar/baz.html [ping: bar.com, baz.com]

In other words, I'm only showing the hostnames of the URLs that will be pinged, and I'm also sorting that list and stripping out duplicates.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Darin, I suspect that for a typical use case where people actually use pings (various ecommerce sites) they'll probably never see text at the _end_ of the status message, since the URI typically has various junk including long session IDs embedded in it.  So I really think we need a better idea here; ideally what we need is a setup that lets the app decide what UI it wants.

Comment 4

[Darin Fisher]

Yes, I agree.  Two tasks: (1) figure out what that UI should be and (2) figure out how to expose the link hover information to the browser front-end.

For #1, I would appreciate suggestions as it seems like a hard problem to me.

For #2, we could just make the browser front-end implement a mouseover listener and use that to augment the information coming in via nsIWebBrowserChrome.

Thoughts?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

bz: don't we show the ellipsis in the middle of the status bar text? See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/browser/resources/content/nsBrowserStatusHandler.js&rev=1.62#122

(though, no idea whether firefox does that as well)

also: doesn't this allow a site to hide an "evil" site, by listing many urls?

Comment 6

[Darin Fisher]

> (though, no idea whether firefox does that as well)

A quick inspection of the status bar code didn't reveal any similar 'crop' code.


> also: doesn't this allow a site to hide an "evil" site, by listing many urls?

Yes.  Suggestions to get around that are welcome :)

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

well, all I can think of is to limit the characters per host, and the number of hosts...

Comment 8

[Boris Zbarsky [:bzbarsky]]

> For #1, I would appreciate suggestions as it seems like a hard problem to me.

Ccing some UI folks.  That's what they're for!  ;)

> For #2, we could just make the browser front-end implement a mouseover listener
> and use that to augment the information coming in via nsIWebBrowserChrome.

That begs the question of why we have the nsIWebBrowserChrome thing in some ways...

If this approach works, great.  If not, perhaps we should extend nsIWebBrowserChrome?

Again, I'd love to hear what Ben, Mike, and Neil think.

Comment 9

[Christian Persch (GNOME) (away; not receiving bug mail)]

You shouldn't require every embedder to middle ellipsisation; for example Epiphany ellipsises the statusbar text at the end.

(In reply to comment #4)
> Yes, I agree.  Two tasks: (1) figure out what that UI should be and (2) figure
> out how to expose the link hover information to the browser front-end.
>
> For #2, we could just make the browser front-end implement a mouseover listener
> and use that to augment the information coming in via nsIWebBrowserChrome.

How about instead of nsIWebBrowserChrome::SetStatus giving only the text, have ::SetStatusNode which gives the embedder the DOM node that's showing the statusbar link text? I've previously wanted this e.g. to figure out if the link will open in new window (target attr.), and could use that now to figure out the ping behaviour... OTOH, ::SetStatusNode would not be much less work than just having my own mouse event listener and setting the statusbar text from there...

Comment 10

[Darin Fisher]

Yes, good suggestion.

Comment 11

[[e-mail]]

(In reply to comment #6)
>>also: doesn't this allow a site to hide an "evil" site, by listing many urls?
>Yes.  Suggestions to get around that are welcome :)
I've seen a suggestion (I forget whether in Bugzilla or the newsgroups) that when the status bar is hidden the current link URL should be suffixed to the tooltip (or I guess display as the tooltip if the link wouldn't normally have one). Thus with pings you could end up with something like this:
+------------------------------+
|Click here to go to paypal now|
|http://www.evilsite.com/paypal|
|http://www.evilsite.net/paypal|
|http://www.evilsite.org/paypal|
+------------------------------+

Comment 12

[Doug Wright]

(In reply to comment #11)
That would involve bug 45375 getting fixed first.

Comment 13

[[e-mail]]

(In reply to comment #12)
>That would involve bug 45375 getting fixed first.
Not necessarily; we could fake it like we do for bookmark tooltips.

Comment 14

[Darin Fisher]

Attachment: patch: nsIWebBrowserChrome2 [fixed-on-trunk]

OK, this patch introduces nsIWebBrowserChrome2.  It provides SetStatusWithContext, which if implemented is called instead of nsIWebBrowserChrome::SetStatus.

Comment 15

[Darin Fisher]

Attachment: patch: make use of new status notification to show pings in the status bar

Comment 16

[Darin Fisher]

So, I left several callers of nsIWebBrowserChrome::SetStatus alone.  I could modify them to all call SetStatusWithContext, but that shouldn't be necessary since SetStatus must still be a valid API to call.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I should be able to get to this by tomorrow.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #4)
> Yes, I agree.  Two tasks: (1) figure out what that UI should be and (2) figure
> out how to expose the link hover information to the browser front-end.
> 
> For #1, I would appreciate suggestions as it seems like a hard problem to me.

We're making an assumption that a user wants to know (or should know) about these pings enough for them to make it into the primary UI. I'm not so sure that's the case. We don't surface information in the primary UI about what cookies are being written, nor about what scripts are being run. The <a ping> attribute doesn't seem like any more intrusive a technology, and I'm not sure (other than the recent firestorm of attention it's gotten in blogs) why there's a requirement to surface this information in a (relatively) prominent location.

This information:
 - doesn't assist the user in navigating the web
 - doesn't give the user information that they can act on
 - adds clutter to the status bar

Why are we convinced that this belongs here as opposed to in Page Info, View Source, or some other secondary view of the page content? 

Comment 19

[Darin Fisher]

Mike:

All good points.  Notice that there are two patches in this bug.  The first enables the second, but it also enables the browser UI to do other interesting things when the user hovers over a link.  I think it's a good patch regardless of how we modify the UI.

I haven't requested review on the second patch yet because it is just a prototype.  I agree that it is of questionable value.  I have another patch that adds the ping URLs to the link properties dialog, which to me seems very reasonable.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

I'll try to get to this on wednesday or so... possibly tomorrow

Comment 21

[Boris Zbarsky [:bzbarsky]]

> This information:
> - doesn't assist the user in navigating the web
> - doesn't give the user information that they can act on
> - adds clutter to the status bar

I agree with point three.  For points one and two, it lets the user decide whether to click the link, which I happen to think is a good thing for users to be able to decide.  Note that I would almost vote that we render links iwth ping differently somehow, except I suspect that would keep sites from ever using this...

Comment 22

[[e-mail]]

(In reply to comment #21)
>> This information:
>> - doesn't assist the user in navigating the web
>> - doesn't give the user information that they can act on
>> - adds clutter to the status bar
>I agree with point three.
Would a "ping" icon, along the lines of the cookie/popup icons, perhaps prefixed to the URL in the status bar, be too much clutter?

Comment 23

[Mike Connor [:mconnor]]

i don't think I'd put this in the URL bar, but it'd be useful in the status bar, IMO.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #19)
> All good points.  Notice that there are two patches in this bug.  The first

Yeah, I did notice, just wanted to throw in :) Sorry, I tend to come off stronger in writing that I usually mean to be.

> prototype.  I agree that it is of questionable value.  I have another patch
> that adds the ping URLs to the link properties dialog, which to me seems very
> reasonable.

That sounds very reasonable to me, too. We'll also want to consider (seperate bug, I think) what sort of options we want to give users over the control of this. My gut says that a simple "on/off" switch might be best, although we should at least consider the pros and cons of finer grained control as we offer with cookies.

(In reply to comment #21)
> be able to decide.  Note that I would almost vote that we render links iwth
> ping differently somehow, except I suspect that would keep sites from ever
> using this...

I also think that you overestimate the general browsing public's interest in this sort of thing. Click tracking isn't always nefarious, and I worry that we'll just be adding information that the common user won't know/care about. I think the right way to handle this, like other invisible actions that occur during browsing, is through privacy and content options.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> I also think that you overestimate the general browsing public's interest in
> this sort of thing.

That's very possible.

That said, Neil's suggestion (have mouseover show the link url and a little icon that can be clicked or hovered or whatever to learn where the tracking stuff goes) may not be a bad idea.  At least as far as I'm concerned.  ;)

I agree that the common user may not care about this much; at the same time a lot of users do, and that number is likely to grow as people become more aware of privacy issues...  I don't think having a pref to control this UI is the right approach (certainly not a pref with user-exposed UI)...

Anyway, I should stand by what I said earlier and let the people who do UI design hash this out.  ;)

Note also that, unfortunate as it is, there is a political aspect to this.  There are people who would attempt to spin this feature not having user-visible UI for as much as it's worth.  :(  Especially given the language in the HTML5 spec.

Comment 26

[Mark Smith [:mcs]]

Speaking as a Firefox user and Mozilla contributor, there is definitely a political aspect to this as well as a user trust issue.  That is probably why the Web Applications WhatWG spec. (http://www.whatwg.org/specs/web-apps/current-work/#ping) says:

   When the ping attribute is present, user agents should clearly indicate
   to the user that following the hyperlink will also cause secondary
   requests to be sent in the background, possibly including listing the
   actual target URIs.

Of course it is up to the Firefox community to decide how best to provide the clear indication.  In my opinion, showing the ping URIs in the link properties dialog a necessary but not sufficient step.  I'd like to see something shown on hover.  And please add preferences UI to allow users to disable pings.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 210042 [details] [diff] [review]
patch: nsIWebBrowserChrome2 [fixed-on-trunk]

nsWebShell.cpp

        ios->NewURI(NS_ConvertUTF16toUTF8(Substring(start, iter)),
                    doc->GetDocumentCharacterSet().get(),
                    doc->GetBaseURI(),
Hm... I think this should pass the content's base URI, not the document's. (I know this was a bug before too, but should be easy to fix)

Something else that was there before:
    httpInternal->SetDocumentURI(doc->GetBaseURI());

Shouldn't this use doc->GetDocumentURI()?


Finally, an edge case: If creating or initing the timer fails, should we maybe not open the channel, because we can't enfore the timeout?

nsIWebBrowerChrome2.idl

Maybe it should be mentioned that not necessarily all callers will use this new method, i.e. that setStatus needs to be implemented as well.

should the documentation allow context to be null, even if the type is STATUS_LINK? should it explicitly say that it will (or at least can) be null for other types?

r=me with those changes

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210042 [details] [diff] [review]
patch: nsIWebBrowserChrome2 [fixed-on-trunk]

In addition to biesi's comments:

>Index: xpfe/appshell/public/nsIXULBrowserWindow.idl

For 1.8 branch (if we take this there) we can't change this interface, right?

>+	/**
>+   * Tells the object implementing this function what link we are currently
>+   * over.
>+	 */

Weird indent.

>Index: xpfe/appshell/src/nsContentTreeOwner.cpp

>+NS_IMETHODIMP nsContentTreeOwner::SetStatus(PRUint32 aStatusType,
>+                                            const PRUnichar* aStatus)
>+{
>+  return SetStatusWithContext(aStatusType, nsDependentString(aStatus), nsnull);

You probably need to deal with a null aStatus here...

sr=bzbarsky with those and biesi's comments fixed.

Comment 29

[Darin Fisher]

Thanks for the comments guys.  I made all of the suggested changes.  I'm not sure yet if <a ping> will make it into FF2.  It seems somewhat unlikely--to be honest--given all of the emotional outcries against it.  So, I'm tempted to take the nsIXULBrowserWindow changes.  We can always revert the interface if the feature does find its way into FF2.  However, given the objection to the proposed status text changes, this patch may not even be necessary for FF2 in any case.

Comment 30

[Darin Fisher]

Comment on attachment 210042 [details] [diff] [review]
patch: nsIWebBrowserChrome2 [fixed-on-trunk]

I went ahead and landed the nsIWebBrowserChrome2 patch on the trunk w/ suggested fixes and mods to nsIXULBrowserWindow.

Comment 31

[Darin Fisher]

OK, marking this bug WONTFIX since showing pings in the status bar is just way too geeky.