Tabzilla: Create an out of date version warning for users not on the current version

RESOLVED FIXED

Status

Websites
Tabzilla
--
enhancement
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Tyler, Assigned: kohei)

Tracking

Details

(Whiteboard: [kb=1175446] [fxgrowth])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
We should create a warning that notifies users not on CurrentVersion -1 (or current ESR) that they are out of date and should update to the current version immediately.
I'm thinking something like the new language bar :kohei made. They're doing a similar thing on SUMO already.

Great idea! +1
OS: Windows XP → All
:kohei and :agibson, what do you think? This doesn't sound that hard to me. We could inject the current Fx version into the tabzilla.js as a variable, we already have good code for getting the Fx version from the UA string, then we do a bar similar to the translation bar :kohei just made. There was a screenshot of SUMO doing nearly this very thing floating around, I'll see if I can find it and attach it here. I just wanted to ask you to see if I'm missing some complexity here. As I see it this should be a pretty easy win :)

Thanks.
Flags: needinfo?(kohei.yoshino)
Flags: needinfo?(agibson)
(Assignee)

Comment 4

5 years ago
Yeah, let's do this! The Firefox base templates already have the latest and ESR versions so we can use them.
Flags: needinfo?(kohei.yoshino)
(Assignee)

Comment 5

5 years ago
I'm making my Translation Bar code more generic so this warning can be implemented easier.
https://github.com/mozilla/bedrock/pull/1258
(Assignee)

Comment 6

5 years ago
WIP: https://github.com/kyoshino/bedrock/commit/b9b2356

There will be one information bar at once to avoid confusion, so if this upgrade warning is shown, the translation bar won't appear.

Should we check the minor version also?
Assignee: nobody → kohei.yoshino
Severity: normal → enhancement
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 906943
Whiteboard: [kb=1175446]
(Assignee)

Comment 7

5 years ago
Sorry for the bugspam -- the Target Milestore was set unexpectedly.
(Assignee)

Comment 8

5 years ago
Bug 559111 is relevant. I don't know whether it should be implemented.
I think this sounds great! 

+1 for only showing one popup at a time, that was going to be my only recommendation.

I believe we only check the major version in other areas of bedrock, so probably best stuck to the same behavior for consistency and to avoid mixed messaging?
Flags: needinfo?(agibson)
(Assignee)

Comment 10

5 years ago
So we can simply check the major version. Here's an updated commit. Embedded the latest versions as variables as :pmac said, in order to make the version check work also on other sites. Still needs some tests.

https://github.com/kyoshino/bedrock/commit/e0e10e2

I was thinking about the design. It should be prominent... with a yellow background?

Comment 11

5 years ago
What is the reason to make this warning be part of the translation bar and not just put it in Tabzilla itself? If it is going to be tabzilla, why is it blocked by bug 90643?
(Assignee)

Comment 12

4 years ago
Updating the summary since we have removed the -1 logic. (Bug 935673)
Summary: Create an out of date version warning for users not on CurrentVersion -1 → Tabzilla: Create an out of date version warning for users not on the current cersion
(Reporter)

Comment 13

4 years ago
That's fine, please make sure to loop me into other update policy related discussions as this currentversion -1 logic was being used in other places and it's important we remain consistent. Thanks
(Assignee)

Comment 14

4 years ago
Created attachment 8350665 [details] [review]
Pull Request on GitHub
(Assignee)

Updated

4 years ago
Depends on: 952549
(Assignee)

Comment 15

4 years ago
Opps, a typo in the summary.

