820109 - "Check for updates..." doesn't work in CTP doorhanger

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Alex Keybl [:akeybl]]

"Check for updates..." doesn't work from the CTP doorhanger. No page is opened in my testing.

Comment 1

[Alex Keybl [:akeybl]]

We're targeting this for landing in tomorrow's beta, if at all possible.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

This should do the trick.

Comment 3

[Dão Gottwald [:dao]]

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

>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml
>@@ -1432,17 +1432,17 @@
>           <xul:spacer flex="1"/>
>           <xul:button class="popup-notification-menubutton center-item-button"
>                       oncommand="document.getBindingParent(this).runCallback();"
>                       xbl:inherits="label=buttonlabel"/>
>         </xul:hbox>
>         <xul:hbox flex="1" align="center" class="center-item-warning">
>           <xul:image class="center-item-warning-icon"/>
>           <xul:label class="center-item-warning-description" xbl:inherits="xbl:text=warningText"/>
>-          <xul:label xbl:inherits="href=updateLink" value="&checkForUpdates;" class="text-link"/>
>+          <xul:label xbl:inherits="href=updateLink" value="&checkForUpdates;" class="text-link" onclick="event.preventDefault(); gBrowser.loadOneTab(this.href, { inBackground: false });"/>

Use openUILinkIn rather than gBrowser.loadOne tab.

Why are you using onclick and not oncommand?

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Dão Gottwald [:dao] from comment #3)
> Why are you using onclick and not oncommand?

It doesn't look like using oncommand works.

Comment 5

[Dão Gottwald [:dao]]

Will onclick be triggered by the Enter key here?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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

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

::: browser/base/content/urlbarBindings.xml
@@ +1436,5 @@
>          </xul:hbox>
>          <xul:hbox flex="1" align="center" class="center-item-warning">
>            <xul:image class="center-item-warning-icon"/>
>            <xul:label class="center-item-warning-description" xbl:inherits="xbl:text=warningText"/>
> +          <xul:label xbl:inherits="href=updateLink" value="&checkForUpdates;" class="text-link" onclick="event.preventDefault(); gBrowser.loadOneTab(this.href, { inBackground: false });"/>

oncommand probably won't work here because this is a xul:label. If this is changed to a xul:button then oncommand should work and we can style it like a link.

Comment 7

[Paul Silaghi, QA [:pauly]]

(In reply to Alex Keybl [:akeybl] from comment #0)
> "Check for updates..." doesn't work from the CTP doorhanger. No page is
> opened in my testing.

I'm not seeing the same behavior.
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=793338#c13 and bug 797976 which handles this.

Comment 8

[Alex Keybl [:akeybl]]

Presumably David is also able to repro, since he's fixing this bug.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

The behavior I was seeing with my local debug build was that after clicking the link, the browser would sit there for ages and then open up the plugin check page in a new window. I thought the issue was a) the responsiveness and b) that it was in a new window, not a new tab. I knew we had a bug on the link not opening in a new tab (thanks Paul), but I couldn't find it, so I figured I'd just fix it in this bug.

Actually, now that I look at it, bug 797976 is maybe more of a general toolkit problem, whereas we just need to fix this behavior here once.

Dao - looks like onclick is triggered by the enter key, but not the spacebar.

Jared - is there already a way of styling a button like a link or would we have to put something together?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

If Dao is OK with using the label then that's fine with me.

If you want to style a button like a link, you can try:
button {
  -moz-appearance: none;
  color: -moz-nativehyperlinktext;
  text-decoration: underline;
}
button:active {
  color: -moz-activehyperlinktext;
}

Comment 11

[Dão Gottwald [:dao]]

I'm OK with using a text-link label, but we should add a comment pointing to the underlying bug that you're working around.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

Ok - I added the comment and changed the handler to use openUILinkIn.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 691077 [details] [diff] [review]
patch v2

I'm targeting Firefox 20 with bug 263433, so we should only land this on aurora and beta.

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 691077 [details] [diff] [review]
patch v2

Pre-approving for branches given the fact that this should be a minimal risk fix, and easily verifiable.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch for aurora

Had to re-base for the test. This is the patch for aurora.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch for beta

Had to rebase for the test. This is the patch for beta.

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Apparently I can't decide how to spell rebase :)

Anyway, here's the pushes to aurora and beta:

https://hg.mozilla.org/releases/mozilla-aurora/rev/0f7f10745389
https://hg.mozilla.org/releases/mozilla-beta/rev/58ca79c2a77a

Comment 18

[Dão Gottwald [:dao]]

Resolving, since there's nothing else to do here. Fixed by bug 263433 on mozilla-central.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/58ca79c2a77a

Comment 20

[Paul Silaghi, QA [:pauly]]

Actually FF 20 is affected.

Comment 21

[Dão Gottwald [:dao]]

Firefox 20 is being taken care of in bug 263433.

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 692432 [details] [diff] [review]
patch for beta

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed (well, maybe "desired" is a better adjective) for click-to-play
User impact if declined: the "check for updates..." link in the popup notification will open a new window instead of a new tab
Fix Landed on Version: 18
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/aea328a61fd6

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 18b5 Win 7 x64, Ubuntu 12.04 and Mac OS X 10.8.2

Comment 25

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 17.0.2 ESR on Win 7 x64.

Comment 26

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 19b3 on Win 7, Mac OS X 10.8.2.

Comment 27

[Paul Silaghi, QA [:pauly]]

While testing this, I've noticed that Java and Flash CTP notifications are a little different. Java doesn't have a "check for updates" option.
http://img221.imageshack.us/img221/6690/ctpjavadifferentflash.png
Is this known ?

Comment 28

[Georg Fritzsche [:gfritzsche]]

Note that on(In reply to Paul Silaghi [QA] from comment #27)
> While testing this, I've noticed that Java and Flash CTP notifications are a
> little different. Java doesn't have a "check for updates" option.
> http://img221.imageshack.us/img221/6690/ctpjavadifferentflash.png
> Is this known ?

Note that one of them is marked "vulnerable", while the other is marked "outdated". We only show "check for updates" on "outdated" plugins.

Comment 29

[Robert Kaiser]

Yes, there is no Java version that is known to be current and secure at this time, esp. as Oracle's record in updating for security vulnerabilities is sketchy at best. There are also plans to not just generally require CTP for any Java, but for any plugin except current Flash.

And for Flash, we are only CTP-blocking really outdated versions right now, so you can get out of the block with an update. Adobe is also pretty agile with updates and working closely with us to fix issues.