[ja][he] button label glitch in preferences with Nightly73.0a1 localized build
Categories
(Firefox :: Settings UI, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox71 | --- | unaffected |
firefox72 | --- | unaffected |
firefox73 | + | verified |
firefox74 | + | verified |
People
(Reporter: alice0775, Assigned: Gijs)
References
(Regression)
Details
(Keywords: nightly-community, regression)
Attachments
(2 files)
62.97 KB,
image/png
|
Details | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
[Tracking Requested - why for this release]: Options UI glitch in some localized build[ja][he]
At least, [ja] and [he] build are affected. This problem is reproducible with OS default text size.
Reproducible: always
Steps To Reproduce:
- Start Nightly73.0a1 ja build
- Open web page e.g. https://www.wikipedia.org/
- Open about:preferences#privacy in new tab
- Switch tracking protection modes
Standard -> Strict > Custom -> Strict -> Standard
(標準 -> 厳格 -> カスタム -> 厳格 -> 標準)
Actual results:
button label glitch. See attached screenshot
Expected results:
Button label should not overflowed.
Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8855bff16ed6b9a414fc4d9c387db9ac8a505a2a&tochange=9b7cd94eaf0a2385f53791c982e2210bd6b96818
Suspect: 8bd0989df6b285db776212f8798ba571b94c6db0 Tim Nguyen — Bug 1599783 - Fix shifting of content blocking radiogroup when switching options. r=Gijs
Comment 1•4 years ago
|
||
For reference, in Firefox 71, the whole preferences container seems to get wider when the element shows, so this was also pretty broken before bug 1599783. I guess the ideal UI here would be moving the button the next line?
I don't personally have time to take a close look into this before the end of January. Gijs/Johann, do you or someone from your team have time to look into this ?
FWIW, a way to quickly test this is installing this language pack: https://addons.mozilla.org/en-US/firefox/addon/japanese-language-pack-1/ , and then switch the language in the "Languages" section in the general preferences.
Assignee | ||
Comment 2•4 years ago
|
||
The regressing bug was uplifted to 72, but you marked that not affected - you're sure about that? If so, it must be some interaction between the regressing commit and some other commit... just not sure what that would be. :-(
![]() |
Reporter | |
Comment 3•4 years ago
|
||
(In reply to :Gijs (he/him) from comment #2)
The regressing bug was uplifted to 72, but you marked that not affected - you're sure aboonly ut that? If so, it must be some interaction between the regressing commit and some other commit... just not sure what that would be. :-(
The offending patch https://hg.mozilla.org/mozilla-central/rev/8bd0989df6b2 is not landing to beta repository[1] yet. So, I think this is only affected to mozilla-central.
[1]
https://searchfox.org/mozilla-beta/source/browser/components/preferences/in-content/privacy.inc.xhtml#108
https://searchfox.org/mozilla-beta/source/browser/themes/shared/incontentprefs/privacy.css#178
Comment 4•4 years ago
•
|
||
(In reply to :Gijs (he/him) from comment #2)
The regressing bug was uplifted to 72, but you marked that not affected - you're sure about that? If so, it must be some interaction between the regressing commit and some other commit... just not sure what that would be. :-(
In bug 1599783, Firefox 72 got the backout patch, while Firefox 73 received the HTML elements patch, which might explain this.
Assignee | ||
Comment 5•4 years ago
•
|
||
I can reproduce by applying a diff like this to en-US:
-content-blocking-reload-description = You will need to reload your tabs to apply these changes.
+content-blocking-reload-description = You will need to reload your tabs to apply these changes. This should be quite a long string and I don't know why that makes a diff.
content-blocking-reload-tabs-button =
- .label = Reload All Tabs
+ .label = Reload All Tabs Also this button.
.accesskey = R
In other words, just make the button and text strings longer.
The issue seems to be that the <description>
containing the long-form instructional text is set to have a fixed width of 318px (unsure what that number is about?), and the button has a min-width of 150px. Without a flex-shrink: 0
on the button, that shrinks to its min-width from its preferred width in order to accommodate the maximum width of the container, and the text overflows.
A simple change that just keeps the text more readable is to remove some containers and use align-items: center
on the container, and using flex-shrink: 0
on the button. Of course, then the button juts out because now the entire button overflows the flex container.
It feels like it should be possible to strip out all the containers and be left with:
<image class="content-blocking-info-image"/>
<html:span data-l10n-id="content-blocking-reload-description"></html:span>
<button class="accessory-button reload-tabs-button"
is="highlightable-button"
data-l10n-id="content-blocking-reload-tabs-button"/>
and then flex-align: center
the button, flex-align: start
the image, and have the flex container take the height of the <span>
. However, that doesn't actually work - the message ends up overflowing at the bottom. I don't know why the container height of the html:div
doesn't adapt for the size of the span after it gets wrapped (because that's what happens in a simpel HTML testcase), but it doesn't. I'd needinfo Dan but he's out.
Anyway, even if we do all that, the result is pretty ugly because the button is quite wide and so the text ends up having to wrap in a very narrow space in order to fit next to the button. The simplest solution here would be redesigning this so the message is above the button (perhaps centering the button underneath it). Eric, would that be acceptable?
Comment 6•4 years ago
•
|
||
FWIW, along with your suggestion, you could use flex-wrap: wrap
to put the button on a separate line only when there isn't enough space in the message bar.
Comment 7•4 years ago
|
||
Anyway, even if we do all that, the result is pretty ugly because the button is quite wide and so the text ends up having to wrap in a very narrow space in order to fit next to the button. The simplest solution here would be redesigning this so the message is above the button (perhaps centering the button underneath it). Eric, would that be acceptable?
Yes, works works for me.
FWIW, along with your suggestion, you could use flex-wrap: wrap to put the button on a separate line only when there isn't enough space in the message bar.
I agree with Tim that this should only happen when there isn't enough space in the mesage bar. When ready can you flag me for ui-review? thanks!
Updated•4 years ago
|
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 9•4 years ago
|
||
FWIW, I believe the attached patch will also fix bug 1605624 (the dep).
Comment hidden (obsolete) |
Assignee | ||
Comment 11•4 years ago
•
|
||
Eric, updated try builds will appear at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec67bccadd0c20e429d070ce4bc0320d69db3f2b (now also mac) . It should be possible to use the devtools inspector to change the labels to see what happens if they're bigger. :-)
![]() |
Reporter | |
Comment 12•4 years ago
|
||
(In reply to :Gijs (he/him) from comment #11)
Eric, updated try builds will appear at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec67bccadd0c20e429d070ce4bc0320d69db3f2b (now also mac) . It should be possible to use the devtools inspector to change the labels to see what happens if they're bigger. :-)
FYI,
The trybuild did not fix bug 1605624. The jump can be reproduced even with en-US build of try and default text size of Windows 10.
Comment 13•4 years ago
|
||
(In reply to Alice0775 White from comment #12)
(In reply to :Gijs (he/him) from comment #11)
Eric, updated try builds will appear at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec67bccadd0c20e429d070ce4bc0320d69db3f2b (now also mac) . It should be possible to use the devtools inspector to change the labels to see what happens if they're bigger. :-)
FYI,
The trybuild did not fix bug 1605624. The jump can be reproduced even with en-US build of try and default text size of Windows 10.
This looks good to me, thanks Gijs! When testing on mac en-US I couldn't reproduce bug 1605624.
Assignee | ||
Comment 14•4 years ago
|
||
(In reply to Alice0775 White from comment #12)
(In reply to :Gijs (he/him) from comment #11)
Eric, updated try builds will appear at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec67bccadd0c20e429d070ce4bc0320d69db3f2b (now also mac) . It should be possible to use the devtools inspector to change the labels to see what happens if they're bigger. :-)
FYI,
The trybuild did not fix bug 1605624. The jump can be reproduced even with en-US build of try and default text size of Windows 10.
Thanks. We'll have to look into that separately, then. I don't know off-hand what causes it if it isn't this discrepancy in width.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 15•4 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d06b9bcc5d27 allow reload all tabs button to wrap when necessary, r=johannh
Comment 17•4 years ago
|
||
bugherder |
Comment 18•4 years ago
|
||
Please nominate this for Beta approval when you get a chance.
Comment 20•4 years ago
|
||
Reproduced the initial issue using Beta build 73.0b2 (Build id: 20200107212705) with japanese-language-pack.
Verified - Fixed in latest Nightly JA build 74.0a1 (Build id: 20200108215606) using Windows 10.
Assignee | ||
Comment 21•4 years ago
|
||
Comment on attachment 9118570 [details]
Bug 1605619 - allow reload all tabs button to wrap when necessary, r?johannh
Beta/Release Uplift Approval Request
- User impact if declined: Broken wrapping on buttons in about:preferences
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce: See earlier comments
- List of other uplifts needed: n/a
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): CSS/markup-only change; early in the beta cycle ; switching to more standards-based markup
- String changes made/needed: nope
Comment 22•4 years ago
|
||
Comment on attachment 9118570 [details]
Bug 1605619 - allow reload all tabs button to wrap when necessary, r?johannh
Fixes broken wrapping on buttons in about:preferences, especially annoying in some locales. Approved for 73.0b3.
Comment 23•4 years ago
|
||
bugherder uplift |
Comment 24•4 years ago
|
||
Verified - Fixed in latest Beta JA build 73.0b3 (Build id: 20200110003145) using Windows 10.
Updated•4 years ago
|
Description
•