The default bug view has changed. See this FAQ.

Implement in product notifications to set up Sync

VERIFIED FIXED in mozilla6

Status

Cloud Services
Firefox Sync: UI
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: rags, Assigned: mak)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [tracking+ because it's new in 6], URL)

Attachments

(4 attachments, 11 obsolete attachments)

240.35 KB, image/png
shorlander
: review+
Details
1.26 KB, patch
Details | Diff | Splinter Review
30.97 KB, image/png
faaborg
: review+
Details
21.95 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Sync as a feature is virtually undiscoverable in product. While our goal is to have Sync operate in the background, transparently *after* you have set it up, that's no use if you don't even know that there is a feature called Sync.

Alex & I have talked about a simple notification mechanism that prompts users to set up Sync in Firefox 4 the first time they either save a password or store a bookmark. 

Alex will be posting mockups once he has them.

This should block final, but ideally we should get this into the next Beta so we can actually start ramping up new account creation now as opposed to waiting until GA.
(Reporter)

Comment 1

6 years ago
Madhava - is there something you can think of that would be appropriate for Fennec as well?
Mockup: http://people.mozilla.com/~faaborg/files/firefox4Mockups/visualBugs/syncPromotion/syncPromotion.htm
Assignee: faaborg → nobody
We might want to let these remove themselves after a certain number views.  Ragavan: what do you think our threshold is for assuming that the user simply isn't interested in setting up sync?
This bug is being tracked as part of this set: http://areweprettyyet.com/4/syncPromotion/
The first part of this mockup can be accomplished, happily, with an engagement snippet push which doesn't require us to block on Firefox 4.
blocking2.0: --- → -
(Assignee)

Comment 6

6 years ago
I'm trying to figure out the panels stuff, hope to have a reviewable patch before we close next Aurora merge.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 7

6 years ago
Created attachment 524865 [details]
screenshot v1

So, this is what I have so far, talking with Dao he had the impression this is a bit different from the usual information we give to the user, and that could leave the user surprised by the nature of this piece of UI.
So, here is some question for UX:
1. Should the background color match those of the other notifications (light-blue or yellow)?
2. Should we use the information icon rather than the Sync icon?
3. Should the text rather be more introductory and less imperative, like "Do you know you can..."

As it is in my patch, this appears 5 times then disappears, it does not appear if Sync is setup, does not appear anymore once dismissed.
(Assignee)

Updated

6 years ago
Attachment #524865 - Flags: feedback?(faaborg)
Comment on attachment 524865 [details]
screenshot v1

some comments:
-use a 16x16 sync icon
-let's have Stephen provide graphics for OS X, the grey isn't meshing well with the black panels.
-the short declarative statement is fine, trying to reduce the amount of text and avoid sounding too conversational
-I'm fine with these only showing up 5 times
Attachment #524865 - Flags: feedback?(faaborg) → feedback-
cc'ing stephen for a take on the OS X graphics, this also might mesh with some of the sharing work he's been doing recently, since those panels also have more than one z-order surface.

We want the message to fall back into the visual hierarchy so that it isn't the focus of the panel, and is just some additional tacked on information.
(In reply to comment #9)
> We want the message to fall back into the visual hierarchy so that it isn't the
> focus of the panel, and is just some additional tacked on information.

This seems to be related to the motivation that was behind the questions in comment 7, but without going far enough. Think of those users who are deeply puzzled whenever they're unexpectedly told to do something, to the extent that they think something fishy might be going on. The password notification seems particularly problematic here.

A visual hierarchy would say "the thing up there is important, but you should still deal with me at some point", whereas I think we need it to say "I'm peripheral, feel free to move on". Hence my suggestions: information icon or light bulb instead of the sync icon (no meaning to the target audience, potentially adds to the confusion), and "You can access your ..." instead of "Access your ...". Saving two words isn't a net win if it throws the user off for another two seconds. We might even go further and really be explicit about this being tacked-on information: "By the way, you can access your ..."
(Assignee)

Comment 11

6 years ago
Maybe we could be explicit about the fact this is additional option, by adding a "also" like "You can also access your...".
(Assignee)

Comment 12

6 years ago
Created attachment 525266 [details] [diff] [review]
patch v1.0

Saving a checkpoint of the work. This is identical to the mock-up on Windows, while styling on Linux and Mac is only guessed (I used a simple top border on Lin and a inset shadow on Mac). Waiting on Stephen for actual styling.

This can easily be converted to a binding if if we figure out this could be useful for other notifications, I did not do that because it has only 2 appears in 2 different popups, so looked like a useless overhead.

The questions in comment 10 and comment 11 are still open to answers.
(Assignee)

Comment 13

6 years ago
Sorry for typos in previous comment, I copy/pasted too much to reorder it, if in need of clarifications just ping me.
(Assignee)

Comment 14

6 years ago
Comment on attachment 525266 [details] [diff] [review]
patch v1.0

f?shorlander since this makes much easier to track bugs with queries.
This bug needs Mac and Linux styling, please.
Attachment #525266 - Flags: feedback?(shorlander)
(Assignee)

Comment 15

6 years ago
Created attachment 527003 [details] [diff] [review]
patch v1.1

Incorporated changes from Stephen.
Should be mostly complete on Windows and Mac, Linux theming has to be verified yet.
Attachment #524865 - Attachment is obsolete: true
Attachment #525266 - Attachment is obsolete: true
Attachment #525266 - Flags: feedback?(shorlander)
(Assignee)

Comment 16

6 years ago
Created attachment 527274 [details]
linux screenshot

linux styling so far is really simple (the menu button uses a similar simple border afaict)
Attachment #527274 - Flags: feedback?(shorlander)
(Assignee)

Comment 17

6 years ago
Created attachment 527276 [details] [diff] [review]
patch v1.2

some minor style fixes.
Attachment #527003 - Attachment is obsolete: true
Created attachment 529539 [details] [diff] [review]
patch v1.3

Updated mak's patch with Linux styling
Attachment #527276 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
Comment on attachment 527274 [details]
linux screenshot

the new styling is mostly this one with a background gradient (PS: don't trust bugzilla's diff), btw obsoleting screenshot.
Attachment #527274 - Attachment is obsolete: true
Attachment #527274 - Flags: feedback?(shorlander)
(Assignee)

Updated

6 years ago
Attachment #529539 - Flags: review?(dolske)
Comment on attachment 529539 [details] [diff] [review]
patch v1.3

Review of attachment 529539 [details] [diff] [review]:

Please get a response to comment 10.

::: browser/base/content/browser.xul
@@ +224,5 @@
       </hbox>
+      <hbox class="sync-promo-notification" hidden="true" align="start">
+        <hbox align="center" flex="1">
+          <image src="chrome://browser/skin/sync-24.png"
+                 class="sync-promo-icon"/>

Content shouldn't refer to image like this, belongs in the theme stylesheets instead.

::: browser/themes/gnomestripe/browser/browser.css
@@ +1421,5 @@
+  width: 18em;
+}
+#editBookmarkPanel .sync-promo-description {
+  width: 16em;
+}

