Remember Password notification is cut off at the bottom

VERIFIED FIXED in Firefox 6

Status

()

Firefox
General
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: dao, Assigned: mak)

Tracking

({regression})

Trunk
Firefox 7
x86
Linux
regression
Points:
---

Firefox Tracking Flags

(firefox6- fixed)

Details

(Whiteboard: [fixed-in-places])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 535003 [details]
screenshot
(Assignee)

Comment 1

6 years ago
hm, looks like a wrong negative margin on the bottom.
(Reporter)

Updated

6 years ago
tracking-firefox6: --- → ?
(Assignee)

Comment 2

6 years ago
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?
(Assignee)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
I'm on Ubuntu 10.10.
(Assignee)

Comment 5

6 years ago
Created attachment 536178 [details] [diff] [review]
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).
(Assignee)

Updated

6 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED

Comment 6

6 years ago
feel free to request approval for 6 but not going to track this.
tracking-firefox6: ? → -
(Assignee)

Comment 7

6 years ago
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)
(Reporter)

Comment 8

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

seems to work for me
Attachment #536178 - Flags: feedback?(dao) → feedback+
(Assignee)

Comment 9

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

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-
(Assignee)

Comment 11

6 years ago
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)
(Assignee)

Comment 12

6 years ago
Created attachment 538031 [details] [diff] [review]
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).
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-
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Comment 16

6 years ago
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
(Assignee)

Comment 17

6 years ago
Created attachment 538093 [details] [diff] [review]
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.
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+
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
Baking on Places:
http://hg.mozilla.org/projects/places/rev/ba0f05780cba
Whiteboard: [fixed-in-places]
(Assignee)

Comment 21

6 years ago
http://hg.mozilla.org/mozilla-central/rev/ba0f05780cba
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
(Assignee)

Comment 22

6 years ago
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
(Assignee)

Comment 23

6 years ago
Created attachment 541666 [details] [diff] [review]
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
Attachment #541666 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 666875

Comment 25

6 years ago
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

Updated

6 years ago
Attachment #541666 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 26

6 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/2277f53482d9
status-firefox6: --- → fixed
You need to log in before you can comment on or make changes to this bug.