Closed Bug 1706650 Opened 3 years ago Closed 2 years ago

The Hamburger Menu has the "Pending Update" icon, but clicking menu once doesn't show update menu; second time works

Categories

(Firefox :: Toolbars and Customization, defect, P3)

defect

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- wontfix
firefox93 --- wontfix
firefox94 --- wontfix
firefox95 --- fixed

People

(Reporter: mgaudet, Assigned: arai)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

I've noticed for the last couple of weeks (unfortunately, this is one of those bugs that happened but below level of conscious note for a while) that when I see the update pending badge on my hamburger menu, clicking it the first time doesn't have the "Update Available" menu entry; clicking the hamburger button again, and it shows up.

The last few days I hadn't seen this, but finally sort of caught it on a screen recording.

Sorry that this bug has sat idle for so long! This UI doesn't usually fall under Application Update's responsibility, so I'm going to try to find a better component for it.

Component: Application Update → Toolbars and Customization
Product: Toolkit → Firefox

Changing severity to S3 because there is a workaround, and this does not block critical functionality.

Severity: -- → S3
Priority: -- → P3

Likely from bug 1695783, that removed label-update-restart attribute and made it lazy for the first open.

https://hg.mozilla.org/mozilla-central/rev/f60666a066fa

-                       data-l10n-id="appmenuitem-update-banner"
-                       data-l10n-attrs="label-update-downloading"
-                       label-update-available="&updateAvailable.panelUI.label;"
-                       label-update-manual="&updateManual.panelUI.label;"
-                       label-update-unsupported="&updateUnsupported.panelUI.label;"
-                       label-update-restart="&updateRestart.panelUI.label2;"
+                       data-l10n-id="appmenuitem-update-banner2"
+                       data-l10n-attrs="label-update-downloading, label-update-available, label-update-manual, label-update-unsupported, label-update-restart"

The existence of the label- attribute is used as condition for whether the notification is known, when showing badge and banner:

