747519 - Port new doorhanger options from |Bug 711618 - implement basic click to play permission model|

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Bug 711618 added two things:
* two new options for the plugins click-to-play doorhanger, namely "Always/Never activate plugins for this site"
* Permission Manager integration

This bug is for porting the first part, bug 747186 is for the second part (Data Manager in our case).

Neil, I hope you don't mind me only doing the doorhanger part. I'm not even sure whether we should add two new buttons for the notification bar at all, but if you really feel the necessity, I'd either leave that part to you or a follow-up.

Note: For some reason, the Always option does not stick with the attached patch (as you can see in the DM, the permission is created correctly, but if you reload the page, the click-to-play plugin placeholder is back). Obviously we need to fix that, but I'm not sure what's wrong because it works in FF (current Nightly) and I just ported the code. Maybe we need to reset this.clickToPlayPluginsActivated (as you know, FF sets a property on the browser object instead, so it probably behaves differently), but where?

BTW I'm my test page is: http://www.java.com/de/download/testjava.jsp

Comment 1

[[e-mail]]

I had no problem with the flash plugin check page, but I'm one of those idiots using 64-bit Windows builds so I'd have to spin up a 32-bit build to test Java.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #1)
> I had no problem with the flash plugin check page, but I'm one of those
> idiots using 64-bit Windows builds so I'd have to spin up a 32-bit build to
> test Java.

We can surely agree on a different test page if it makes it easier for you. ;-)

Comment 3

[[e-mail]]

Comment on attachment 617079 [details] [diff] [review]
patch

>+            },{
Nit: space after comma (x2)

>+                var notification = PopupNotifications.getNotification("click-to-play-plugins",
>+                                                                      this.activeBrowser);
>+                if (notification)
>+                  notification.remove();
>+              }).bind(this)
Gavin agrees you don't need to remove the notification, it removes itself. (In which case you don't need the binding either, because pm is in scope.)

Comment 4

[[e-mail]]

