1582750 - [Protections Panel] descriptionHeightWorkaround is not applied in time to all necessary elements, resulting in clipping

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Oana Botisan, Desktop QA]

Attachment: Screenshot_2.png

Affected versions

• Firefox 71.a01
• Firefox 70.0b8

Affected platforms

• Windows 10 x64
• Ubuntu 18.04 x64
• macOS 10.14

Steps to reproduce

1. Place the browser on the lower half of the screen.
2. Open any site with a new profile or click on the shield icon from URL bar and then on the "i" symbol.
3. Observe the "Browse without being followed" panel.

Expected result

• The doorhanger is correctly displayed .

Actual result

• Part of the doorhanger is missing.

Regression range

• This is a not regression.

Additional notes

• Please look at the attached image.
• This happens sometimes when you open the browser for the first time. I just logged the issue under this steps, because following these steps the issue is always reproducing.

Comment 1

[Johann Hofmann [:johannh]]

FYI this was not fixed by bug 1581415.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

I managed to reproduce this bug and figure out the issue.

STR:

1. Open a new window (or restart/open Firefox - basically you must use a window in which the protections popup has never been opened)
2. Navigate to reddit.com, and click the shield icon as soon as it's available, before the site finishes loading
3. Wait for the site to finish loading - the panel will go from the "no trackers detected" state to a state where it shows category items. At this point, the content overflows the panel and it doesn't resize, resulting in clipping.

I debugged this by adding a console.log() in descriptionHeightWorkaround() when it actually sets the heights of elements. The first time the panel is opened, all the category section items are omitted, because at the time they are processed, they are invisible.

The solution is to call descriptionHeightWorkaround() manually, the first time we show the category items. Patch is coming up.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1582750 - Manually invoke descriptionHeightWorkaround() on protections panel the first time we show category items. r=johannh

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35ca0dc8f9e7
Manually invoke descriptionHeightWorkaround() on protections panel the first time we show category items. r=johannh

Comment 5

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/35ca0dc8f9e7

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 9097443 [details]
Bug 1582750 - Manually invoke descriptionHeightWorkaround() on protections panel the first time we show category items. r=johannh

Beta/Release Uplift Approval Request

• User impact if declined: Occasional visual glitch - especially on first run of 70 when the onboarding message is shown.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 2
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The patch adds an extra call to an existing function that is already called every time the panel is opened. The function in question implements caching and does nothing if the input data didn't change. The patch is an objective improvement and even if there turns out to be a case that isn't covered, this patch should do no harm.
• String changes made/needed:

Comment 7

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

Comment on attachment 9097443 [details]
Bug 1582750 - Manually invoke descriptionHeightWorkaround() on protections panel the first time we show category items. r=johannh

Polish for Skyline feature, fine for uplift for beta 12.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

This is going to need a further adjustment. There's one edge case it doesn't cover, here's the state flow for it:

1. User begins a page load.
2. User opens protections popup, it says "No trackers detected."
3. Content blocking event received, but only Allowing and None Detected - nothing Blocked.
4. Another content blocking event received, this time all three shown.

We call descriptionHeightWorkaround at step 3, but we also need to call it at step 4 - and we don't.

The fix is to call eliminate our "_descriptionHeightWorkaroundCalled" (or whatever I named it) state variable, and simply call it every time. And I think this is necessary and OK.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1582750 - Ensure descriptionHeightWorkaround is called for multiple consecutive content blocking events. r=johannh

Comment 10

[Oana Pop-Rus]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f9ffb33e44e
Ensure descriptionHeightWorkaround is called for multiple consecutive content blocking events. r=johannh

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/5f9ffb33e44e

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 9097783 [details]
Bug 1582750 - Ensure descriptionHeightWorkaround is called for multiple consecutive content blocking events. r=johannh

Beta/Release Uplift Approval Request

• User impact if declined: See comment 6. Visual polish/quality.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: See comment 8
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small tweak to confirmed fix - covers an edge case.
• String changes made/needed:

Comment 14

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

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

Comment 15

[Oana Botisan, Desktop QA]

We managed to investigate the issue a bit and these are the results:

STR from comment 2:

• using latest Nightly 71.0a1 and Firefox 70.0b12 on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64. The issue is not reproducing anymore.

STR from comment 0:

• I verified the fix using latest Nightly 71.0a1 and Firefox 70.0b12 on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64. The issue is still reproducing.

Should I log a different bug with the steps from comment 0? Maybe an enhancement for that part?

Comment 16

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

Comment on attachment 9097783 [details]
Bug 1582750 - Ensure descriptionHeightWorkaround is called for multiple consecutive content blocking events. r=johannh

Followup fix for skyline feature. OK for beta 13 uplift.

Comment 17

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

Oana, yes, can you file a followup bug?

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b4e46f6a9a6

Comment 19

[Oana Botisan, Desktop QA]

I logged bug 1586652 as a follow up bug.

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

See bug 1586652 comment 1.

Comment 21

[Ciprian Georgiu, Desktop QA]

I cannot reproduce the steps from comment 2 anymore, on latest Nightly 71.0a1 and Beta 70.0b13 across platforms: Windows 10 x64, macOS 10.13, Ubuntu 18.04 x64.

The initial steps reported in the bug (comment 0) are still repro, but since the issue is tracked separately in bug 1586652, I think we can close this one. Marking as verified fixed.