[Photon] Update notification causes scroll bar in hamburger panel menu if the update label's text wraps

VERIFIED FIXED in Firefox 55

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: soeren.hentzschel, Assigned: mikedeboer)

Tracking

(Blocks 1 bug)

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-structure])

Attachments

(3 attachments)

Posted image screenshot
With browser.photon.structure.enabled set to true, if there is a update notification you can't see the full menu. You have to scroll to see all menu items. Please see the attached screenshot.
The update label's text does not wrap for me, but I expect this will always be the case in some locales, so we should fix this.

Sören, can you reproduce on a clean profile?



Paolo, I think you had to deal with this as a hackaround in the other panel sizing bug, because we don't determine the height correctly for <description>s. I'm still confused about the dependency tree there - is bug 1293242 enough to fix this, or do we need the hackaround in bug 1009116 involving sync flushes even if we fix 1293242?
Flags: qe-verify+
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(cadeyrn)
Summary: [Photon] Update notification causes scroll bar in new static hamburger menu → [Photon] Update notification causes scroll bar in hamburger panel menu if the update label's text wraps
Whiteboard: [photon-structure]
(In reply to :Gijs from comment #1)
> is bug 1293242 enough to fix this, or do we need the hackaround in bug 1009116
> involving sync flushes even if we fix 1293242?

Bug 1293242 is expected to fix this, but I still had to keep the workaround as it seems the current patch does not handle all cases. I think we have to test whether the patch in bug 1293242 works here or not. How can I test the new Photon UI?
Flags: needinfo?(paolo.mozmail)
Priority: -- → P2
QA Contact: gwimberly
(In reply to :Paolo Amadini from comment #2)
> (In reply to :Gijs from comment #1)
> > is bug 1293242 enough to fix this, or do we need the hackaround in bug 1009116
> > involving sync flushes even if we fix 1293242?
> 
> Bug 1293242 is expected to fix this, but I still had to keep the workaround
> as it seems the current patch does not handle all cases. I think we have to
> test whether the patch in bug 1293242 works here or not. How can I test the
> new Photon UI?

You can test by flipping the browser.photon.structure.enabled pref. :-)
(In reply to :Gijs from comment #1)
> The update label's text does not wrap for me, but I expect this will always
> be the case in some locales, so we should fix this.
> 
> Sören, can you reproduce on a clean profile?

Yes, it's reproducible with a clean profile.
Flags: needinfo?(cadeyrn)
I will try to figure this out on Monday.
Flags: needinfo?(gijskruitbosch+bugs)
This regressed with bug 1354141 - it worked OK before that landed. Mike, I suspect that the width/height of the elements here either changes mid-transition, or we're doing something wrong with how we measure them. I'm not really sure how best to debug this... Obviously we can just set a fixed width here and that will make the problem go away for English, but I'd rather understand what's going on a bit better because we'll have the same problem - just worse - with other languages.

Trying to check what's going on, it seems like the panel banner item has width/height 0 (when using dwu) when we measure the initial rect here:

https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/browser/components/customizableui/PanelMultiView.jsm#407-409

... even though the item isn't hidden anymore (the attribute is gone)

I tried flushing layout from inside the code where we unhide the item, but that doesn't seem to help - possibly because the panel might still have the hidden attribute set.

In fact, even flushing (Cu.reportError(banner.getBoundingClientRect().width) at the point noted above doesn't seem to help and still returns 0. Maybe I'm misunderstanding how we use the sizes from that rect elsewhere?

Mike, can you think of something else we can do here?
Blocks: 1354141
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mdeboer)
This is fundamentally bug 1293242. When the Photon version of the panel is opened for the first time it has no explicit height, so we get the wrong height due to the layout bug. This is fixed if I apply the descriptionHeightWorkaround to this element in the "popupshown" event:

let bannerText = this.document.getAnonymousElementByAttribute(
  this._mainView.querySelector(".panel-banner-item"), "class",
  "toolbarbutton-multiline-text");

I expect the same issue would affect bug 1009116 too, I'll check to see if there is anything to do there. This might have to be fixed somewhat differently here depending on the performance requirements of the Photon panel.
Depends on: 1293242
(In reply to :Paolo Amadini from comment #7)
> This is fundamentally bug 1293242. When the Photon version of the panel is
> opened for the first time it has no explicit height, so we get the wrong
> height due to the layout bug. This is fixed if I apply the
> descriptionHeightWorkaround to this element in the "popupshown" event:

But then we get a jump in the panel's height after it appears, right? That doesn't seem OK...

Fundamentally, I don't understand why the element has no height/width at all at the point where the popup starts being shown, even if we force a layout flush.

> let bannerText = this.document.getAnonymousElementByAttribute(
>   this._mainView.querySelector(".panel-banner-item"), "class",
>   "toolbarbutton-multiline-text");

I assume you meant to copy/paste more code here?
(In reply to :Gijs from comment #8)
> Fundamentally, I don't understand why the element has no height/width at all
> at the point where the popup starts being shown, even if we force a layout
> flush.

Which point are you referring to exactly? The code in comment 6 doesn't seem to be invoked at all, as playTransition is false.

> > let bannerText = this.document.getAnonymousElementByAttribute(
> >   this._mainView.querySelector(".panel-banner-item"), "class",
> >   "toolbarbutton-multiline-text");
> 
> I assume you meant to copy/paste more code here?

No, I just wanted to say that I basically added this element to the "let element of" loop in decriptionHeightWorkaround.
(In reply to :Paolo Amadini from comment #9)
> (In reply to :Gijs from comment #8)
> > Fundamentally, I don't understand why the element has no height/width at all
> > at the point where the popup starts being shown, even if we force a layout
> > flush.
> 
> Which point are you referring to exactly? The code in comment 6 doesn't seem
> to be invoked at all, as playTransition is false.

But this.panelViews is truthy - I'm pretty sure it runs, as I had logging output from there.
For me, this code does run later, when closing the panel...

Anyways, I've tested the non-Photon view with both the old and new architecture and the issue doesn't exist. This may be because of the flexbox layout we're using there, or other peculiarities of how we arranged the elements.

So, in this case a possible workaround would be to rearrange the elements to avoid the layout bug. Just moving the element outside of the scrollable area isn't enough. Setting a max-width on the button seems to work, but then the button won't expand horizontally if the width of the main view changes.
Duplicate of this bug: 1367346
For ease of reproduction: you can trigger the badge + banner by running this in the browser console:

AppMenuNotifications.showNotification("update-restart", {callback() {}}, null, {dismissed: true})

(this changed 2 hours ago because of bug 1359733, so on older builds you will need to use: gMenuButtonUpdateBadge.showUpdateNotification("restart", true, () => {}) )
I think I've found a solution the same way Paolo suggested in comment 7. I'll wait until bug 1009116 get merged into m-c to apply it to multiline labels as well.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Flags: needinfo?(mdeboer)
Priority: P2 → P1
I'll also try to address the styling b0rks that I noticed in the update notification.
Comment on attachment 8871815 [details]
Bug 1364738 - Fix up multi-line labels inside panelviews that have wrapped around and align elements in the banner.

https://reviewboard.mozilla.org/r/143284/#review147094

I made a first review pass for the descriptionHeightWorkaround function - I haven't reviewed the style changes.

::: browser/components/customizableui/PanelMultiView.jsm:744
(Diff revision 2)
>        case "popupshown":
>          // Now that the main view is visible, we can check the height of the
>          // description elements it contains.
>          this.descriptionHeightWorkaround();
> +
> +        if (!this.panelViews) {

For the record, we'll need to set the height in the Photon version too, but before we display the panel (bug 1367776).

::: browser/components/customizableui/PanelMultiView.jsm:944
(Diff revision 2)
> -    // We batch DOM changes together in order to reduce synchronous layouts.
> -    // First we reset any change we may have made previously. The first time
> -    // this is called, and in the best case scenario, this has no effect.
> +    let buttons = viewNode.querySelectorAll("toolbarbutton[wrap]");
> +    let descriptions = viewNode.getElementsByTagName("description");
> +    let workedAroundBefore = false;

You should probably keep and edit at least some of the comments...

::: browser/components/customizableui/PanelMultiView.jsm:960
(Diff revision 2)
> -    // from a _transitionHeight callback and there are no description elements
> -    // visible, then _transitionHeight will not trigger a layout again.
> -    for (let item of items) {
> -      item.height = item.element.getBoundingClientRect().height;
> -    }
> -    // Now we can make all the necessary DOM changes at once.
> +    // reflow.
> +    if (workedAroundBefore)
> +      viewNode.clientHeight;
> +
> +    let nodes = [...buttons, ...descriptions];
> +    let dwu = this._dwu;

Can you just make _dwu a lazy getter instead? For classes you can use XPCOMUtils.defineLazyGetter in the constructor, or manually call Object.defineProperty(this, "_dwu", ...) in the current getter.

::: browser/components/customizableui/PanelMultiView.jsm:966
(Diff revision 2)
> +      if (node.labelElement) {
> +        node = node.labelElement;
> +        bounds = dwu.getBoundsWithoutFlushing(node);
>        }

It seems we can just map the labelElement of each button when we get the list of elements at the beginning, so you can remove this condition. Setting the height on the outer element based on the labelElement might not take extra padding and margins into account.

Also, you need to reset the height of the labelElements as well, just like the other descriptions, because in the generic case we have to assume that the text contents or the width may have changed at any time. (And I think this is the case for the banner, that may display different labels at different times. If you don't do this the height will be incorrect if the label changes so it doesn't wrap anymore.)

If you want to optimize this further, so we don't have sync reflows if not strictly necessary, what you can do is to store the old text contents and the width in a Map, and exclude the element if they haven't changed since last time. This, assuming text size and margins won't change.

::: browser/components/customizableui/content/panelUI.inc.xul:500
(Diff revision 2)
>         type="arrow"
>         hidden="true"
>         flip="slide"
>         position="bottomcenter topright"
>         noautofocus="true">
> -  <photonpanelmultiview id="appMenu-multiView" mainViewId="appMenu-mainView">
> +  <photonpanelmultiview id="appMenu-multiView" mainViewId="appMenu-mainView" descriptionheightworkaround="true">

I was wondering if this should just be the default, but it's good to have something in the XUL that reminds the reader that there is something weird going on, and they need to call the workaround function manually if they make new description elements visible while the panel is open.

Speaking of which, make sure the badge cannot appear while the panel is shown, and if so, call the workaround function manually at that point.
Attachment #8871815 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #18)
> Setting the height on the outer element based on the labelElement might not take
> extra padding and margins into account.

Nevermind this bit, I missed "node = node.labelElement;" when reading it. But the rest is still valid!
Attachment #8871815 - Flags: review?(paolo.mozmail)
Posted image screenshot
FYI: the text in the update notification is different on current Nightly and the reported problem is no longer visible within the update notification. But the text is now: "R… Restart to update Nightly". I don't know which bug changed that so I decided to report it here.
(In reply to :Paolo Amadini from comment #18)
> Speaking of which, make sure the badge cannot appear while the panel is
> shown, and if so, call the workaround function manually at that point.

To be clear, I think you mean the banner in the menu - 'badge' is generally what we call the thing on the toolbar button.

This would probably need to happen in https://dxr.mozilla.org/mozilla-central/rev/bce03a8eac301bcd9408b22333b1a67c3eaed057/browser/components/customizableui/content/panelUI.js#777-790 .
Comment on attachment 8871815 [details]
Bug 1364738 - Fix up multi-line labels inside panelviews that have wrapped around and align elements in the banner.

https://reviewboard.mozilla.org/r/143284/#review147284

The style and mainview changes look OK to me. I defer to Paolo for the review of the descriptionHeightWorkaround stuff.

::: browser/components/customizableui/PanelMultiView.jsm:944
(Diff revision 2)
>      if (!this.node.hasAttribute("descriptionheightworkaround")) {
>        // This view does not require the workaround.
>        return;
>      }
>  
> -    // We batch DOM changes together in order to reduce synchronous layouts.
> +    let buttons = viewNode.querySelectorAll("toolbarbutton[wrap]");

Can we add :not([hidden]) and then bail early if buttons.length == 0 (and descriptions.length == 0) , and not do anything? Because we don't actually need to do anything here for the common case in this panel, it's only when this banner shows up...
Attachment #8871815 - Flags: review?(gijskruitbosch+bugs)
(In reply to Sören Hentzschel from comment #20)
> Created attachment 8872034 [details]
> screenshot
> 
> FYI: the text in the update notification is different on current Nightly and
> the reported problem is no longer visible within the update notification.
> But the text is now: "R… Restart to update Nightly". I don't know which bug
> changed that so I decided to report it here.

Yes, this is being fixed by the patch that Mike has already put up on this bug. It's because the 'mainview' attribute has gone missing since bug 1009116. We might want to land it separately because it's so visible...
(In reply to :Paolo Amadini from comment #18)
> For the record, we'll need to set the height in the Photon version too, but
> before we display the panel (bug 1367776).

How? Using this code, but then in "popupshowing"?

> You should probably keep and edit at least some of the comments...

Sorry, I didn't intend to remove them.

> Can you just make _dwu a lazy getter instead? For classes you can use
> XPCOMUtils.defineLazyGetter in the constructor, or manually call
> Object.defineProperty(this, "_dwu", ...) in the current getter.

I already added that in bug 1364672 ;-)
Flags: needinfo?(paolo.mozmail)
(In reply to Mike de Boer [:mikedeboer] from comment #24)
> (In reply to :Paolo Amadini from comment #18)
> > For the record, we'll need to set the height in the Photon version too, but
> > before we display the panel (bug 1367776).
> 
> How? Using this code, but then in "popupshowing"?

Probably something slightly different, based on the position of the anchor on the screen. I expect it to be similar to what the popup widget code does to calculate the position and direction, but I haven't looked into this yet.
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8871815 [details]
Bug 1364738 - Fix up multi-line labels inside panelviews that have wrapped around and align elements in the banner.

https://reviewboard.mozilla.org/r/143284/#review147618

There's a main issue that should be fixed, I'll continue the review on the next version. I can review the styling changes as well if you want, as I see you haven't asked Gijs for those.

::: browser/components/customizableui/PanelMultiView.jsm:990
(Diff revision 3)
> +    for (let [node, nodeText, bounds] of nodes) {
> +      // Skip invisible elements.
> +      if (!bounds.width || !bounds.height)
> +        continue;
> +
> +      this._descriptionsWidthMap.set(nodeText, bounds.width);

Keying by text content and looking for the same width has actually the issue that once the text swapped between two values, if the element has the same width we'll stop updating the height when the text changes. Also, we're keeping around unneeded keys if the text has variable elements inside it. Just as an example, it could the number of blocked trackers in the Identity Block.

We should probably use the element itself as the key, and for the value have an object or array with both the text and the height. By doing this, we can also use a WeakMap, which should free resources in case views add <description> elements dynamically.
Attachment #8871815 - Flags: review?(paolo.mozmail)
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Comment on attachment 8871815 [details]
Bug 1364738 - Fix up multi-line labels inside panelviews that have wrapped around and align elements in the banner.

https://reviewboard.mozilla.org/r/143284/#review147842

Thanks!

::: browser/components/customizableui/PanelMultiView.jsm:932
(Diff revision 6)
>    }
>  
>    /**
>     * If the main view or a subview contains wrapping elements, the attribute
>     * "descriptionheightworkaround" should be set on the view to force all the
> -   * "description" elements to a fixed height. If the attribute is set and the
> +   * "description" or label elements to a fixed height. If the attribute is set

This should say "wrapping toolbarbutton elements" instead of "label elements", as we don't consider the latter.

::: browser/components/customizableui/PanelMultiView.jsm:973
(Diff revision 6)
>        items.push({ element });
>      }
> +
>      // We now read the computed style to store the height of any element that
>      // may contain wrapping text, which will be zero if the element is hidden.
>      // This might trigger a synchronous layout, but if this function was called

Either two missing lines or one line to remove...

::: browser/components/customizableui/PanelMultiView.jsm:979
(Diff revision 6)
>      for (let item of items) {
> -      item.height = item.element.getBoundingClientRect().height;
> +      item.bounds = item.element.getBoundingClientRect();
>      }
> +
>      // Now we can make all the necessary DOM changes at once.
> -    for (let item of items) {
> +    for (let {element, bounds} of items) {

nit: spaces inside braces
Attachment #8871815 - Flags: review?(paolo.mozmail) → review+
Also, I had missed comment 22 and 23, where Gijs reviewed the rest of the changes. You may want to consider landing the mainview attribute change separately.
Comment on attachment 8871815 [details]
Bug 1364738 - Fix up multi-line labels inside panelviews that have wrapped around and align elements in the banner.

https://reviewboard.mozilla.org/r/143284/#review147862
Attachment #8871815 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db14c0e392d0
Fix up multi-line labels inside panelviews that have wrapped around and align elements in the banner. r=Gijs,Paolo
https://hg.mozilla.org/mozilla-central/rev/db14c0e392d0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1369339
Depends on: 1369971
I can reproduce this issue by setting browser.photon.structure.enabled to true in nightly 55.0a1 (2017-05-14) (64-bit) in 64bit Linux.

I have verfied that this issue is no longer reproducible with yesterday's nightly (2017-06-08) (64-bit) in order to be able to trigger update to today's nightly.

Build ID 	20170608100220
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
QA Whiteboard: [bugday-20170607]
I have successfully reproduced this bug with Nightly 55.0a1 (2017-05-14) (32-bit) on windows 10(32bit)

this bug is verified fix with  Nightly 55.0a1 (2017-06-01) (32-bit)

Build ID: 20170601030206
Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0
As per comment 37 and comment 38 this bug is verified as fixed in both linux and windows. So, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.