659578 - Remember Password notification is cut off at the bottom

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 1

[Marco Bonardo [:mak]]

hm, looks like a wrong negative margin on the bottom.

Comment 2

[Marco Bonardo [:mak]]

Hm, I cannot reproduce the bug nor in Ubuntu 10.04 nor in Fedora 14. Could you please give me more information on your OS, maybe it's another version of Ubuntu or another window manager?

Comment 3

[Marco Bonardo [:mak]]

Maybe it's the fact you only have "Remember password on <domain>?" on one line, while I have "Remember password for <user> on <domain>?" on two lines, and this causes a different layout flow.

Comment 4

[Dão Gottwald [:dao]]

I'm on Ubuntu 10.10.

Comment 5

[Marco Bonardo [:mak]]

Attachment: mozilla-aurora patch (no l10n changes)

Could you please try this patch? Looks like setting textContent doesn't correctly reflow the height of the box, so I have to do that manually, as for the width :(

Also, margins in css were wrong, and looks like using bind on a listener function confuses arguments.callee (may make sense, since it generates a new Function).

Comment 6

[Asa Dotzler [:asa]]

feel free to request approval for 6 but not going to track this.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 536178 [details] [diff] [review]
mozilla-aurora patch (no l10n changes)

Feedback requests are easier to track.  You're free to review if you wish, otherwise I'd just appreciate to know if you can confirm it works as intended on your system, and I will ask review to Dolske.

Locally I've fixed those spurious newlines in the binding.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 536178 [details] [diff] [review]
mozilla-aurora patch (no l10n changes)

seems to work for me

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.1

unbitrot due to bug 659335, and removed spurious newlines from the previous patch.

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 536879 [details] [diff] [review]
patch v1.1

>+              self._panel.removeEventListener("popupshown", arguments.callee, true);
>+              self._promomessage.width = self._promomessage.getBoundingClientRect().width;
>+              self._promomessage.firstChild.textContent = self._notificationMessage + " ";

This whitespace should be part of the preceeding string or a separate string so it's localizable. We do this in other places, too, e.g. https://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/syncQuota.properties#37

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 536879 [details] [diff] [review]
patch v1.1

yes, makes sense, new patch coming.
In case there should be problem for Aurora approval of that minimal l10n change we'll fallback.

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.1

This should be the right approach for l10n based on previous comments. Pike, do you think we may get Aurora approval with this change or do I need to bring on 2 versions of the patch (actually the fallback would do what's in the quoted code in comment 10).

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 538031 [details] [diff] [review]
patch v1.1

We'll need to change the string for this, thus this requires the entity name to change, and also that this can't land like this on aurora.

The patch is very right for central though, getting whitespace into the localization is crucial.

Comment 14

[Marco Bonardo [:mak]]

Ehr, right, I should have changed the property names, damn me!
Btw, "this can't land like this on aurora" is a clear indication we need to fallback to a simple string concat there. I'll bring on 2 patches.

Comment 15

[Axel Hecht [:Pike]]

Yes, please, two patches, and the hard-coded ' ' is an OK hack-around for aurora/beta.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 536178 [details] [diff] [review]
mozilla-aurora patch (no l10n changes)

Reviving patch v1.0 (+ " " workaround) for mozilla-aurora.

Comment 17

[Marco Bonardo [:mak]]

Attachment: mozilla-central patch

This changes the properties names from syncPromoNotification.bookmarks.label to syncPromoNotification.bookmarks.description (same for passwords, and it's a description text after all).
Apart the l10n change the 2 patches are doing same exact stuff.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 538093 [details] [diff] [review]
mozilla-central patch

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

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +318,5 @@
>  
>  # LOCALIZATION NOTE (syncPromoNotification.bookmarks.label): This appears in
>  # the add bookmark star panel.  %S will be replaced by syncBrandShortName.
> +# The final space separates this text from the Learn More link.
> +syncPromoNotification.bookmarks.description=You can access your bookmarks on all your devices with %S.\u0020

Blah, hacky. Can you use "-moz-margin-end: 1em" instead? I guess this is fine if you can't, though.

Comment 19

[Marco Bonardo [:mak]]

(In reply to comment #18)
> >  # LOCALIZATION NOTE (syncPromoNotification.bookmarks.label): This appears in
> >  # the add bookmark star panel.  %S will be replaced by syncBrandShortName.
> > +# The final space separates this text from the Learn More link.
> > +syncPromoNotification.bookmarks.description=You can access your bookmarks on all your devices with %S.\u0020
> 
> Blah, hacky. Can you use "-moz-margin-end: 1em" instead? I guess this is
> fine if you can't, though.

No, some locale needs to be able to localize the spaces, see discussion above, the -moz-margin-end would move the problem from localization to theme, that would be wrong as it was adding it in js.

Comment 20

[Marco Bonardo [:mak]]

Baking on Places:
http://hg.mozilla.org/projects/places/rev/ba0f05780cba

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ba0f05780cba

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 536178 [details] [diff] [review]
mozilla-aurora patch (no l10n changes)

btw, I need an updated path for Aurora, for unbitrot, so obsoleting this one and preparing a new one.

Comment 23

[Marco Bonardo [:mak]]

Attachment: mozilla Aurora patch (no l10n changes)

This is the same patch as central, without l10n changes.
The patch should be safe enough, it's limited to that specific binding, it solves:
- linux: the panel is cut at the bottom and has wrong margins
- all platforms: missing whitespace before the "Learn more" link
- all platforms: the popupshown listener is not removed correctly

Comment 25

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110627 Firefox/7.0a1

Verified issue and it is no longer present. Setting status to Verified Fixed.

Comment 26

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/2277f53482d9