1589015 - [Windows] The bottom of the Privacy Panel is too short when there are element that occupy two rows

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[Oana Botisan, Desktop QA]

Attachment: the bottom of the panel is too short.gif

Affected versions

• Firefox 70.0
• Nightly 71.0a1

Affected platforms

• Windows 10 x64

Steps to reproduce

1. Download a “de” build from https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/
2. Install it and launch the browser with a clean profile.
3. Load https://www.welt.de/ in a new tab.
4. Wait until the site is fully loaded and click on the shield icon.
5. Refresh the page if the issue is not reproducing.

Expected result

• The bottom of the Privacy Panel has the correct dimensions.

Actual result

• The bottom of the Privacy Panel is cut off.

Regression range

• This is not a regression. I can reproduce the issue on Nightly from 2019-09-16 when the Privacy Panel was fully translated.

Additional notes

• If you restart the browser, the dorhanger has the normal proportions for a few seconds and then the bottom of it is cut off again.
• In the attached gif you can see both the actual result (welt.de) and expected behaviour (youtube).

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

This is because of the cookies category item's label being dynamically set. descriptionHeightWorkaround is calculating the height for a string that doesn't wrap, and then assumes that it doesn't need to re-calculate it. The solution is to invalidate the height every time we change the label. Patching coming up.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1589015 - Stop using two label elements in the cookies category item. r=johannh

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

The situation is more complicated than I thought. The reason the height isn't invalidated, is because the toolbarbutton reports its textContent as the concatenation of the two labels it contains, regardless of whether one of them is hidden. This means that the textContent looks constant if we switch between them, so descriptionHeightWorkaround thinks it doesn't need to do anything.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66b01a30ed12
Stop using two label elements in the cookies category item. r=johannh

Comment 6

[Razvan Maries]

Backed out for causing perma fails on: browser_trackingUI_categories.js.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-browser-chrome-e10s-3%2Cm%28bc3%29&revision=66b01a30ed12782ead9f0d2af4703d51c4fd4f48&selectedJob=271846796&group_state=expanded

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=271846796&repo=autoland&lineNumber=4494

Backout: https://hg.mozilla.org/integration/autoland/rev/c3e12bc6ff0a94c7885c943f9085b7b1b7872f16

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1589015 - Update cookies category label tests. r=johannh

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

We confirmed that the patch seems to be effective - but it wasn't ready to land yet because I hadn't run tests.

Try push with updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ecbc121ef53e0da739ca85c830154dacf865aa8

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa914a108591
Stop using two label elements in the cookies category item. r=johannh
https://hg.mozilla.org/integration/autoland/rev/9fbd56a4ec97
Update cookies category label tests. r=johannh

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/fa914a108591
https://hg.mozilla.org/mozilla-central/rev/9fbd56a4ec97

Comment 11

[Pascal Chevrel:pascalc]

Nihanth, this is marked as P1 and 71 affected, is that something we could uplift safely to beta or can we let it ride the trains to 72? Thanks

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 9101849 [details]
Bug 1589015 - Stop using two label elements in the cookies category item. r=johannh

Beta/Release Uplift Approval Request

• User impact if declined: In some cases the protections panel is cropped at the bottom, which looks glitchy.
• 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: Medium
• Why is the change risky/not risky? (and alternatives if risky): I'm marking this medium because it's not just a couple of lines of CSS, but it's not very risky either.
• String changes made/needed:

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Pascal Chevrel:pascalc from comment #11)

Nihanth, this is marked as P1 and 71 affected, is that something we could uplift safely to beta or can we let it ride the trains to 72? Thanks

I think we can uplift, requested above :)

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9101849 [details]
Bug 1589015 - Stop using two label elements in the cookies category item. r=johannh

P1, low risk visual fix that baked on nightly for more than a week, uplift approved for 71 beta 6, thanks.

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/868642f885bb
https://hg.mozilla.org/releases/mozilla-beta/rev/ef43ee07acda

Comment 16

[Oana Botisan, Desktop QA]

I still managed to reproduce the issue on latest Nightly 72.0a1 on Windows 10 x64. After I closed the browser and reopened it, the doorhanger had the expected dimensions. But when I closed it again and reopened it, the issue reproduced. I think it might be intermittent.

Comment 17

[(inactive) Nihanth Subramanya [:nhnt11]]

Oana, if you can still reproduce this, can you please file a bug blocking this one? I'm not sure anyone has cycles to look further into the edge cases here, but we should track the issue if it exists.