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)
Tracking
()
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.
Reporter | ||
Comment 1•3 years ago
|
||
Reporter | ||
Comment 2•3 years ago
|
||
The last few days I hadn't seen this, but finally sort of caught it on a screen recording.
Comment 3•3 years ago
|
||
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.
Comment 4•3 years ago
|
||
Changing severity to S3 because there is a workaround, and this does not block critical functionality.
Assignee | ||
Comment 5•2 years ago
|
||
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:
_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.
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 6•2 years ago
|
||
Assignee | ||
Comment 7•2 years ago
|
||
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
<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
_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.
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;
}
}
}
}
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 inL10nOverlays
, 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 callingdocument.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?
Comment 8•2 years ago
|
||
(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?
Updated•2 years ago
|
Assignee | ||
Comment 9•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 10•2 years ago
|
||
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
Comment 11•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Description
•