(In reply to Tyler Downer [:Tyler] from comment #13)
> That's fine, please make sure to loop me into other update policy related
> discussions as this currentversion -1 logic was being used in other places
> and it's important we remain consistent. Thanks

According to pmac's comment 1, the -1 logic has been used also on SUMO. I'll file a bug to remove it.
Summary: Tabzilla: Create an out of date version warning for users not on the current cersion → Tabzilla: Create an out of date version warning for users not on the current version
(Assignee)

Comment 16

4 years ago
Looks like the version check logic on SUMO is maintained manually. It could be replaced with this Update Bar for consistency and maintainability. Added Bug 922742 Comment 20.
Looking at the patch, this uses 3 strings, 2 of them we already have in the firefox/new age so we could just import them in tabzilla.lang, the last one we don't have, it is:

Update later.

That means that we need to get that translation done for all of our Firefox locales soon so as to cover as much as our userbase as possible. Are the strings used for this new feature complete and final?
(Assignee)

Comment 18

4 years ago
A messaging for ESR users has been discussed in Bug 952549, but I think those strings will be final.
(Reporter)

Comment 19

4 years ago
Curious on that status of this bug?
Flags: needinfo?(kohei.yoshino)
(Assignee)

Comment 20

4 years ago
Bug 952549 has been inactive for a while. We should solve that bug first...
Flags: needinfo?(kohei.yoshino)
Given what's now being proposed in bug 988725, is Tabzilla still a suitable place to house this functionality? We need to consider that Tabzilla can be used on any website, and not just *.mozilla.org domains. 

Just a thought for consideration.
(Reporter)

Comment 22

4 years ago
Alex,
That is an in-product change, so for the millions of users on out of date versions that we are trying to reach with this fix, it won't help. Also I see no problem with making tabzilla give a warning and another site using the same warning, the more warnings the better.
(In reply to Tyler Downer [:Tyler] from comment #22)
> Alex,
> That is an in-product change, so for the millions of users on out of date
> versions that we are trying to reach with this fix, it won't help. Also I
> see no problem with making tabzilla give a warning and another site using
> the same warning, the more warnings the better.

What I'm trying to say is that if this in-product change is restricted to a white-list of *.mozilla.org domains only, it would give inconsistent results for ESR users when Tabzilla is used on any other type of domain. There might not be many of those sites, but it's something to consider.
(Reporter)

Comment 24

4 years ago
Yes, I hear you, but we have to keep in mind that ESR users are 1. very few and 2, not our main objective of support. They have their own IT department, and they should be managing updates, so the ESR experience is the least of my concerns when compared to all the regular users.
(In reply to Tyler Downer [:Tyler] from comment #24)
> Yes, I hear you, but we have to keep in mind that ESR users are 1. very few
> and 2, not our main objective of support. They have their own IT department,
> and they should be managing updates, so the ESR experience is the least of
> my concerns when compared to all the regular users.

Roger, I just thought I'd point it out :)
Holly, to proceed with l10n for this bug can we please get confirmation on what the messaging should be here? Seems some of this was discussed in Bug 952549 but we're not quite clear on what the final strings should be. Thanks
Flags: needinfo?(hhabstritt.bugzilla)
(Assignee)

Comment 27

4 years ago
(In reply to Alex Gibson [:agibson] from comment #21)
> Given what's now being proposed in bug 988725, is Tabzilla still a suitable
> place to house this functionality? We need to consider that Tabzilla can be
> used on any website, and not just *.mozilla.org domains. 

Right, SUMO may want this, but most sites don't. Tabzilla.setupUpdatebar should not be in tabzilla.js. We can just *plug in* the function to Tabzilla's generic Infobar functionality. I'll update my pull request then.
(In reply to Kohei Yoshino [:kohei] from comment #27)
> Right, SUMO may want this, but most sites don't. Tabzilla.setupUpdatebar
> should not be in tabzilla.js. We can just *plug in* the function to
> Tabzilla's generic Infobar functionality. I'll update my pull request then.

Kohei, should we not wait to see the outcome of bug 988725 as well as discuss this with others first before updating your code?
(Assignee)

Comment 29

4 years ago
Oh I just updated my code :)

Bug 988725 won't block this work. Once the bug is solved, we can show ESR users a different message, but until then Firefox 24 users won't get any notifications.

As you see in the updated PR, SUMO can utilize updatebar.js if they want.
(Reporter)

Comment 30

4 years ago
If I understand correctly, this means that each site that wants this (MDN, AMO, SUMO, etc.) will have to specifically enable it? Or will this just be for all mozilla.org sites? Just determining the logistics
(Assignee)

Comment 31

4 years ago
Yes, this is an opt-in functionality like the Translation Bar (Bug 906943).
http://bedrock.readthedocs.org/en/latest/tabzilla.html#translation-bar

This Update Bar may not be needed by many sites, so I have excluded the code from tabzilla.js. Instead, if SUMO or other sites want to use it, they have to include updatebar.js then add an HTML attribute.
We've been discussing this a bit further on IRC. Treating 10 & 17 users all as not up-to-date makes sense. However, the Tabzilla banner style notification should be disabled on /new. (There is a separate in-page conditional message already existing on this page)

The banner copy used in bug 952549 was approved by Matej.  (See attachment: https://bug952549.bugzilla.mozilla.org/attachment.cgi?id=8356201)  "Update to stay fast and safe" links to /new and should be a button. "Later" should simply be a text link that closes the notification. 

Does this work for everyone?
Flags: needinfo?(hhabstritt.bugzilla)
(Assignee)

Comment 33

4 years ago
(In reply to Holly Habstritt [:Habber] from comment #32)
> the Tabzilla banner style notification should be disabled on /new.

Yes, Tabzilla's Update Bar will be disabled on /firefox/new/. I should have commented it in this bug earlier.

> The banner copy used in bug 952549 was approved by Matej.  (See attachment:
> https://bug952549.bugzilla.mozilla.org/attachment.cgi?id=8356201)  "Update
> to stay fast and safe" links to /new and should be a button. "Later" should
> simply be a text link that closes the notification. 

Looks like the current label in my PR is "Update later." as Habber recommended in Bug 952549 Comment 10. I'll change it to "Later" -- it's a simple, common label and I personally like it!

"Update to stay fast and safe" is the same label as /firefox/new/ so we won't need another l10n.
(Reporter)

Comment 34

4 years ago
So, whats the status of this?
(Reporter)

Comment 35

4 years ago
Any progress?

Comment 36

4 years ago
I'm going to pull this into Firefox Growth Team as we have found big gains from doing this on key websites. If nearly every website at Mozilla uses tabzilla, this bug could possibly get more older Firefox users to upgrade to the latest version more than one-off website notification.

:jpetto: can you scope out how much work this project would be?
Flags: needinfo?(jon)
Whiteboard: [kb=1175446] → [kb=1175446] [fxgrowth]
(Assignee)

Comment 37

4 years ago
I've found my WIP (linked in my comment 6) here but anyway, I believe this also depends on Bug 988725.
https://github.com/kyoshino/bedrock/commit/27966519eab78e5020cd247415acd0f2e37ae8d9
Depends on: 988725

Comment 38

4 years ago
I'll (re-)investigate dependencies and steps tomorrow and report back.

It'll be a huge win if we can use Kohei's WIP branch.
Flags: needinfo?(jon)

Comment 39

4 years ago
After reading through bug 988725, it appears we have a few options:

1) Wait for bug 988725 to be resolved so we can properly identify ESR users. Need to inquire about timeline.

2) Implement this update bar now, knowing that ESR users will be incorrectly prompted to update. Seems acceptable as per comment 24, but not ideal.

3) Use the UI Tour JS (https://github.com/Unfocused/mozilla-uitour) to get the full version (as mentioned in comment 46 on bug 988725 - https://bugzilla.mozilla.org/show_bug.cgi?id=988725#c46). Asked for verification that this option is feasible in bug 988725.

If available today, option #3 seems to be the best. The extra JS dependency isn't ideal, but would likely be temporary (assuming bug 988725 gets completed/resolved).

:kohei - Your WIP branch looks pretty close. How much more work do you think is needed to complete? Assuming we move forward with option #2 or #3 above, do you have time to finish? If not, I could take it on.
Flags: needinfo?(kohei.yoshino)
(Assignee)

Updated

4 years ago
No longer depends on: 988725
(Assignee)

Updated

4 years ago
Depends on: 1065525
(Assignee)

Comment 40

4 years ago
As I commented in Bug 988725, Mozilla.UITour.getConfiguration("appinfo", callback) can be probably used though it's still available only on Nightly (Firefox 35). We can copy a part of mozilla-uitour.js into tabzilla.js so there would be no extra JS dependency. I'll work on this and Bug 939470 (the same feature on /firefox/new/) next week.
Flags: needinfo?(kohei.yoshino)

Comment 41

4 years ago
My concern about the UITour variable is that it would only work on whitelisted domains and tabzilla is across almost all websites. I would prefer to prompt for a Firefox upgrade on Firefox visitors that are older than 3 major versions and ignoring ESR builds.

Comment 42

4 years ago
:kohei & jpetto: can we come up with a solution soon so we can move forward?

Comment 43

4 years ago
I think our best bet is to do as :cmore said - show the notice to users who are 3 major versions behind or more, while acknowledging that ESR users may see an incorrect message. If and when it becomes feasible to check for ESR users, we can update the code.

:kohei - Do you think you'll have time to finish your WIP branch this week? If not, I can take it on so we can get it out soon.
Flags: needinfo?(kohei.yoshino)
(Assignee)

Comment 44

4 years ago
I'll work on this later this week :)
Flags: needinfo?(kohei.yoshino)

Comment 45

4 years ago
Awesome! I'll be on the lookout for a PR.

Thanks :kohei!

Comment 46

4 years ago
:kohei - Think you'll have a PR ready for review tomorrow? Don't mean to be a pest, but we do want to get this out as soon as possible. Let me know if you're backed up or stuck on another task.

Thanks!
Flags: needinfo?(kohei.yoshino)
(Assignee)

Comment 47

4 years ago
I'm still thinking about consistency with the version check on /firefox/new/ (Bug 939470).

/firefox/new/ could probably use the UITour API to always show an accurate message, so I think it would be nice to share the same code/experience with this Update Bar, rather than showing the bar only to visitors on older than 3 major versions.

The Update Bar may only be used on www.mozilla.org and support.mozilla.org anyway, which are currently whitelisted and also requested in Bug 988725, so using the API won't be a major issue.

:cmore - what do you think?

I have tested my current WIP on Nightly, confirmed that the latest API provided the appinfo and the Update Bar was working with a test data set:

https://github.com/kyoshino/bedrock/compare/bug-935719-updatebar
Flags: needinfo?(kohei.yoshino) → needinfo?(chrismore.bugzilla)
(Assignee)

Comment 48

4 years ago
Created attachment 8499330 [details]
screenshot

Comment 49

4 years ago
Hm, so your proposed fix would only work on Fx 35 and up, correct? As far as I understand, we need to target all the existing users who are on Fx 29 or lower. There are many users who have not been updating and may not know that they are out of date. Using the UITour API is certainly the better technical solution, but wont help the users who are currently behind.
A combination of the two should work as well right? Use the accurate information if available, and fall back to the "3 major versions back" method if not?

Comment 51

4 years ago
(In reply to Paul McLanahan [:pmac] from comment #50)
> A combination of the two should work as well right? Use the accurate
> information if available, and fall back to the "3 major versions back"
> method if not?

This absolutely sounds like the way to go.
(Assignee)

Comment 52

4 years ago
Okay, I will add a fallback for 34 and below.
(Assignee)

Comment 53

4 years ago
Created attachment 8499898 [details] [review]
Pull Request on GitHub

Sorry I'm late... Here's an updated PR for this.
Attachment #8350665 - Attachment is obsolete: true

Comment 54

4 years ago
Thanks for the PRs!
Flags: needinfo?(chrismore.bugzilla)

Comment 55

4 years ago
Can we see a working demo of this after it is merged and will mozilla.org require an additional PR to opt-in to the notice?

Comment 56

4 years ago
I can probably get the branch on a demo server today - I think two will free up after the push. I'll update this bug when it's up.

The PR enables the update bar by default for all of mozilla.org. Individual pages can opt out if/when necessary. The only page opting out initially will be /firefox/new/ (as it already has the same logic). Other sites using Tabzilla can configure which notification bars they want in their settings:

https://github.com/mozilla/bedrock/pull/2339/files#diff-7194d3de4cc9718f18e49e996f1e50b0R1057

Comment 57

4 years ago
The branch is now up on demo5:

https://www-demo5.allizom.org/en-US/

If you set your user agent string to a Firefox at version 30 or below, you'll see the update bar (unless visiting the /firefox/new/ page).

Comment 58

4 years ago
Commits pushed to master at https://github.com/mozilla/bedrock

https://github.com/mozilla/bedrock/commit/23b39bade21228ac054e3c8f428c2c06f7f23310
Fix Bug 935719 - Tabzilla: Create an out of date version warning for users not on the current version

https://github.com/mozilla/bedrock/commit/6a0f68882f084bb038f2874ec021b7d38351a7dc
Merge pull request #2339 from kyoshino/bug-935719-updatebar

Fix Bug 935719 - Tabzilla: Create an out of date version warning for users not on the current version

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
See Also: → bug 1092335
(Assignee)

Updated

3 years ago
Depends on: 1125166
You need to log in before you can comment on or make changes to this bug.