new bookmark dialog: remove bookmark button too small for localization

VERIFIED FIXED in Firefox 62

Status

()

defect
P1
normal
VERIFIED FIXED
Last year
4 months ago

People

(Reporter: soeren.hentzschel, Assigned: dao)

Tracking

(Blocks 1 bug)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62+ verified, firefox63 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments)

Reporter

Description

Last year
Posted image screenshot
As you can see in the attached screenshot the translation of the "remove bookmark" button is longer than the button itself.
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee

Updated

Last year
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Assignee

Comment 1

Last year
I have a patch that allows these buttons to grow to multiple lines but that looses the accesskey underline.

Alternatively, we could allow one of the two buttons to consume more than 50% of the width, but I'm not sure that we want that, how exactly to implement it, or that this even fixes the problem for all locales.

Yet another option would be to use an ellipsis and a tooltip but that's clearly suboptimal.

Opinions or other ideas, anyone?
Flags: needinfo?(mak77)
Flags: needinfo?(abenson)
We could evaluate enlarging the panel enough for the longest localization we have at hand, or we could make the panel width settable by the locale itself through dtd, like we used to do with other panels.
Flags: needinfo?(mak77)
Tracking this so we remember to uplift if y'all come up with a fix.
Duplicate of this bug: 1471847
So, what about we just make the width localize-able?
Flags: needinfo?(dao+bmo)
> Alternatively, we could allow one of the two buttons to consume more than 50% of the width, but I'm not sure that we want that, how exactly to implement it, or that this even fixes the problem for all locales.

