Closed Bug 1673233 Opened 1 year ago Closed 1 year ago

With 4 tabs and long tab names on about:certificate, the first tab text can overflow off-view

Categories

(Firefox :: Security, defect, P3)

defect

Tracking

()

RESOLVED FIXED
84 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox82 --- unaffected
firefox83 --- fixed
firefox84 --- fixed

People

(Reporter: itiel_yn8, Assigned: itiel_yn8)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image Screenshot

STR:

  1. Go to https://english.president.gov.tw/
  2. Open the site's certification
  3. Click on the second, third or fourth tab

AR:
The first tab text gets overflowed off-view

ER:
Text in all tabs should be visible

Although I understand why this was marked as a regression, this can presumably happen to any certificate with a sufficient number of items in the chain, where at least one of the labels cannot be wrapped or elided, which isn't something that was introduced in bug 1659293.

I also actually can't reproduce on en-US nightly. The "(unknown)" string gets elided into "(unkn..." , which makes sense because the tab label has overflow: hidden and text-overflow: ellipsis. Is this effect locale-dependent?

(All that said, I am surprised it says "(unknown)"... which is a separate issue with that patch, I think...)

Flags: needinfo?(itiel_yn8)
No longer regressed by: 1659293
See Also: → 1659293

(In reply to :Gijs (he/him) from comment #1)

(All that said, I am surprised it says "(unknown)"... which is a separate issue with that patch, I think...)

Put up a patch for this in bug 1673325.

(In reply to :Gijs (he/him) from comment #1)

I also actually can't reproduce on en-US nightly. The "(unknown)" string gets elided into "(unkn..." , which makes sense because the tab label has overflow: hidden and text-overflow: ellipsis. Is this effect locale-dependent?

Sigh. This somewhat seems to be my fault, as I have added unicode-bidi: plaintext in bug 1671373 to make the tabs text auto directional, but it seems it doesn't play nicely if the element inherits direction: rtl :(

This is my plan for a fix, let me know if this sounds good for you:

  1. Revert the logic for dir-auto added in bug 1671373
  2. Force LTR for the tabs instead of the line here: https://searchfox.org/mozilla-central/rev/e1d1f043957191616721b9e8bf811c0aab8a203a/toolkit/components/certviewer/content/components/certificate-tabs-section.js#56
    2a. How would I target the "unknown" tab so it'd not get LTR'd?
  3. To not regress 1671373 wrt the border-radius, target the element according to its parent direction and set the radius with no logical properties
Flags: needinfo?(itiel_yn8) → needinfo?(gijskruitbosch+bugs)

(In reply to Itiel from comment #3)

(In reply to :Gijs (he/him) from comment #1)

I also actually can't reproduce on en-US nightly. The "(unknown)" string gets elided into "(unkn..." , which makes sense because the tab label has overflow: hidden and text-overflow: ellipsis. Is this effect locale-dependent?

Sigh. This somewhat seems to be my fault, as I have added unicode-bidi: plaintext in bug 1671373 to make the tabs text auto directional, but it seems it doesn't play nicely if the element inherits direction: rtl :(

This is my plan for a fix, let me know if this sounds good for you:

  1. Revert the logic for dir-auto added in bug 1671373
  2. Force LTR for the tabs instead of the line here: https://searchfox.org/mozilla-central/rev/e1d1f043957191616721b9e8bf811c0aab8a203a/toolkit/components/certviewer/content/components/certificate-tabs-section.js#56
    2a. How would I target the "unknown" tab so it'd not get LTR'd?

You can probably use an attribute selector for [data-l10n-id] to distinguish that case ?

  1. To not regress 1671373 wrt the border-radius, target the element according to its parent direction and set the radius with no logical properties

Hm, can we not just set the radius on the parent?

But yeah, this sounds reasonable. Thank you!

Assignee: nobody → itiel_yn8
Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :Gijs (he/him) from comment #4)

2a. How would I target the "unknown" tab so it'd not get LTR'd?

You can probably use an attribute selector for [data-l10n-id] to distinguish that case ?

I thought about that, but the ID may change over time for various reasons...
I think I'll just set dir=ltr in here: https://searchfox.org/mozilla-central/rev/e1d1f043957191616721b9e8bf811c0aab8a203a/toolkit/components/certviewer/content/components/certificate-tabs-section.js#51 if !tabName.

  1. To not regress 1671373 wrt the border-radius, target the element according to its parent direction and set the radius with no logical properties

Hm, can we not just set the radius on the parent?

I'll see if this works out.
Thanks!

See Also: → 1673367
Flags: needinfo?(itiel_yn8)

(In reply to Itiel from comment #5)

  1. To not regress 1671373 wrt the border-radius, target the element according to its parent direction and set the radius with no logical properties

Hm, can we not just set the radius on the parent?

I'll see if this works out.

It didn't :-(

Status: NEW → ASSIGNED
Flags: needinfo?(itiel_yn8)
Severity: -- → S3
Priority: -- → P3
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56697cdf8cf3
Fix long text overflowing without ellipsis on the about:certificate tabs in RTL r=johannh
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch

The patch landed in nightly and beta is affected.
:itiel_yn8, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(itiel_yn8)

Comment on attachment 9183798 [details]
Bug 1673233 - Fix long text overflowing without ellipsis on the about:certificate tabs in RTL r?gijs,johannh

Beta/Release Uplift Approval Request

  • User impact if declined: For RTL users, long about:certificate tab names can overflow off-view
  • 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:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Simple patch, mostly minor CSS changes
  • String changes made/needed: None
Flags: needinfo?(itiel_yn8)
Attachment #9183798 - Flags: approval-mozilla-beta?

Comment on attachment 9183798 [details]
Bug 1673233 - Fix long text overflowing without ellipsis on the about:certificate tabs in RTL r?gijs,johannh

83 visual regression for RTL users and the patch is small and CSS, uplift approved for 83 beta 8, thanks.

Attachment #9183798 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.