Last Comment Bug 659578 - Remember Password notification is cut off at the bottom
: Remember Password notification is cut off at the bottom
Status: VERIFIED FIXED
[fixed-in-places]
: regression
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: Firefox 7
Assigned To: Marco Bonardo [::mak]
:
Mentors:
: 666875 (view as bug list)
Depends on:
Blocks: 618913
  Show dependency treegraph
 
Reported: 2011-05-25 01:06 PDT by Dão Gottwald [:dao]
Modified: 2013-12-27 14:31 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
screenshot (17.97 KB, image/png)
2011-05-25 01:06 PDT, Dão Gottwald [:dao]
no flags Details
mozilla-aurora patch (no l10n changes) (2.63 KB, patch)
2011-05-30 14:51 PDT, Marco Bonardo [::mak]
dao+bmo: feedback+
Details | Diff | Review
patch v1.1 (3.86 KB, patch)
2011-06-02 07:24 PDT, Marco Bonardo [::mak]
philipp: feedback-
Details | Diff | Review
patch v1.1 (5.35 KB, patch)
2011-06-08 08:59 PDT, Marco Bonardo [::mak]
l10n: feedback-
Details | Diff | Review
mozilla-central patch (6.06 KB, patch)
2011-06-08 12:11 PDT, Marco Bonardo [::mak]
dolske: review+
Details | Diff | Review
mozilla Aurora patch (no l10n changes) (3.10 KB, patch)
2011-06-24 05:38 PDT, Marco Bonardo [::mak]
asa: approval‑mozilla‑aurora+
Details | Diff | Review

Description Dão Gottwald [:dao] 2011-05-25 01:06:13 PDT
Created attachment 535003 [details]
screenshot
Comment 1 Marco Bonardo [::mak] 2011-05-25 02:59:13 PDT
hm, looks like a wrong negative margin on the bottom.
Comment 2 Marco Bonardo [::mak] 2011-05-30 12:41:06 PDT
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] 2011-05-30 12:47:15 PDT
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] 2011-05-30 12:48:51 PDT
I'm on Ubuntu 10.10.
Comment 5 Marco Bonardo [::mak] 2011-05-30 14:51:57 PDT
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).
Comment 6 Asa Dotzler [:asa] 2011-05-31 14:44:30 PDT
feel free to request approval for 6 but not going to track this.
Comment 7 Marco Bonardo [::mak] 2011-05-31 18:43:06 PDT
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] 2011-06-02 02:21:03 PDT
Comment on attachment 536178 [details] [diff] [review]
mozilla-aurora patch (no l10n changes)

seems to work for me
Comment 9 Marco Bonardo [::mak] 2011-06-02 07:24:59 PDT
Created attachment 536879 [details] [diff] [review]
patch v1.1

unbitrot due to bug 659335, and removed spurious newlines from the previous patch.
Comment 10 Philipp von Weitershausen [:philikon] 2011-06-07 09:53:57 PDT
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] 2011-06-07 09:57:35 PDT
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] 2011-06-08 08:59:12 PDT
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).
Comment 13 Axel Hecht [:Pike] 2011-06-08 10:54:36 PDT
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] 2011-06-08 11:49:54 PDT
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] 2011-06-08 11:57:56 PDT
Yes, please, two patches, and the hard-coded ' ' is an OK hack-around for aurora/beta.
Comment 16 Marco Bonardo [::mak] 2011-06-08 12:05:09 PDT
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] 2011-06-08 12:11:12 PDT
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.
Comment 18 Justin Dolske [:Dolske] 2011-06-22 18:12:54 PDT
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] 2011-06-23 03:18:48 PDT
(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] 2011-06-23 05:38:38 PDT
Baking on Places:
http://hg.mozilla.org/projects/places/rev/ba0f05780cba
Comment 21 Marco Bonardo [::mak] 2011-06-23 13:47:22 PDT
http://hg.mozilla.org/mozilla-central/rev/ba0f05780cba
Comment 22 Marco Bonardo [::mak] 2011-06-23 13:56:53 PDT
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] 2011-06-24 05:38:19 PDT
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
Comment 24 Daniel Holbert [:dholbert] 2011-06-24 09:08:44 PDT
*** Bug 666875 has been marked as a duplicate of this bug. ***
Comment 25 George Carstoiu 2011-06-28 07:32:45 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.