https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/browser/components/customizableui/content/panelUI.js#991-995

  _showBannerItem(notification) {
    if (!this._panelBannerItem) {
      this._panelBannerItem = this.mainView.querySelector(".panel-banner-item");
    }
    let label = this._panelBannerItem.getAttribute("label-" + notification.id);
    // Ignore items we don't know about.
    if (!label) {
      return;
    }

so, if the attribute isn't set until opening the menu at first time,
the condition fails when the update banner/badge is shown before opening menu.

We should check data-l10n-attrs attribute as well.

Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Keywords: regression
Regressed by: 1695783
Has Regression Range: --- → yes

The above patch almost works, but the banner is shown with empty label, because of the following issue:

(1) Before showing the menu panel, the panel is inside #appMenu-viewCache, not direcly attached to the document

https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/browser/base/content/appmenu-viewcache.inc.xhtml#5-29

<html:template id="appMenu-viewCache">
...
  <panelview id="appMenu-protonMainView" class="PanelUI-subView"
             descriptionheightworkaround="true"
             lockpanelvertical="true">
    <vbox class="panel-subview-body">
      <vbox id="appMenu-proton-addon-banners"/>
      <toolbarbutton id="appMenu-proton-update-banner" class="panel-banner-item"
                     data-l10n-id="appmenuitem-update-banner3"
                     data-l10n-attrs="label-update-downloading, label-update-available, label-update-manual, label-update-unsupported, label-update-restart"
                     oncommand="PanelUI._onBannerItemSelected(event)"
                     wrap="true"
                     hidden="true"/>

(2) When showing banner, we get the #appMenu-proton-update-banner element and set its "label" attribute to localized string for the update status

https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/browser/components/customizableui/content/panelUI.js#997

  _showBannerItem(notification) {
    if (!this._panelBannerItem) {
      this._panelBannerItem = this.mainView.querySelector(".panel-banner-item");
    }
    let label = this._panelBannerItem.getAttribute("label-" + notification.id);
    // Ignore items we don't know about.
    if (!label) {
      return;
    }
    this._panelBannerItem.setAttribute("notificationid", notification.id);
    this._panelBannerItem.setAttribute("label", label);

(the patch above fixes "label-" + notification.id attribute population)

(3) When showing the menu panel after that, the menu panel is moved to the main document, and localization for the tree is performed, and that removes "label" attribute.

https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/dom/l10n/L10nOverlays.cpp#155

void L10nOverlays::OverlayAttributes(
    const Nullable<Sequence<AttributeNameValue>>& aTranslation,
    Element* aToElement, ErrorResult& aRv) {
...
  uint32_t i = aToElement->GetAttrCount();
  while (i > 0) {
    const nsAttrName* attrName = aToElement->GetAttrNameAt(i - 1);

    if (IsAttrNameLocalizable(attrName->LocalName(), aToElement,
                              &explicitlyAllowed) &&
        (aTranslation.IsNull() ||
         !aTranslation.Value().Contains(attrName,
                                        AttributeNameValueComparator()))) {
      nsAutoString name;
      attrName->LocalName()->ToString(name);
      aToElement->RemoveAttribute(name, aRv);
      if (NS_WARN_IF(aRv.Failed())) {
        return;
      }
    }
    i--;
  }

  if (aTranslation.IsNull()) {
    return;
  }

  for (auto& attribute : aTranslation.Value()) {
    RefPtr<nsAtom> nameAtom = NS_Atomize(attribute.mName);
    if (IsAttrNameLocalizable(nameAtom, aToElement, &explicitlyAllowed)) {
      NS_ConvertUTF8toUTF16 value(attribute.mValue);
      if (!aToElement->AttrValueIs(kNameSpaceID_None, nameAtom, value,
                                   eCaseMatters)) {
        aToElement->SetAttr(nameAtom, value, aRv);
        if (NS_WARN_IF(aRv.Failed())) {
          return;
        }
      }
    }
  }

https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/dom/l10n/L10nOverlays.cpp#88

bool L10nOverlays::IsAttrNameLocalizable(
    const nsAtom* nameAtom, Element* aElement,
    nsTArray<nsString>* aExplicitlyAllowed) {
...
  } else if (nameSpace == kNameSpaceID_XUL) {
    // Is it a globally safe attribute?
    if (nameAtom == nsGkAtoms::accesskey || nameAtom == nsGkAtoms::aria_label ||
        nameAtom == nsGkAtoms::aria_valuetext || nameAtom == nsGkAtoms::label ||
        nameAtom == nsGkAtoms::title || nameAtom == nsGkAtoms::tooltiptext) {
      return true;
    }

possible solutions:

  • (a) Stop removing label attribute in L10nOverlays, maybe with special case
  • (b) Add event listener for panel's shown event and set the label there, maybe by storing the information in separate attribute
  • (c) Remove data-l10n-* attribute of #appMenu-proton-update-banner after calling document.l10n.translateFragment on it, so that no more localization happens when showing the menu popup at the first time

:zbraniecki, can I have your opinion, especially around why we remove "label" attribute in L10nOverlays, and what would be the best solution/workaround for it?

Flags: needinfo?(zbraniecki)

(a) Stop removing label attribute in L10nOverlays, maybe with special case

I'm against this solution. We need to be very careful about the API design to minimize the number of edge cases, and this increases them. If the fragment is "handled" by DOM L10n, then it should be very clear that the DOM L10n will do whatever it has to to localize it and no expectations of "exceptions" should be made. Especially in cases like label which is definitely a localizable DOM attribute.

(b) Add event listener for panel's shown event and set the label there, maybe by storing the information in separate attribute

That seems very dirty-hacky, but would likely work :)

(c) Remove data-l10n-* attribute of #appMenu-proton-update-banner after calling document.l10n.translateFragment on it, so that no more localization happens when showing the menu popup at the first time

That would again brake the API contract which allows for DOM l10n annotations to be used to retranslate document at any point during document's lifecycle in result of l10n resources update, or language change.

====

My main question is, why are we placing the code in a state where we ask the DOM to localize the element (setting l10n-id) but also are asking it not to localize the same element.

If the answer is "because we want DOM L10n to localize the attributes such as label-update-available, label-update-pending etc. but not label" then maybe we're designing it wrong?

What if instead we did this:

<div>
   <toolbarbutton class="foo"/>
</div>
let elem = document.querySelector(".foo");
let l10nId = determineCurrentState() == "pending" ? "label-update-pending" : "label-update-available";
document.l10n.setAttributes(elem, l10nId);

This way our code would react to state changes of the system by setting the correct DOM L10n annotation allowing the l10n system to maintain and propagate its state properly.

And as a bonus we'd get rid of the extranous attributes.

Is there a reason we can't do it?

Flags: needinfo?(zbraniecki) → needinfo?(arai.unmht)

Thank you!

Yeah, I realized that we're abusing the API, and setting id for each state sounds clearer.
I'll update the patch with that solution.

Flags: needinfo?(arai.unmht)
Attachment #9247280 - Attachment description: WIP: Bug 1706650 - Populate localization attributes before using it as source of supported labels in update banner. → Bug 1706650 - Split localization item for update banner label for each notification. r?zbraniecki!
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/autoland/rev/19aae26d756d
Split localization item for update banner label for each notification. r=zbraniecki,fluent-reviewers,Gijs
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
You need to log in before you can comment on or make changes to this bug.