1470394 - new bookmark dialog: remove bookmark button too small for localization

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Sören Hentzschel]

Attachment: screenshot

As you can see in the attached screenshot the translation of the "remove bookmark" button is longer than the button itself.

Comment 1

[Dão Gottwald [:dao]]

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?

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this so we remember to uplift if y'all come up with a fix.

Comment 5

[Marco Bonardo [:mak]]

So, what about we just make the width localize-able?

Comment 6

[Tim Nguyen :ntim]

> 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.

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[Dão Gottwald [:dao]]

(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.

Comment 10

[Marco Bonardo [:mak]]

fwiw, we could make it localize-able in 63, and just make it larger in 62 as a workaround.

Comment 11

[Aaron Benson]

What about changing the label to read: "Remove" instead of "Remove Bookmark"?

Comment 12

[Dão Gottwald [:dao]]

(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?

Comment 13

[Marco Bonardo [:mak]]

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...

Comment 14

[Marco Bonardo [:mak]]

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

[Aaron Benson]

Makes sense. Sounds good to me!

Comment 16

[Dão Gottwald [:dao]]

Attachment: Bug 1470394 - Make bookmark panel width localizable.

Review commit: https://reviewboard.mozilla.org/r/257756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257756/

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8992928 [details]
Bug 1470394 - Make bookmark panel width localizable.

https://reviewboard.mozilla.org/r/257756/#review264984

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/050d2e0e3b62
Make bookmark panel width localizable. r=mak

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/050d2e0e3b62

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch for uplift

I picked 30em somewhat arbitrarily. We may have to tweak this again if it's not enough for some locales.

Comment 21

[Marco Bonardo [:mak]]

We can probably ask francesco to do a little research for us for the pair of longer strings

Comment 22

[Francesco Lodolo [:flod]]

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"

Comment 23

[Marco Bonardo [:mak]]

Dao, can you please update (or just verify) the patch to cover these 38chars strings?

Comment 24

[Dão Gottwald [:dao]]

(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.

Comment 25

[Marco Bonardo [:mak]]

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

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

thanks

Comment 26

[Dão Gottwald [:dao]]

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]: /

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

CSS change, low risk, Beta62+

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 29

[Anca Soncutean, Desktop QA]

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.

Comment 30

[Francesco Lodolo [:flod]]

(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.

Comment 31

[Francesco Lodolo [:flod]]

(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.

Comment 32

[Marco Bonardo [:mak]]

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.

Comment 33

[Anca Soncutean, Desktop QA]

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.

Comment 34

[Daniel Varga [:dvarga]]

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> -

Comment 35

[Bob Clary [:bc] (inactive)]

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

Comment 36

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-beta/rev/812b11ed03e02e7d5ec9f23c6abcbc46d7859740

Comment 37

[Anca Soncutean, Desktop QA]

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.