Remember Password notification is cut off at the bottom

VERIFIED FIXED in Firefox 6

Status

()

defect
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: dao, Assigned: mak)

Tracking

({regression})

Trunk
Firefox 7
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6- fixed)

Details

(Whiteboard: [fixed-in-places])

Attachments

(3 attachments, 3 obsolete attachments)

Posted image screenshot
No description provided.
hm, looks like a wrong negative margin on the bottom.
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?
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.
I'm on Ubuntu 10.10.
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).
Assignee: nobody → mak77
Status: NEW → ASSIGNED
feel free to request approval for 6 but not going to track this.
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.
Attachment #536178 - Flags: feedback?(dao)
Comment on attachment 536178 [details] [diff] [review]
mozilla-aurora patch (no l10n changes)

seems to work for me
Attachment #536178 - Flags: feedback?(dao) → feedback+
Posted patch patch v1.1 (obsolete) — Splinter Review
unbitrot due to bug 659335, and removed spurious newlines from the previous patch.
Attachment #536178 - Attachment is obsolete: true
Attachment #536879 - Flags: review?(dolske)
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
Attachment #536879 - Flags: feedback-
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.
Attachment #536879 - Attachment is obsolete: true
Attachment #536879 - Flags: review?(dolske)
Posted patch patch v1.1 (obsolete) — Splinter Review
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).
Attachment #538031 - Flags: review?(dolske)
Attachment #538031 - Flags: feedback?(l10n)
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.
Attachment #538031 - Flags: feedback?(l10n) → feedback-
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.
Yes, please, two patches, and the hard-coded ' ' is an OK hack-around for aurora/beta.
Comment on attachment 536178 [details] [diff] [review]
mozilla-aurora patch (no l10n changes)

Reviving patch v1.0 (+ " " workaround) for mozilla-aurora.
Attachment #536178 - Attachment description: patch v1.0 → mozilla-aurora patch (no l10n changes)
Attachment #536178 - Attachment is obsolete: false
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.
Attachment #538031 - Attachment is obsolete: true
Attachment #538031 - Flags: review?(dolske)
Attachment #538093 - Flags: review?(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.
Attachment #538093 - Flags: review?(dolske) → review+
(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.
Baking on Places:
http://hg.mozilla.org/projects/places/rev/ba0f05780cba
Whiteboard: [fixed-in-places]
http://hg.mozilla.org/mozilla-central/rev/ba0f05780cba
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
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.
Attachment #536178 - Attachment is obsolete: true
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
Attachment #541666 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 666875
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.
Status: RESOLVED → VERIFIED
Attachment #541666 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.