1564077 - “Clear Cookies and Site Data…” button is cut out in French

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Théo Chevalier]

Attachment: Screenshot

When opening the page info window in a French build (starting from 69.0), the clear cookies and site data button is cut out at the bottom, the label is really close to the window border.

The button is correctly rendered if you open one of the sub panels (blocked cookies, or connection security details for instance) then go back to the main panel.

Comment 1

[Johann Hofmann [:johannh]]

Is this reproducible in 100% of cases? If yes, could you try to get a regression window with mozregression?

In any case this popup will go through a major change in the 70 timeline so I'm not sure we'll fix this for 69.

Comment 2

[Théo Chevalier]

Yes, I can reproduce on Nightly with a fresh profile — with default settings, just by opening the popup at first start.

However, mozregression doesn’t work with localized builds :/

Comment 3

[Théo Chevalier]

Looks like this has been fixed on Nightly over the last 5 days, I can’t reproduce anymore (even with a fresh profile). Not closing the bug yet as there might be a patch to uplift, but it’s not my call

Comment 4

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

The priority flag is not set for this bug.
:johannh, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]:
Ugly and potentially non-functional privacy UI in popular locales.

Ok, I had some time to look into this (took quite a while, it turns out trying to find the regressor was the best strategy here).

I have to say I think this is a pretty bad regression that will affect at the very least all German and French releases from 69 on. We should probably track this.

I found that bug 1519577 seems to have regressed this by making this change: https://hg.mozilla.org/releases/mozilla-beta/diff/2999bbc0422b544bbeab7097d3ca8349d606a2ea/browser/components/customizableui/PanelMultiView.jsm

Which makes sense because apparently we fail to calculate the description height inside of a toolbarbutton.

Andrew, can you please help me figure out what we can do here? We have toolbarbutton elements in the identity (now protections) popup that got their descriptionHeightWorkaround broken by your change. I'm not really sure why this change was made so I'm somewhat hesitant to touch it myself. Again, I would really like to get this into 69 so it would be great if we could figure it out quickly.

Thanks!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

This doesn't sound like a bug we should ship 69 with. Calling this a blocker.

Comment 10

[Andrew Swan [:aswan]]