(In reply to Jens Hatlak from comment #0)
> Neil, I hope you don't mind me only doing the doorhanger part. I'm not even
> sure whether we should add two new buttons for the notification bar at all
I think I should copy geolocation (which also uses secondary actions for doorhangers) and add a checkbox for notification boxes.

(In reply to Jens Hatlak from comment #2)
> We can surely agree on a different test page if it makes it easier for you.
FYI I was using http://www.adobe.com/software/flash/about/

(In reply to comment #3)
> Gavin agrees you don't need to remove the notification, it removes itself.
He's filed bug 747649 on this.

Comment 5

[[e-mail]]

Comment on attachment 617079 [details] [diff] [review]
patch

>+activatePluginsMessage.always=Always activate plugins for this site
>+activatePluginsMessage.never=Never activate plugins for this site
Prefer .label suffixes please.

Comment 6

[[e-mail]]

Comment on attachment 617079 [details] [diff] [review]
patch

>+                pm.add(this.activeBrowser.currentURI, "plugins",
Oops, you need to bind this here, I overlooked that, sorry.

Comment 7

[[e-mail]]

Comment on attachment 617079 [details] [diff] [review]
patch

I keep tripping over nits while writing the notification box version...

>+                        Components.interfaces.nsIPermissionManager.ALLOW_ACTION);
There's a const nsIPermissionManager for this... (same for deny of course)

Comment 8

[[e-mail]]

(In reply to comment #3)
> >+            },{
> Nit: space after comma (x2)
The (x2) was only transiently in my port. Ignore it.

Comment 9

[[e-mail]]

Attachment: Includes notificationbox changes

This is a rolled-up patch because I don't do interdiffs (or mq). Let me know if you feel up to reviewing the final patch, or whether I should ask IanN.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 617240 [details] [diff] [review]
Includes notificationbox changes

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

You forgot the l10n part so I cannot really test this...

I wonder if you tested the issue I had with the Always option not sticking (see initial comment).

::: suite/common/bindings/notification.xml
@@ +2031,5 @@
> +              label: this._stringBundle.GetStringFromName("activatepluginsMessage.always.label"),
> +              accessKey: this._stringBundle.GetStringFromName("activatepluginsMessage.always.accesskey"),
> +              callback: (function () {
> +                pm.add(this.activeBrowser.currentURI, "plugins",
> +                        Components.interfaces.nsIPermissionManager.ALLOW_ACTION);

Heh, you make me discover my own indentation mistakes! :)

(Here and below: As you said, s/Components\.interfaces\.//)

Comment 11

[[e-mail]]

(In reply to Jens Hatlak from comment #10)
> You forgot the l10n part so I cannot really test this...
Oops, that's the important bit too, because it needs to land before uplift.

> I wonder if you tested the issue I had with the Always option not sticking
> (see initial comment).
As I said, I couldn't duplicate it on the Flash page. I'll try building 32-bit.

> Heh, you make me discover my own indentation mistakes! :)
> 
> (Here and below: As you said, s/Components\.interfaces\.//)
Well, I did manage to fix it in my new code ;-)

Comment 12

[[e-mail]]

Attachment: Addressed review comments [Checkin: Comment 22]

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 617298 [details] [diff] [review]
Addressed review comments [Checkin: Comment 22]

Sorry, I don't like having the checkbox on the notification bar in addition and to the right of two buttons. IMHO it's important that a notification bar distracts as few as possible (remember, with CTP active, the notification bar might appear quite often!), and this violates it:
* there are too many elements in the area the user looks for interactivity (far right) and that place is taken by the least important element (the checkbox label)
* the most important one, "Activate plugins", is too far to the left (almost middle with small screens) since the checkbox and its label take so much space

Since "Don't activate" (without the checkbox) is pretty redundant anyway (you can use the close button at the far right), what about this:

Would you like to activate the plugins on this page? [Activate] [Always] [Never]

Of course you're free to discuss this with Ian instead. If you choose to go with your approach, be sure to ask him about the "Don't" (which I don't like in "formal" UI; bad enough it's already used with Geolocation).

I think we should come to a decision regarding the above first so that we can at least finalize the l10n part in time for the uplift.

Comment 14

[[e-mail]]

(In reply to Jens Hatlak from comment #13)
> Would you like to activate the plugins on this page? [Activate] [Always] [Never]
I had originally prototyped this before my comment 4 and the problem was that I wanted to make it clear that the activation was per-site without taking up space repeating the phrase for each button.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #14)
> (In reply to Jens Hatlak from comment #13)
> > Would you like to activate the plugins on this page? [Activate] [Always] [Never]
> I had originally prototyped this before my comment 4 and the problem was
> that I wanted to make it clear that the activation was per-site without
> taking up space repeating the phrase for each button.

Well, the sentence in front gives some context, but yes there's also some ambiguity. But do we really want to let perfect be the enemy of good?

[Note that we're talking about something that currently needs two prefs manually set by the user (only one of which will be exposed through UI mid-term) so I might rest my case, but obviously I'm more concerned about usability than 100% correctness.]

Comment 16

[[e-mail]]

(In reply to Jens Hatlak from comment #0)
> BTW I'm my test page is: http://www.java.com/de/download/testjava.jsp
I tested using http://www.java.com/en/download/testjava.jsp (naturally) and I could reproduce the bug, until I attached a debugger, at which point it started working. It also worked after a restart, or opening in a new tab or window.

Note that our code does not get called when the allow permission exists! So, unless someone has any bright ideas, I guess we'll just have to live with it.

(In reply to Jens Hatlak from comment #15)
> Note that we're talking about something that currently needs two prefs
Two? Which is the second?

Comment 17

[[e-mail]]

A possible tweak to move the checkbox before the buttons:
%s/box.appendChild(checkbox);/box.insertBefore(checkbox, box.firstChild);/

Comment 18

[[e-mail]]

Comment on attachment 617298 [details] [diff] [review]
Addressed review comments [Checkin: Comment 22]

Looking for feedback with or without comment #17.

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #16)
> It also worked after a restart.

Now I'm scared. Did you mean reload?

> Note that our code does not get called when the allow permission exists!

Then maybe we have to add some code outside of the binding, i.e. in normal JS code, similar to what FF does (where this works).

> So, unless someone has any bright ideas, I guess we'll just have to live with it.

Actually I don't want to live with it, because it means that "Allow for this website" won't work at all. Both with your test page (Flash) and mine (Java), if I store an Allow permission (checked with the Data Manager), it has no effect on reloads or restarts.

I do however agree that we're really close to the uplift, and the above issue does not concern l10n, so we can delay it and create a follow-up for that if nothing else works.

> > Note that we're talking about something that currently needs two prefs
> Two? Which is the second?

The two are (seemed obvious to me!):
* plugins.click_to_play
* browser.doorhanger.enabled

Comment 20

[[e-mail]]

(In reply to Jens Hatlak from comment #19)
> (In reply to comment #16)
> > It also worked after a restart.
> Now I'm scared. Did you mean reload?
No. As far as I'm concerned, everything works, with or without the doorhanger, except reloading the Java page. EDIT: But see below, sigh...

> > Note that our code does not get called when the allow permission exists!
> Then maybe we have to add some code outside of the binding, i.e. in normal
> JS code, similar to what FF does (where this works).
Sorry for not being clear, but when the allow permission exists, no chrome JS code is triggered at all, it's all done in the backend.

> Actually I don't want to live with it, because it means that "Allow for this
> website" won't work at all. Both with your test page (Flash) and mine
> (Java), if I store an Allow permission (checked with the Data Manager), it
> has no effect on reloads or restarts.
Would you believe it, I've now retested again, and everything works perfectly for me. I've no idea why; the only thing I changed was comment #17.

> > > Note that we're talking about something that currently needs two prefs
> > Two? Which is the second?
> The two are (seemed obvious to me!):
> * plugins.click_to_play
> * browser.doorhanger.enabled
I thought you said that you couldn't store the permission with the doorhanger enabled (default), which would indicate that it currently needs one pref.

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

Excuse me, I think I made a mistake. I thought I had started my investigations from a build that included the CTP back-end changes. Turns out that was not the case; instead I was only rebuilding the suite part. I just did a full rebuild and now everything (Flash, Java) works as expected. -> Nothing to do regarding the permission stickiness.

> > > > Note that we're talking about something that currently needs two prefs
> > > Two? Which is the second?
> > The two are (seemed obvious to me!):
> > * plugins.click_to_play
> > * browser.doorhanger.enabled
> I thought you said that you couldn't store the permission with the
> doorhanger enabled (default), which would indicate that it currently needs
> one pref.

I think I confused you. I was talking about the notification bar labels and texts (new issue, still TBD) here, not about permissions (old issue, solved now). What I was trying to say was this: You need to enable CTP (which will hopefully have Preferences UI soon) and disable doorhangers (for which we have no Preferences UI and likely won't have some any time soon) in order to see the notification bar for which we're in the process of discussing the UI design. So even if people enable CTP, they still need to disable doorhangers manually, without Preferences UI, in order to see what we discuss.

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 617298 [details] [diff] [review]
Addressed review comments [Checkin: Comment 22]

http://hg.mozilla.org/comm-central/rev/8e416a0c96c2

Neil and I discussed on IRC what to do with the Aurora uplift (= l10n freeze) so close and decided together to land this as-is for now (which he did in time, i.e. this is already on Aurora now) but continue the discussion here with a possibly different outcome for what we want to have in the end. IOW: Ian, still your call!

Comment 23

[Ian Neal]

Comment on attachment 617298 [details] [diff] [review]
Addressed review comments [Checkin: Comment 22]

I think I prefer the checkbox after the buttons, what happens for RTL users?

Comment 24

[[e-mail]]

(In reply to Ian Neal from comment #23)
> I think I prefer the checkbox after the buttons, what happens for RTL users?
I assume the whole line is reversed, i.e.
website this for Remember [X] [activate Don't] [plugins Activate] ?page this...

Comment 25

[Jens Hatlak (:InvisibleSmiley)]

I think this is done.