Is this right for all locales?
Attachment #529539 - Flags: review-
(Assignee)

Comment 21

6 years ago
(In reply to comment #20)
> Comment on attachment 529539 [details] [diff] [review] [review]
> patch v1.3
> 
> Review of attachment 529539 [details] [diff] [review] [review]:
> 
> Please get a response to comment 10.

I got an answer from faaborg and it was positive about the fact the current phrasing is what we want. Comment 8 is still valid afaict.
Alex or Stephen?

> Content shouldn't refer to image like this, belongs in the theme stylesheets
> instead.

will do.

> ::: browser/themes/gnomestripe/browser/browser.css
> @@ +1421,5 @@
> +  width: 18em;
> +}
> +#editBookmarkPanel .sync-promo-description {
> +  width: 16em;
> +}
> 
> Is this right for all locales?

How can I tell that? I suppose the worse problem would be for shorter panels, since otherwise we just flex, should I just try all current supported locales?
(In reply to comment #21)
> I got an answer from faaborg and it was positive about the fact the current
> phrasing is what we want. Comment 8 is still valid afaict.
> Alex or Stephen?

Well... is comment 8 valid? Sure, it made two good points about the upsides of a short declarative statement, but I tried to put these in perspective, so I'd really like to get an actual response.

> > Is this right for all locales?
> 
> How can I tell that? I suppose the worse problem would be for shorter panels,
> since otherwise we just flex, should I just try all current supported locales?

What does "we just flex" mean, exactly? That the description would take up more space regardless of the specified width?
I just noticed that the password notification's description has a px max-width, which is of course wrong.
(Assignee)

Comment 23

6 years ago
(In reply to comment #22)
> What does "we just flex" mean, exactly? That the description would take up more
> space regardless of the specified width?

no, but empty space should take up the remaining space, there is no easy fix for this afaict, it's a platform bug.

> I just noticed that the password notification's description has a px max-width,
> which is of course wrong.

yeah, panels are still bogus, especially regarding width constraints.
> > I just noticed that the password notification's description has a px max-width,
> > which is of course wrong.
> 
> yeah, panels are still bogus, especially regarding width constraints.

Maybe, but the fact that it's 'px' rather than 'em' seems to unnecessarily make it worse. I wonder how this interacts with your patch with an increased font size.
>Well... is comment 8 valid? Sure, it made two good points about the upsides of
>a short declarative statement, but I tried to put these in perspective, so I'd
>really like to get an actual response.

We can stick "You can" in front of both, so it is more clearly optional.  I mostly just want to avoid a full n sales pitch, like "Did you know that now you can..." etc.
(Assignee)

Comment 26

6 years ago
I have an alternative idea for the width, I could set an empty description in the xul, runtime detect width of the current panel and set description and width. This should solve problems with locales and exotic px widths.
(Assignee)

Comment 27

6 years ago
Created attachment 530296 [details] [diff] [review]
patch v1.4

This implements a hack to set the description width dinamically, it workarounds the different locales/panels sizes. The only drawback (apart some more js) is that the promo text appears some milliseconds after the panel is shown. I've not found another way of taking into account the size of the dinamically styled images. Suggestions are welcome if you have better ideas.

Apart that, added "You can " in front of the statements, and moved the image src declaration to the css.
Attachment #529539 - Attachment is obsolete: true
Attachment #530296 - Flags: review?(dolske)
Attachment #530296 - Flags: feedback?(dao)
Attachment #529539 - Flags: review?(dolske)
Why aren't you just changing the 'px' width to 'em'?
(Assignee)

Comment 29

6 years ago
I think the old approach is fragile:
- needs to be manually tweaked per OS
- the available space is only guessed, may cause issues if the guess is not good
- may cause locale/dpi issues in edge cases
- it's harder to maintain, changes to the panels may easily break it
- may cause issues with different themes using larger icon or close button

Plus I'd not want to introduce some unwanted regression in panels by changing that size here, it should be in its own bug probably.
On the other side, the new approach allows to know exactly what's the available space and set the width accordingly.
The available space is determined by the theme anyway, isn't it? E.g. http://hg.mozilla.org/mozilla-central/annotate/44617683f1a5/toolkit/themes/winstripe/global/notification.css#l62
(Assignee)

Comment 31

6 years ago
Yes, for the passwords panel, but the promo appears also in bookmarks panel, and that panel may change size based on the locale. The fact that it appears in different kind of panels, and the available space may depend on images sizes makes that fragile. For example, just looking at panels in different Linux distros shows me different space due to different close button icons.
Comment on attachment 530296 [details] [diff] [review]
patch v1.4

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

r+ with the 0/-1 and label fixed.

::: browser/base/content/browser-doctype.inc
@@ +3,5 @@
>  %brandDTD;
>  <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd" >
>  %browserDTD;
> +<!ENTITY % syncBrandDTD SYSTEM "chrome://browser/locale/syncBrand.dtd">
> +%syncBrandDTD;

Is there any reason to keep syncBrand.dtd instead of just merging it into brand.dtd? Seems a bit silly to have a separate branding DTD just for sync, I assume this was just a accidental carryover from when Sync was a separate add-on?

(This would be fodder for a separate bug, but thought I'd ask here.)

::: browser/base/content/browser.js
@@ +8653,5 @@
> +    } catch(ex) {}
> +    // If the pref evaluates to -1, the notification is already hidden and there
> +    // is nothing left to do.
> +    if (viewsLeft == -1)
> +      return;

Why involve -1 here? Seems like it would be simpler to just make sure the pref is always >= 0, with 0 being the final state a user would eventually end up in.

@@ +8657,5 @@
> +      return;
> +
> +    try {
> +      // If the user has already setup Sync, don't show the notification.
> +      Services.prefs.getCharPref("services.sync.username");

This would be slightly less magical if you just explicitly checked .prefHasUserValue() instead of relying on the implicit throw.

if (prefHasUserValue("sync.username"))
  setIntPref("foo", viewsLeft = 0);
else
  setIntPref("foo", --viewsLeft)

@@ +8676,5 @@
> +      let textContent = descElt.firstChild.textContent;
> +      descElt.firstChild.textContent = "";
> +      this._panel.addEventListener("popupshown", function () {
> +        event.target.removeEventListener("popupshown", arguments.callee, true);
> +        descElt.width = descElt.getBoundingClientRect().width;

Should this be descElt.maxWidth? Eh, guess it doesn't matter.

::: browser/base/content/browser.xul
@@ +356,5 @@
> +      <hbox class="sync-promo-notification" hidden="true" align="start">
> +        <hbox align="center" flex="1">
> +          <image class="sync-promo-icon"/>
> +          <description flex="1" class="sync-promo-description">
> +            &syncPromoNotification.bookmarks.label; <description class="text-link inline-link" href="https://services.mozilla.com/">&syncPromoNotification.learnMoreLinkText;</description>

I think you meant to use syncPromoNotification.passwords.label here? It's not used anywhere else.
Attachment #530296 - Flags: review?(dolske) → review+
Also, can you attach updated screenshots for faaborg (or other UX person) to ui-r? I see a ui-r- on the first screenshot, but it's not clear if the current appearance is fine and happy for UX. [I didn't look at the specifics of the CSS color and such.]
(Assignee)

Comment 34

6 years ago
(In reply to comment #32)
> Is there any reason to keep syncBrand.dtd instead of just merging it into
> brand.dtd? Seems a bit silly to have a separate branding DTD just for sync,
> I assume this was just a accidental carryover from when Sync was a separate
> add-on?

will file a followup to kill it.

> Why involve -1 here? Seems like it would be simpler to just make sure the
> pref is always >= 0, with 0 being the final state a user would eventually
> end up in.

I did that to avoid doing a querySelector on each popupShowing and setting hidden to something I may already know it's hidden.  Since, per IRC discussion, you are fine with that minimal perf hit I'll stop at 0.

Will attach an updated patch after some sleep :)
(Assignee)

Comment 35

6 years ago
(In reply to comment #33)
> Also, can you attach updated screenshots for faaborg (or other UX person) to
> ui-r?

Will ask ui-r tomorrow, but since Shorlander worked directly on my patch, and I merged his CSS patches into it, it should be easy :)
(Assignee)

Updated

6 years ago
Whiteboard: [has review][needs new patch][needs ui-r]
(Assignee)

Updated

6 years ago
Blocks: 658512
(Assignee)

Comment 36

6 years ago
Created attachment 533975 [details] [diff] [review]
patch v1.5
Attachment #530296 - Attachment is obsolete: true
Attachment #530296 - Flags: feedback?(dao)
(Assignee)

Comment 37

6 years ago
Created attachment 533978 [details]
screenshot

Notice the Linux screen is from Fedora, I didn't have a build ready in Ubuntu, but Stephen tweaked the patch on it so he knows how it appears.
(Assignee)

Updated

6 years ago
Attachment #533978 - Flags: review?(shorlander)
(Assignee)

Updated

6 years ago
Whiteboard: [has review][needs new patch][needs ui-r] → [needs ui-r][land in services-central?]
Comment on attachment 533978 [details]
screenshot

ui-r+
Attachment #533978 - Flags: review?(shorlander) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [needs ui-r][land in services-central?] → [land in services-central]
http://hg.mozilla.org/services/services-central/rev/70b227f6aedc
Keywords: checkin-needed
Whiteboard: [land in services-central] → [fixed in services]
Created attachment 534084 [details] [diff] [review]
mochitest fix

Pushed this follow-up fix for a perma-orange in test_notifications.html: http://hg.mozilla.org/services/services-central/rev/0bae69db1d82
I would like us to use a 16x16 icon and grey text so that these don't visually compete as much with the main panel.
(Assignee)

Comment 42

6 years ago
I used a 16x16, and was requested to use a 24x24...
Yeah I am ok with greying out the text but the 16x16 icon looked pretty odd.
(Assignee)

Comment 44

6 years ago
on linux and mac greytext will be practically unreadable too.

Btw, I suspect this will be backed out since too many tests rely on notifications being always direct children of the panel. querySelector fail.
yeah colors are linux are a minefield with the range of OS themes, let's just do the greytext on Windows.  On OS X we could perhaps change the opacity, or just keep it the same, I don't spend enough time there to make that call.

for the icon, I want this to feel like a tacked on optional and informational message.  I'm concerned that at 24x24 the two areas start to become more equal in weight.
>the 16x16 icon looked pretty odd.

perhaps if we used more of a glyph version of the sync logo instead of the full color one?  As an object it looks unbalanced against the key/star, but we might be able to balance it horizontally against the close icon if we go for more of a glyph style.
Created attachment 534110 [details]
Glyph Style Sync Icon

(In reply to comment #46)
> >the 16x16 icon looked pretty odd.
> 
> perhaps if we used more of a glyph version of the sync logo instead of the
> full color one?  As an object it looks unbalanced against the key/star, but
> we might be able to balance it horizontally against the close icon if we go
> for more of a glyph style.

More like this style? It's more subtle. The normal Sync icon is a pretty dominant icon in that context.
(In reply to comment #44)
> Btw, I suspect this will be backed out since too many tests rely on
> notifications being always direct children of the panel. querySelector fail.

Yep. Had to back it out, too many tests were failing. Will need to fix these tests and get review for those.

http://hg.mozilla.org/services/services-central/rev/44f75de46b87
Whiteboard: [fixed in services]
Comment on attachment 534110 [details]
Glyph Style Sync Icon

yep looks good. the message isn't' as visually strong, and the height of the icon matches the height of the text well (which is why you were correct to use 24x24, now that I think about it).
Attachment #534110 - Flags: review+
Note about greytext, the italic styling would be aero (vista/7) only, non-italic greytext on XP as usual.

OS X looks good slightly lighter like Stephen mocked up, and on Linux we shouldn't mess with it because every time we do it is guaranteed to be unreadable in a certain set of cases.
(In reply to comment #45)
> for the icon, I want this to feel like a tacked on optional and
> informational message.

As mentioned earlier, a light bulb or information icon could communicate this in a more straightforward way. I've yet to understand why it's important to have the sync icon (or something resembling it) here.
Thanks everyone for jumping on this. If I understand correctly the last two open items are:

1. comment #48: Philip's changes need to be reviewed. Did this land on someones plate? Can we just use this bug to track that gets done or was another bug opened for it? 

2. comment #51: I think Dao is still waiting on a response. I'll add my opinion here, but anyone else can jump in.  Using the sync icon helps re-enforce the association of this icon with sync itself. This is good because in the preferences dialog the sync icon is present. So if the user does see this hint, then goes into preferences, hopefully the sync icon will be familiar and the user will go to the right place to set up/manage sync. My $0.02.
(In reply to comment #52)
> Thanks everyone for jumping on this. If I understand correctly the last two
> open items are:
> 
> 1. comment #48: Philip's changes need to be reviewed. Did this land on
> someones plate? Can we just use this bug to track that gets done or was
> another bug opened for it?

This isn't quite correct. My comment 48 was slightly confusing: the work that will have to reviewed hasn't been done yet. I started on Friday, but upon realizing that it touches hundreds of tests, I stopped. I brianstormed with Dolske a bit on IRC on what a good approach to changing those tests would be, and we came up with a few possible solutions.

I'll be happy to take point on this if Marco is busy. We can use this bug for this or spin it out to another one. In any case, this bug will depend on that work and we can't land before it has been done and reviewed.
Comment on attachment 533975 [details] [diff] [review]
patch v1.5

>+            &syncPromoNotification.bookmarks.label; <description class="text-link" href="https://services.mozilla.com/">&syncPromoNotification.learnMoreLinkText;</description>

Btw, this URL should be https://services.mozilla.com/sync.
Log of test failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Services-Central/1305915827.1305918287.11802.gz
(In reply to comment #52)
> 2. comment #51: I think Dao is still waiting on a response. I'll add my
> opinion here, but anyone else can jump in.  Using the sync icon helps
> re-enforce the association of this icon with sync itself. This is good
> because in the preferences dialog the sync icon is present. So if the user
> does see this hint, then goes into preferences, hopefully the sync icon will
> be familiar and the user will go to the right place to set up/manage sync.
> My $0.02.

The "learn more" link should cover that, right? Though it seems that that page desperately needs an update and overall simplification (as well as the real Sync icon!).
(Assignee)

Comment 57

6 years ago
So, actually, it's not hundreds of tests, it's mostly 3-4 tests that would need fix and the add-on manager code. But I'm not sure going this way will pay back, just looking at how this breaks the add-on manager, I think it may break extensions as badly. Today I'll try to go the custom binding way to see if I can retain compatibility.

Regarding styling, as far as I understand previous points:
- icon should have some minor opacity (?)
- italic greytext on Aero
- normal greytext on Classic and XP
- greytext on Mac
These are trivial, compared to solving the compatibility problem.

Finally, sorry for being late, lots of stuff happened in this release, and clearly I failed some planning here, 6 weeks is not a lot of time to fit everything and I guess we are still getting used to it.
(In reply to comment #57)
> So, actually, it's not hundreds of tests, it's mostly 3-4 tests that would
> need fix and the add-on manager code. But I'm not sure going this way will
> pay back, just looking at how this breaks the add-on manager, I think it may
> break extensions as badly. Today I'll try to go the custom binding way to
> see if I can retain compatibility.

We shouldn't make extensions drive the solution here (we already do far too often). If they break, the fix for them would be easy.

> Regarding styling, as far as I understand previous points:
> - icon should have some minor opacity (?)

No, we want to use the new glyph as mocked up by Stephen.

> - italic greytext on Aero
> - normal greytext on Classic and XP

Yup.

> - greytext on Mac

I don't think so. See comment 50: "OS X looks good slightly lighter like Stephen mocked up"

> Finally, sorry for being late, lots of stuff happened in this release, and
> clearly I failed some planning here, 6 weeks is not a lot of time to fit
> everything and I guess we are still getting used to it.

No problem. Thanks for doing so much work on this!
(In reply to comment #56)
> The "learn more" link should cover that, right? Though it seems that that
> page desperately needs an update and overall simplification (as well as the
> real Sync icon!).

See comment 54: the current patch doesn't point to the right page.
(Assignee)

Comment 60

6 years ago
(In reply to comment #58)
> We shouldn't make extensions drive the solution here (we already do far too
> often). If they break, the fix for them would be easy.

We are breaking valid use-cases, and internal code using them (like the add-on manager, and that's not something I want to touch 1 day before a merge).
While we are not driven by extensions, we should not do breakage unless needed.
I think I have a local version of the patch that should be compatible with tests and add-ons, I just have to solve a minor issue with styling and run tests on it to be sure.

> > Regarding styling, as far as I understand previous points:
> > - icon should have some minor opacity (?)
> 
> No, we want to use the new glyph as mocked up by Stephen.

Well, then I miss the images.

> > - greytext on Mac
> 
> I don't think so. See comment 50: "OS X looks good slightly lighter like
> Stephen mocked up"

Lighter is unclear, needs definition.
(In reply to comment #59)
> (In reply to comment #56)
> > The "learn more" link should cover that, right? Though it seems that that
> > page desperately needs an update and overall simplification (as well as the
> > real Sync icon!).
> 
> See comment 54: the current patch doesn't point to the right page.

I know, but I was redirected to http://www.mozilla.com/de/mobile/sync/. http://www.mozilla.com/en/mobile/sync/ seems better -- and has a gigantic Sync icon. The point is that there's no need to directly build up the brand in this very notification as if it were a billboard.
(Assignee)

Comment 62

6 years ago
Created attachment 534437 [details] [diff] [review]
patch v2.0

I've run locally all tests that were failing with the previous approach, additionally I pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=e9c052d5d788

TODO:
- [me] test on mac and linux (doing right now)
- [ux] finalize styling (gliph icon missing and Mac graytext needs definition)

Even if I still have these 2 outstanding TODOs, I'd like to start the review process early, reviewing minor styling interdiff should be trivial.
Anyone is free to send feedback, as usual.
Attachment #533975 - Attachment is obsolete: true
Attachment #534437 - Flags: review?(dolske)
(Assignee)

Comment 63

6 years ago
ehr, please ignore changes to browser.js and browser.properties, I removed those changes locally.
(Assignee)

Comment 64

6 years ago
Also removed locally implements="nsIDOMEventListener", unused leftover.
Tested Mac and Linux, no issues found so far.
(Assignee)

Comment 65

6 years ago
Created attachment 534573 [details] [diff] [review]
patch v2.1

addressed my previous comments, plus updated based on IRC discussion with Dolske to use .properties.
Attachment #534437 - Attachment is obsolete: true
Attachment #534573 - Flags: review?(dolske)
Attachment #534437 - Flags: review?(dolske)
Comment on attachment 534573 [details] [diff] [review]
patch v2.1

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

This basically looks fine, but talked with mak on IRC about hanging this UI off a new node in the existing arrowpanel binding, so that we don't have to duplicate it.

::: browser/base/content/urlbarBindings.xml
@@ +1369,5 @@
> +            // decided to setup Sync after noticing it.
> +            viewsLeft = 0;
> +          }
> +          else {
> +            this._viewsLeft = viewsLeft -1;

nit: spacing (viewsLeft - 1)

@@ +1375,5 @@
> +        }
> +
> +        // This has to be done regardless, since the panel could have been made
> +        // visible by a previous call.
> +        this._promobox.hidden = !viewsLeft;

It might help simplify things if you just set .hidden = true at the top of the function... Then the 2 |if (viewsLeft)| could merge together, and the box  unhidden within. (Oh, and I guess early return for when the sync username is already set).

But fine either way, I suppose.
(Assignee)

Comment 67

6 years ago
Created attachment 534630 [details] [diff] [review]
patch v3.0

third approach, as discussed on IRC.
Attachment #534573 - Attachment is obsolete: true
Attachment #534630 - Flags: review?(dolske)
Attachment #534573 - Flags: review?(dolske)
Comment on attachment 534630 [details] [diff] [review]
patch v3.0

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

::: browser/base/content/urlbarBindings.xml
@@ +1287,5 @@
> +    </content>
> +
> +    <implementation implements="nsIDOMEventListener">
> +      <constructor><![CDATA[
> +        Components.utils.reportError("construct");

Oops. Remove this. ;-)

@@ +1326,5 @@
> +      </property>
> +      <property name="_notificationType">
> +        <getter><![CDATA[
> +          let type = this._panel.firstChild.getAttribute("popupid") ||
> +                     this._panel.id;

Add a brief comment here explaining this.
Attachment #534630 - Flags: review?(dolske) → review+
(Assignee)

Comment 69

6 years ago
Created attachment 534640 [details] [diff] [review]
patch v3.1

Addressed comments, on try: http://tbpl.mozilla.org/?tree=Try&rev=b1f6dcdbcffa
Attachment #534630 - Attachment is obsolete: true
Landed: http://hg.mozilla.org/mozilla-central/rev/72d69ed97230

\o/\o/\o/\o/\o/

Thanks again, Marco, for taking on this work, and Dolske for the quick review. To be clear, what's the follow-up we still need to do for trunk + aurora? Just the different glyphs? I want to make sure we have those bugs on file.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Assignee)

Updated

6 years ago
Depends on: 659266
(Assignee)

Updated

6 years ago
Depends on: 659267
(Assignee)

Comment 71

6 years ago
I filed dependencies on the known missing UI bits.
The "Learn More" links from both the remember password and bookmark dialogs are broken.  Clicking them does nothing at all. Tested on m-c nightly build from today on Mac and Win XP.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 73

6 years ago
file a follow-up bug please and ask tracking ff6, don't reopen.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 74

6 years ago
Strategy to disable this feature on Aurora is either:
set browser.syncPromoViewsLeft = 0 (soft disable)
remove footertype="promobox" from arrow panels (hard disable)
tracking-firefox6: --- → ?

Updated

6 years ago
Depends on: 659335
tracking-firefox6: ? → +

Updated

6 years ago
Depends on: 659578

Updated

6 years ago
Whiteboard: potential feature disable for 6

Comment 75

6 years ago
Sorry if my status Whiteboard note caused confusion or concern. The note was *not* an indication that the release team has evaluated this feature and determined that it needs to be backed out. The Whiteboard note is just a record of why the release team is tracking this issue in the first place -- that it's a new feature that hasn't yet fully proved itself for this release. I've udpated the Whiteboard to say "[tracking+ because this is new feature in 6]" which should hopefully be less alarming.
Whiteboard: potential feature disable for 6 → [tracking+ because this is new feature in 6]
re bug 659335, it's verified in places and will be merged to m-c tomorrow and subsequently verified.
Depends on: 666875
Blocks: 666875
No longer depends on: 666875

Comment 77

6 years ago
Verified on 6.0 Beta, build 2:
Mac: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:6.0) Gecko/20100101 Firefox/6.0
Windows 7: Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0
Windows XP: Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0
Linux: Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Notifications to set up sync are visible (example: when bookmarking a website a message that promotes Sync is visible to the user)
Status: RESOLVED → VERIFIED

Comment 78

6 years ago
Mak, any others, it looks like the depends are all cleared. Is there anything else on anyone's radar that would keep this from shipping in Firefox 6?
Whiteboard: [tracking+ because this is new feature in 6] → [tracking+ because it's new in 6]
(Assignee)

Comment 79

6 years ago
I'm not aware of any other issue, so far.

Updated

6 years ago
tracking-firefox6: + → ---

Updated

5 years ago
Depends on: 708797
Depends on: 662565
Depends on: 662567
You need to log in before you can comment on or make changes to this bug.