Removing display: flex; from #editBookmarkPanelBottomButtons seems to work for this solution.
(In reply to Tim Nguyen :ntim from comment #6)
> > Alternatively, we could allow one of the two buttons to consume more than 50% of the width, but I'm not sure that we want that, how exactly to implement it, or that this even fixes the problem for all locales.

It's only a partial solution, the sum of the 2 strings may still be over the allowed space.
Assignee

Comment 8

Last year
(In reply to Marco Bonardo [::mak] from comment #5)
> So, what about we just make the width localize-able?

Probably not an option 62, so no reason to go ahead with this just now. I'd still like to hear back from Aaron.
Flags: needinfo?(dao+bmo)
Duplicate of this bug: 1474209
fwiw, we could make it localize-able in 63, and just make it larger in 62 as a workaround.

Comment 11

11 months ago
What about changing the label to read: "Remove" instead of "Remove Bookmark"?
Flags: needinfo?(abenson)
Assignee

Comment 12

11 months ago
(In reply to Aaron Benson from comment #11)
> What about changing the label to read: "Remove" instead of "Remove Bookmark"?

That would help but we might still need a fallback solution for locales with particularly long strings. Marco, what do you think?
Flags: needinfo?(mak77)
I'm not sure, I fear some user may confuse Remove with Cancel, and think it will just "remove" the panel. At that point it would be dataloss. And I know a few users who'd make that mistake...
Flags: needinfo?(mak77)
I still think our best solution here is to make the width localize-able in Nightly.
For Beta we can just pick the current translations and set a width that covers the worst case.

Comment 15

11 months ago
Makes sense. Sounds good to me!
Comment hidden (mozreview-request)

Comment 17

11 months ago
mozreview-review
Comment on attachment 8992928 [details]
Bug 1470394 - Make bookmark panel width localizable.

https://reviewboard.mozilla.org/r/257756/#review264984
Attachment #8992928 - Flags: review?(mak77) → review+

Comment 18

11 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/050d2e0e3b62
Make bookmark panel width localizable. r=mak

Comment 19

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/050d2e0e3b62
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Assignee

Updated

11 months ago
Depends on: 1477246
Assignee

Updated

11 months ago
Depends on: 1477250
Assignee

Comment 20

11 months ago
I picked 30em somewhat arbitrarily. We may have to tweak this again if it's not enough for some locales.
Attachment #8993673 - Flags: review?(mak77)
We can probably ask francesco to do a little research for us for the pair of longer strings
Flags: needinfo?(francesco.lodolo)
Longest seems to be Sorbian (the "Remove bookmark" string is plural, not sure when it shows a number".

"X cytańskej znamjeni wótpóraś" + "Dokóńcony"

Of the tier 1 languages, French

"Supprimer les marque-pages (X)" + "Terminer"

German is relatively short in comparison

"X Lesezeichen entfernen" + "Fertig"
Flags: needinfo?(francesco.lodolo)
Dao, can you please update (or just verify) the patch to cover these 38chars strings?
Flags: needinfo?(dao+bmo)
Assignee

Comment 24

11 months ago
(In reply to Marco Bonardo [::mak] from comment #23)
> Dao, can you please update (or just verify) the patch to cover these 38chars
> strings?

30em works with the Sorbian string.
Flags: needinfo?(dao+bmo)
Comment on attachment 8993673 [details] [diff] [review]
patch for uplift

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

thanks
Attachment #8993673 - Flags: review?(mak77) → review+
Assignee

Comment 26

11 months ago
Comment on attachment 8993673 [details] [diff] [review]
patch for uplift

Approval Request Comment
[Feature/Bug causing the regression]: bug 1459877 / bug 1462166
[User impact if declined]: buttons overflow in some locales
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: basically just increasing the panel width
[String changes made/needed]: /
Attachment #8993673 - Flags: approval-mozilla-beta?
Comment on attachment 8993673 [details] [diff] [review]
patch for uplift

CSS change, low risk, Beta62+
Attachment #8993673 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

11 months ago
Flags: qe-verify+
I’ve reproduced this issue on Firefox 62.0a1 (2018-06-22) under Ubuntu 16.04 x64 (de and fr locale). 
I verified the fix on Firefox 63.0a1 (2018-07-26) and Firefox 62.0b12 (20180726161819) under Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12, using different locales (ar, de, es-ES, fr, he, hsb, ia, it, ja, ko, ru, sr, th, uz, vi, xh, zh-CN, zh-TW).
Things look good on the latest Beta, I encountered no overflowing problems, but on the latest Nightly instead, the issue is still reproducible for ja and ia locale (across all platforms I’ve tested) and fr locale (only on Mac platform). See screenshot for ja locale on Windows 10 (Nightly build): https://drive.google.com/open?id=1p9N3-s5dKHiSNsGE0iOR1n-6F4HGwm6c
For now I won’t make any changes on the bug flags, since I don’t know what will be the final approach regarding the panel width differences between Beta and Nightly.
Flags: needinfo?(dao+bmo)
(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #29)
> Things look good on the latest Beta, I encountered no overflowing problems,
> but on the latest Nightly instead, the issue is still reproducible for ja
> and ia locale (across all platforms I’ve tested) and fr locale (only on Mac
> platform). See screenshot for ja locale on Windows 10 (Nightly build):
> https://drive.google.com/open?id=1p9N3-s5dKHiSNsGE0iOR1n-6F4HGwm6c
> For now I won’t make any changes on the bug flags, since I don’t know what
> will be the final approach regarding the panel width differences between
> Beta and Nightly.

Locales don't have that string yet, I'm going to push it out before the end of the day. It will take a few days to see changes.
Flags: needinfo?(dao+bmo)
(In reply to Francesco Lodolo [:flod] from comment #30)
> Locales don't have that string yet, I'm going to push it out before the end
> of the day. It will take a few days to see changes.

Correction: it's available
https://transvision.mozfr.org/string/?entity=browser/chrome/browser/browser.dtd:editBookmark.panel.width&repo=gecko_strings

I see that French is already localized to 26em. Japanese hasn't localized it, which means it falls back to 23em. ia needs to be fixed when the localizer (hopefully) notices the issue.
To clarify for QA, we took different paths in Beta and Nightly.
Beta has a fixed width that should fit all the locales, though the dialog will be the same size for any locale.
In Nightly it is responsibility of the localizer to set an appropriate width for the dialog to fit their string.
Thank you for your prompt replies!

Based on your clarifications, I will mark the bug as verified on Beta 62. 
I will also keep track on the following localization changes, that are to be made on Nightly.
Backed out changeset db7a4be05d8f (Bug 1470394) for failing browser-chrome browser/base/content/test/general/browser_bookmark_popup.js on windows debug a=backout

https://hg.mozilla.org/releases/mozilla-beta/rev/d96403683b5b001d08e88efab5a02dc044ce7eef

Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=190318345&repo=mozilla-beta&lineNumber=2518

17:37:34     INFO -  107 INFO TEST-PASS | browser/base/content/test/general/browser_bookmark_popup.js | Panel should still be 'open' for non-autoclose - "open" == "open" -
17:37:34     INFO -  108 INFO Waiting for mouseout event
17:37:34     INFO -  109 INFO Got mouseout event, should autoclose now
17:37:34     INFO -  Buffered messages finished
17:37:34    ERROR -  110 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bookmark_popup.js | Test timed out -
17:37:34     INFO -  GECKO(4584) | MEMORY STAT | vsize 2098990MB | vsizeMaxContiguous 128967506MB | residentFast 278MB | heapAllocated 82MB
17:37:34     INFO -  111 INFO TEST-OK | browser/base/content/test/general/browser_bookmark_popup.js | took 90083ms
17:37:34     INFO -  Not taking screenshot here: see the one that was previously logged
17:37:34    ERROR -  112 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bookmark_popup.js | Found a tab after previous test timed out: data:text/html,<html><body></body></html> -
Status: RESOLVED → REOPENED
Flags: needinfo?(dao+bmo)
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 11 months ago11 months ago
Resolution: --- → FIXED

Comment 35

11 months ago
Up until the backout on beta, the android hardware tests on Pixel2 devices was green. After the backout, there appears to be intermittent orange. I retriggered the test 12 times and got:

Ten passes.

One failure with ABORT: file /builds/worker/workspace/build/src/dom/media/MediaShutdownManager.cpp, line 83 at org.mozilla.gecko.mozglue.GeckoLoader.abort(GeckoLoader.java:450)

One failure:  TEST-UNEXPECTED-FAIL | dom/media/test/test_videoPlaybackQuality_totalFrames.html | gizmo.mp4 totalFrames should match! - got 160, expected 166

One failure: TEST-UNEXPECTED-FAIL | dom/media/test/test_videoPlaybackQuality_totalFrames.html | Test timed out!

These tests are Tier3. I don't know if this is significant or not, but these tests were totally green before the backout.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&filter-tier=1&filter-tier=2&filter-tier=3&filter-searchStr=android-hw
Assignee

Updated

10 months ago
Blocks: 1482078
I’ve retested this across platforms (Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13) on Firefox 62.0b17 (ar, es-ES, fr, hsb, ja, de, xh, zh-CN, ko, ru, nn-NO locales). The issue is no longer reproducible. 
Nightly is still broken on some locales, I will keep an eye on any further changes that will occur.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1525567
You need to log in before you can comment on or make changes to this bug.