Closed Bug 1605619 Opened 3 years ago Closed 3 years ago

[ja][he] button label glitch in preferences with Nightly73.0a1 localized build

Categories

(Firefox :: Preferences, defect, P1)

73 Branch
Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 74
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)

Attached image screenshot

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

  1. Start Nightly73.0a1 ja build
  2. Open web page e.g. https://www.wikipedia.org/
  3. Open about:preferences#privacy in new tab
  4. 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

Blocks: 1605624

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.

Flags: needinfo?(jhofmann)
Flags: needinfo?(gijskruitbosch+bugs)

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

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(alice0775)

(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

Flags: needinfo?(alice0775)

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

Flags: needinfo?(gijskruitbosch+bugs)

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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(epang)
Blocks: 1526075

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.

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!

Flags: needinfo?(epang)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)

FWIW, I believe the attached patch will also fix bug 1605624 (the dep).

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

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

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

Flags: needinfo?(epang)

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

Priority: -- → P1
Attachment #9118570 - Attachment description: Bug 1605619 - allow reload all tabs button to wrap when necessary, r?MattN,johannh → Bug 1605619 - allow reload all tabs button to wrap when necessary, r?johannh
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d06b9bcc5d27
allow reload all tabs button to wrap when necessary, r=johannh
Duplicate of this bug: 1607807
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74

Please nominate this for Beta approval when you get a chance.

Flags: qe-verify+
Flags: needinfo?(gijskruitbosch+bugs)
Duplicate of this bug: 1605624

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.

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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9118570 - Flags: approval-mozilla-beta?

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.

Attachment #9118570 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified - Fixed in latest Beta JA build 73.0b3 (Build id: 20200110003145) using Windows 10.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.