Okay, the change mentioned in comment 5 was added since the <toolbarbutton> custom element now injects DOM content directly (as opposed to the anonymous content previously used by the XBL binding). Of course, to mimic XBL's behavior, it only does this if the <toolbarbutton> doesn't have its own inner content (https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/toolkit/content/widgets/toolbarbutton.js#121-125)

This affects the appmenu since it has descriptionheighworkaround=true and it has a bunch of toolbarbuttons that got new <label> children that descriptionHeighWorkaround() now wants to call getBoundingClientRect() on. This fails browser_appmenu.js due to a bunch of unexpected synchronous reflows.

Back to this bug, the issue is the toolbarbuttons here (they've apparently moved recently so they're in a different place in 69 but the issue remains the same):
https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/browser/components/controlcenter/content/protectionsPanel.inc.xul#50

I'm not really sure why those elements are using <toolbarbutton> in the first place? But I suspect rewriting them would be too big/risky to uplift to 69 at this point.

Anyway, it looks like if we set wrap="true" on the toolbarbuttons, then descriptionHeightWorkaround would run on the buttons themselves (but not on the inner <label>s. Would that solve this problem? (its a little hard to tell, but the wrap attribute is mostly used to affect the visibility of the content created by the element/binding. These buttons don't use that content so I don't think it would break anything else).

If that isn't doable, then I guess we really just want descriptionHeightWorkaround to be able to tell these toolbarbuttons apart from "regular" ones. Since these buttons don't use the toolbarbutton-* classes, we could explicitly check for those but that feels awfully hacky. Another option would be to add another attribute to these buttons (or to the actual labels) to tell descriptionHeightWorkaround "no, really look at these even if they are inside a <toolbarbutton>".

Leaving the ni to myself while I try the wrap idea from above.

Comment 11

[Andrew Swan [:aswan]]

Attachment: Screen Shot 2019-07-30 at 1.45.32 PM.png

I couldn't reproduce this in French but I could in German. I'll upload a patch momentarily, here's what the panel looks like with the patch applied.

Comment 12

[Andrew Swan [:aswan]]

Attachment: Bug 1564077 Fix identity panel height issue

Comment 13

[Johann Hofmann [:johannh]]

Thanks for looking into it!

(In reply to Andrew Swan [:aswan] from comment #10)

I'm not really sure why those elements are using <toolbarbutton> in the first place? But I suspect rewriting them would be too big/risky to uplift to 69 at this point.

I think it was because it gave us a bit of styling for free, probably not absolutely necessary but I agree that rewriting isn't something we should do for 69, especially with the big differences between Nightly and Beta.

Anyway, it looks like if we set wrap="true" on the toolbarbuttons, then descriptionHeightWorkaround would run on the buttons themselves (but not on the inner <label>s. Would that solve this problem? (its a little hard to tell, but the wrap attribute is mostly used to affect the visibility of the content created by the element/binding. These buttons don't use that content so I don't think it would break anything else).

I don't think I'm 100% clear on the implications of using wrap but those buttons but I've tried out your patch and it seems to fix the issue without any obvious issues, so I'd be in favor of just doing that.

If that isn't doable, then I guess we really just want descriptionHeightWorkaround to be able to tell these toolbarbuttons apart from "regular" ones. Since these buttons don't use the toolbarbutton-* classes, we could explicitly check for those but that feels awfully hacky. Another option would be to add another attribute to these buttons (or to the actual labels) to tell descriptionHeightWorkaround "no, really look at these even if they are inside a <toolbarbutton>".

Yeah, it's also a solution, though again, using wrap seems to work fine.

Comment 14

[Johann Hofmann [:johannh]]

Assigning Andrew since his patch seems to be a good way forward...

Comment 15

[Andrew Swan [:aswan]]

Attachment: Bug 1564077 Fix protection panel height issue

Comment 16

[Andrew Swan [:aswan]]

(In reply to Johann Hofmann [:johannh] from comment #13)

I don't think I'm 100% clear on the implications of using wrap

The implementation of toolbarbutton is kind of hacky it uses two separate labels:
https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/toolkit/content/widgets/toolbarbutton.js#50-51

and some css to show only one or the other based on the wrap attribute:
https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/toolkit/content/xul.css#125-129

Since the toolbarbuttons in the identity/protection panel don't use the inner content typically created by the CE, none of the builtin css applies here. I think the only other place wrap is referenced is in descriptionHeightWorkaround as described at length in an earlier comment.

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 9081781 [details]
Bug 1564077 Fix identity panel height issue

Beta/Release Uplift Approval Request

• User impact if declined: Identity panel is truncated in some locales
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Well described in the original bug description
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is limited to the identity panel popup.
• String changes made/needed: none

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44cfba422517
Fix protection panel height issue r=johannh

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9081781 [details]
Bug 1564077 Fix identity panel height issue

Fixes an issue causing important UI bits to be cut off. Approved for 69.0b10. Note that this patch is what needs to be landed on Beta, not the patch currently on Autoland.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3e3f6ab6901c

Comment 21

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/44cfba422517

Comment 22

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce this issue using a Firefox 69.0b4 (BuildId:20190712011116) de buid on Windows 10 64bit.

This issue is verified fixed using Firefox 70.0a1 (BuildId:20190801214227) and Firefox 69.0b10 (BuildId:20190731232009) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 18.04 64bit using both fr and de builds.

Comment 23

[Erica Wright [:ewright]]

I believe I'm seeing this issue pop up again on an Arabic build, despite "wrap=true" being set on the toolbarbutton. The trouble is it is not reliable to test, sometimes the toolbarbutton height gets calculated to ~44px, and the bottom is cut off, other times it is correctly calculated to 65.3667px
see: Bug 1594410

Comment 24

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This bug has been identified as part of a pilot on determining root causes of blocking and dot release drivers.

It needs a root-cause set for it. Please see the list at https://docs.google.com/document/d/1FFEGsmoU8T0N8R9kk-MXWptOPtXXXRRIe4vQo3_HgMw/.

Add the root cause as a whiteboard tag in the form [rca - <cause> ] and remove the rca-needed keyword.

If you have questions, please contact :tmaity.