Closed Bug 1564077 Opened 4 months ago Closed 3 months ago

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

Categories

(Firefox :: Site Identity, defect, P1, critical)

69 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 blocking verified
firefox70 + verified

People

(Reporter: tchevalier, Assigned: aswan)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files)

Attached image 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.

Component: Page Info Window → Site Identity and Permission Panels

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.

Flags: needinfo?(theo)

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

Flags: needinfo?(theo)

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

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jhofmann)

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

Flags: needinfo?(jhofmann) → needinfo?(aswan)
Keywords: regression
Priority: -- → P1
Regressed by: 1519577
Duplicate of this bug: 1567296
Duplicate of this bug: 1569455
Duplicate of this bug: 1569467
See Also: → 1468355

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

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.

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.

Flags: needinfo?(aswan)

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.

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

Assignee: nobody → aswan
Status: NEW → ASSIGNED

(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 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
Attachment #9081781 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44cfba422517
Fix protection panel height issue r=johannh

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.

Attachment #9081781 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

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.

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