C-C: Change progressmeter to be html:progress

RESOLVED FIXED in Thunderbird 65.0

Status

defect
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: emilio, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 65.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

Seems like since very recently daily's progressbar has started to jump around, I'll send a screencast in a bit.

I'm trying to find a regression range.
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=9ebf403d5a03ecbe6a1714f218a5cdf30de517b2&tochange=22c54a00f50379df40e6351d16f8a3d9ffb6f090

Is from "always-grey" to "jumpy progressbar". But something earlier broke the progressbar from "working" to "always-grey". 

I'm not sure the mozilla-central changes that have happened in between, how do I look at that?
Posted video screencast.ogv
Jorg, do you know how should I go about debugging this?

The commits in those regression ranges (well, maybe except bug 1497101 which can have a few side-effects) don't seem familiar, so I tend to think this may be due to a Gecko-side regression...
Flags: needinfo?(jorgk)
Yes, it is a Gecko-side regression. The progress bar went broken in bug 1491197 and got fixed "a bit" in bug 1496189. But as far as we know, it was never restored fully, see the bugs blocking bug 1496189, like bug 1499116.

I'm not really happy about things breaking and being left broken instead of addressing the regressions quickly :-(
Flags: needinfo?(jorgk)
The range from comment #2 coincides with bug 1491197 and the one from comment #1 with bug 1496189.
This looks like bug 1497837, will confirm once that's in Daily whether this is fixed.
Depends on: 1497837
Sorry for the churn / regressions with progressmeter conversion. It looked like a pretty straightforward conversion originally but the integration with various platform code and relative lack of test coverage made it more regression-prone than other conversions.

FYI - we are planning to switch remaining consumers of xul:progressmeter to html:progress in the Firefox frontend (bug 1428869).  I'd suggest you do the same for TB - you could alternatively register the progressmeter CE/XBL somewhere TB specific but that's probably a shorter term solution.
Taking this bug to convert all the progressmeter in mail and mailnewsto to html:progress.
Component: Untriaged → General
Summary: Jumpy progressbar when fetching email. → C-C: Change progressmeter to be html:progress
Posted patch 1499593-progressmeter.patch (obsolete) — Splinter Review
This should be all progressmeter.

Calendar has two more, but they use such things I don't know how to work with:
https://searchfox.org/comm-central/search?q=DETERMINED_PROGRESS&case=false&regexp=false&path=

Brian, could you check, if this patch seems reasonable?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9018055 - Flags: review?(acelists)
Attachment #9018055 - Flags: feedback?(bgrinstead)
Comment on attachment 9018055 [details] [diff] [review]
1499593-progressmeter.patch

Review of attachment 9018055 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, this looks close to what Paolo's doing in Bug 1428869
Attachment #9018055 - Flags: feedback?(bgrinstead) → feedback+
Posted patch 1499593-progressmeter.patch (obsolete) — Splinter Review
It slipped a unrelated change into the previous patch.
Attachment #9018055 - Attachment is obsolete: true
Attachment #9018055 - Flags: review?(acelists)
Attachment #9018179 - Flags: review?(acelists)
Comment on attachment 9018179 [details] [diff] [review]
1499593-progressmeter.patch

Review of attachment 9018179 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

::: mail/base/content/FilterListDialog.js
@@ +50,5 @@
>        document.getElementById('statusbar-progresspanel').removeAttribute('collapsed');
>        this.progressMeterVisible = true;
>      }
>  
> +    document.getElementById("statusbar-icon").removeAttribute("value");

So what feature do we lose by this conversion? Does missing "value" attribute mean aned behave as "undetermined" mode (progressbar spinning) ?

::: mail/components/activity/content/activity.xml
@@ +439,3 @@
>              } else {
>                let _percentComplete = 100.0 * aWorkUnitsComplete /
>                                       aTotalWorkUnits;

Do fractional/float values work with the new progressbar?

@@ +442,1 @@
>                element.setAttribute("value", _percentComplete);

Is it better to use the attribute or the .value property (as done at some other places)?

@@ +504,5 @@
>            <xul:hbox>
>              <xul:vbox flex="1">
> +              <html:progress value="0" max="100" flex="1"
> +                             anonid="progressmeter"
> +                             xbl:inherits="value=progress,mode=progressmode"/>

Does the xbl:inherit do anything on html:* elements? I don't know but this needs to be checked or asked.

::: mail/components/migration/content/migration.xul
@@ +79,5 @@
>      <vbox id="migratingItems" class="indent" style="overflow: auto;" flex="1" align="left"/>
>      <separator class="thin"/>
>  
>      <hbox>
> +      <html:progress class="progressmeter-statusbar" id="progressBar"  flex="1" value="0" max="100"/>

Double space before flex.

::: mailnews/base/content/shutdownWindow.xul
@@ +21,5 @@
>      <label id="shutdownStatus_label" value="" />
>      <separator class="thin" />
>    </vbox>
>  
> +  <html:progress id="shutdown_progressmeter" />

May it need a 'max' attribute?

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ -1885,5 @@
>      }
>  
> -    if (aID == "progressMeter") {
> -      el.setAttribute("mode", aValue == "?" ? "undetermined" : "determined");
> -    }

Why can this (and the callers) be dropped? Don't you need to remove the 'value' attribute when "?"?
(In reply to :aceman from comment #13)
> Comment on attachment 9018179 [details] [diff] [review]
> 1499593-progressmeter.patch
> 
> Review of attachment 9018179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks
> 
> ::: mail/base/content/FilterListDialog.js
> @@ +50,5 @@
> >        document.getElementById('statusbar-progresspanel').removeAttribute('collapsed');
> >        this.progressMeterVisible = true;
> >      }
> >  
> > +    document.getElementById("statusbar-icon").removeAttribute("value");
> 
> So what feature do we lose by this conversion? Does missing "value"
> attribute mean aned behave as "undetermined" mode (progressbar spinning) ?

Tested on Linux and it shows the progressbar spinning. Depending on comments in other progressbar bugs this is a Linux speciality. Windows shows a full bar.

> ::: mail/components/activity/content/activity.xml
> @@ +439,3 @@
> >              } else {
> >                let _percentComplete = 100.0 * aWorkUnitsComplete /
> >                                       aTotalWorkUnits;
> 
> Do fractional/float values work with the new progressbar?

Tried in Inspector and it worked.

> @@ +442,1 @@
> >                element.setAttribute("value", _percentComplete);
> 
> Is it better to use the attribute or the .value property (as done at some
> other places)?

You are the JS specialist. I only converted it. Both versions are already with the old progressbar in the tree.

> @@ +504,5 @@
> >            <xul:hbox>
> >              <xul:vbox flex="1">
> > +              <html:progress value="0" max="100" flex="1"
> > +                             anonid="progressmeter"
> > +                             xbl:inherits="value=progress,mode=progressmode"/>
> 
> Does the xbl:inherit do anything on html:* elements? I don't know but this
> needs to be checked or asked.

I removed now the unneeded mode="...". I couldn't check if this works as it too fast on my profile. Maybe with your big messages profile you can see it.

> ::: mail/components/migration/content/migration.xul
> @@ +79,5 @@
> >      <vbox id="migratingItems" class="indent" style="overflow: auto;" flex="1" align="left"/>
> >      <separator class="thin"/>
> >  
> >      <hbox>
> > +      <html:progress class="progressmeter-statusbar" id="progressBar"  flex="1" value="0" max="100"/>
> 
> Double space before flex.

Fixed

> ::: mailnews/base/content/shutdownWindow.xul
> @@ +21,5 @@
> >      <label id="shutdownStatus_label" value="" />
> >      <separator class="thin" />
> >    </vbox>
> >  
> > +  <html:progress id="shutdown_progressmeter" />
> 
> May it need a 'max' attribute?

I've added them there no max was set.

> ::: mailnews/extensions/newsblog/content/feed-subscriptions.js
> @@ -1885,5 @@
> >      }
> >  
> > -    if (aID == "progressMeter") {
> > -      el.setAttribute("mode", aValue == "?" ? "undetermined" : "determined");
> > -    }
> 
> Why can this (and the callers) be dropped? Don't you need to remove the
> 'value' attribute when "?"?

I don't know how to do this. Could you fix this, please?
Attachment #9018179 - Attachment is obsolete: true
Attachment #9018179 - Flags: review?(acelists)
Attachment #9018212 - Flags: review?(acelists)
Updated after landing of bug 1496632.
Attachment #9018212 - Attachment is obsolete: true
Attachment #9018212 - Flags: review?(acelists)
Attachment #9018550 - Flags: review?(acelists)
Comment on attachment 9018550 [details] [diff] [review]
1499593-progressmeter.patch v4

Review of attachment 9018550 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mailWindow.js
@@ +402,1 @@
>        else {

could add braces while you're here
Added the braces.
Attachment #9018550 - Attachment is obsolete: true
Attachment #9018550 - Flags: review?(acelists)
Attachment #9018589 - Flags: review?(acelists)
Comment on attachment 9018589 [details] [diff] [review]
1499593-progressmeter.patch v5

Review of attachment 9018589 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I found this still needs some changes.

::: mail/base/content/ABSearchDialog.xul
@@ -183,5 @@
>  
>    <statusbar class="chromeclass-status" id="status-bar">
>      <statusbarpanel id="statusText" crop="right" flex="1"/>
>      <statusbarpanel class="statusbarpanel-progress" id="statusbar-progresspanel">
> -      <progressmeter class="progressmeter-statusbar" id="statusbar-icon" mode="normal" value="0"/>

Strange, there does not seem to exist a value of "normal" for mode (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/progressmeter).

::: mail/base/content/mainStatusbar.inc
@@ +12,5 @@
>        </statusbarpanel>
>        <statusbarpanel class="statusbarpanel-progress"
>                        id="quotaPanel" hidden="true">
>          <stack>
>            <progressmeter class="progressmeter-statusbar"

What about this one?

::: mail/base/content/tabmail.xml
@@ +1410,4 @@
>            }
>            else {
>              statusFeedback.showProgress(0);
>              gStatusBar.setAttribute("mode", "normal");

There is no "mode" anymore. Change to .value = 0;

::: mail/components/compose/content/MsgComposeCommands.js
@@ +555,5 @@
>  
>      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
>        gSendOperationInProgress = false;
>        gSaveOperationInProgress = false;
>        document.getElementById("compose-progressmeter").setAttribute("mode", "normal");

Drop this.

@@ +572,3 @@
>          percent = 100;
>  
>        document.getElementById("compose-progressmeter").removeAttribute("mode");

Strange, what is this trying to do? Anyway, you can drop this as there is no "mode" now and we set value in the next line so it will be a determined progress.

It seems you need to review every reference to all the progress elements you touch.

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +88,5 @@
>    onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus)
>    {
>      if (aStateFlags & Ci.nsIWebProgressListener.STATE_START) {
>        // start the spinning
> +      gProgressMeter.removeAttribute("value");

There is also gProgressMeter.setAttribute("mode", "normal"); in this file that needs to be killed.

::: mailnews/compose/content/sendProgress.js
@@ +62,1 @@
>          dialog.progress.setAttribute("value", percent);

Also some dialog.progress.setAttribute("mode", "normal"); to kill in this file. Please review all uses of "dialog.progress".

@@ +66,5 @@
>          dialog.progressText.setAttribute("value", percentMsg);
>        }
>        else
>        {
> +        // Progress meter shouldshow no value in this case.

Space after "should".

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ -1885,5 @@
>      }
>  
> -    if (aID == "progressMeter") {
> -      el.setAttribute("mode", aValue == "?" ? "undetermined" : "determined");
> -    }

if (aID == "progressMeter" && aValue == "?") {
    el.removeAttribute("value");
}

We do not need to handle aValue != "?" here as we will set el.value = aValue; later on.

There is also this line in this file:
document.getElementById("progressMeter").collapsed = true;

I'm not sure "collapsed" is still a property on the <progress> element, it is not in
https://searchfox.org/mozilla-central/source/dom/webidl/HTMLElement.webidl
and https://dxr.mozilla.org/comm-central/source/dom/webidl/HTMLProgressElement.webidl .
Alta may look into that.

::: mailnews/import/content/importDialog.js
@@ -126,5 @@
>    var listbox = document.getElementById("moduleList");
>    var deck = document.getElementById("stateDeck");
>    var header = document.getElementById("header");
>    var progressMeterEl = document.getElementById("progressMeter");
> -  progressMeterEl.mode = "determined";

progressMeterEl.value = 0;
Attachment #9018589 - Flags: review?(acelists) → feedback?(alta88)
(In reply to Richard Marti (:Paenglab) from comment #14)
> > @@ +442,1 @@
> > >                element.setAttribute("value", _percentComplete);
> > 
> > Is it better to use the attribute or the .value property (as done at some
> > other places)?
> 
> You are the JS specialist. I only converted it. Both versions are already
> with the old progressbar in the tree.

Yes, but this new element may have other attributes defined (or none). But it seems .max and .value are defined on it, see https://dxr.mozilla.org/comm-central/source/dom/webidl/HTMLProgressElement.webidl. And it also inherits some common ones like .hidden (https://searchfox.org/mozilla-central/source/dom/webidl/HTMLElement.webidl). So we can keep those.

So please convert setAttribute("value") to '.value =' where possible (and the value should be numeric, not string).
 
> > ::: mailnews/extensions/newsblog/content/feed-subscriptions.js
> > @@ -1885,5 @@
> > > -    if (aID == "progressMeter") {
> > > -      el.setAttribute("mode", aValue == "?" ? "undetermined" : "determined");
> > > -    }
> > Why can this (and the callers) be dropped? Don't you need to remove the
> > 'value' attribute when "?"?
> I don't know how to do this. Could you fix this, please?

Yes, I have pasted the code in the previous comment.
(In reply to :aceman from comment #18)

> >    <statusbar class="chromeclass-status" id="status-bar">
> >      <statusbarpanel id="statusText" crop="right" flex="1"/>
> >      <statusbarpanel class="statusbarpanel-progress" id="statusbar-progresspanel">
> > -      <progressmeter class="progressmeter-statusbar" id="statusbar-icon" mode="normal" value="0"/>
> 
> Strange, there does not seem to exist a value of "normal" for mode
> (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/progressmeter).

yeah, perhaps people just wanted to be explicit that this is not an undetermined progressmeter.
(In reply to :aceman from comment #18)
> Comment on attachment 9018589 [details] [diff] [review]
> 1499593-progressmeter.patch v5
> 
> Review of attachment 9018589 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: mail/base/content/mainStatusbar.inc
> @@ +12,5 @@
> >        </statusbarpanel>
> >        <statusbarpanel class="statusbarpanel-progress"
> >                        id="quotaPanel" hidden="true">
> >          <stack>
> >            <progressmeter class="progressmeter-statusbar"
> 
> What about this one?

Missed, now fixed.

> ::: mail/base/content/tabmail.xml
> @@ +1410,4 @@
> >            }
> >            else {
> >              statusFeedback.showProgress(0);
> >              gStatusBar.setAttribute("mode", "normal");
> 
> There is no "mode" anymore. Change to .value = 0;

All changed.

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +555,5 @@
> >  
> >      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> >        gSendOperationInProgress = false;
> >        gSaveOperationInProgress = false;
> >        document.getElementById("compose-progressmeter").setAttribute("mode", "normal");
> 
> Drop this.

Done.

> @@ +572,3 @@
> >          percent = 100;
> >  
> >        document.getElementById("compose-progressmeter").removeAttribute("mode");
> 
> Strange, what is this trying to do? Anyway, you can drop this as there is no
> "mode" now and we set value in the next line so it will be a determined
> progress.
> 
> It seems you need to review every reference to all the progress elements you
> touch.

I hope I found all.

> ::: mailnews/addrbook/prefs/content/pref-directory-add.js
> @@ +88,5 @@
> >    onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus)
> >    {
> >      if (aStateFlags & Ci.nsIWebProgressListener.STATE_START) {
> >        // start the spinning
> > +      gProgressMeter.removeAttribute("value");
> 
> There is also gProgressMeter.setAttribute("mode", "normal"); in this file
> that needs to be killed.

Done

> ::: mailnews/compose/content/sendProgress.js
> @@ +62,1 @@
> >          dialog.progress.setAttribute("value", percent);
> 
> Also some dialog.progress.setAttribute("mode", "normal"); to kill in this
> file. Please review all uses of "dialog.progress".
> 
> @@ +66,5 @@
> >          dialog.progressText.setAttribute("value", percentMsg);
> >        }
> >        else
> >        {
> > +        // Progress meter shouldshow no value in this case.
> 
> Space after "should".

Fixed.

> ::: mailnews/extensions/newsblog/content/feed-subscriptions.js
> @@ -1885,5 @@
> >      }
> >  
> > -    if (aID == "progressMeter") {
> > -      el.setAttribute("mode", aValue == "?" ? "undetermined" : "determined");
> > -    }
> 
> if (aID == "progressMeter" && aValue == "?") {
>     el.removeAttribute("value");
> }
> 
> We do not need to handle aValue != "?" here as we will set el.value =
> aValue; later on.
> 
> There is also this line in this file:
> document.getElementById("progressMeter").collapsed = true;
> 
> I'm not sure "collapsed" is still a property on the <progress> element, it
> is not in
> https://searchfox.org/mozilla-central/source/dom/webidl/HTMLElement.webidl
> and
> https://dxr.mozilla.org/comm-central/source/dom/webidl/HTMLProgressElement.
> webidl .
> Alta may look into that.

Collapsed is checked by CSS: https://searchfox.org/comm-central/source/mozilla/toolkit/content/minimal-xul.css#45

I'll let alta88 the f?

> ::: mailnews/import/content/importDialog.js
> @@ -126,5 @@
> >    var listbox = document.getElementById("moduleList");
> >    var deck = document.getElementById("stateDeck");
> >    var header = document.getElementById("header");
> >    var progressMeterEl = document.getElementById("progressMeter");
> > -  progressMeterEl.mode = "determined";
> 
> progressMeterEl.value = 0;

Done.
Attachment #9018589 - Attachment is obsolete: true
Attachment #9018589 - Flags: feedback?(alta88)
Attachment #9019443 - Flags: review?(acelists)
Attachment #9019443 - Flags: feedback?(alta88)
(In reply to Richard Marti (:Paenglab) from comment #21)
> > There is also this line in this file:
> > document.getElementById("progressMeter").collapsed = true;
> > 
> Collapsed is checked by CSS:
> https://searchfox.org/comm-central/source/mozilla/toolkit/content/minimal-
> xul.css#45

Yes, but that is an attribute. It would be set by setting the property, but if the property does not exist anymore it will do nothing or even throw error.
Comment on attachment 9019443 [details] [diff] [review]
1499593-progressmeter.patch v6

(In reply to Richard Marti (:Paenglab) from comment #21)
> Created attachment 9019443 [details] [diff] [review]
> 1499593-progressmeter.patch v6
> 
> > ::: mailnews/extensions/newsblog/content/feed-subscriptions.js
> > @@ -1885,5 @@
> > >      }
> > >  
> > > -    if (aID == "progressMeter") {
> > > -      el.setAttribute("mode", aValue == "?" ? "undetermined" : "determined");
> > > -    }
> > 
> > if (aID == "progressMeter" && aValue == "?") {
> >     el.removeAttribute("value");
> > }
> > 
> > We do not need to handle aValue != "?" here as we will set el.value =
> > aValue; later on.
> > 
> > There is also this line in this file:
> > document.getElementById("progressMeter").collapsed = true;
> > 
> > I'm not sure "collapsed" is still a property on the <progress> element, it
> > is not in
> > https://searchfox.org/mozilla-central/source/dom/webidl/HTMLElement.webidl
> > and
> > https://dxr.mozilla.org/comm-central/source/dom/webidl/HTMLProgressElement.
> > webidl .
> > Alta may look into that.
> 
> Collapsed is checked by CSS:
> https://searchfox.org/comm-central/source/mozilla/toolkit/content/minimal-
> xul.css#45
> 
> I'll let alta88 the f?
> 

So html progress can have a value="" to indicate undetermined, so anywhere there is a "?" it can be "" now so you can get rid of the "progressMeter" id check and have the value set as in the next bit that sets value for non "statusText" elements. You probably need to setAttribute for collapsed in clearStatusInfo() if the base html element class doesn't have the property; I recall it can't be hidden since it may make the box resize/jump, which is Bad.
Attachment #9019443 - Flags: feedback?(alta88) → feedback+
Removed the line with "collapsed" in feed-subscriptions.js. The setAttribute("collapsed"... didn't work. Exchanging it to setAttribute("hide"... worked but never showed it after.

I can't do more as my JS knowledge is limited and I want that TB is working and don't break after m-c changes. When you want to improve my changes, please do this in follow-up bugs. Actually I have no more time for this here as <groupbox> and <caption> is the next actual area which needs work on TB.
Attachment #9019443 - Attachment is obsolete: true
Attachment #9019443 - Flags: review?(acelists)
Attachment #9019579 - Flags: review?(acelists)
Looks good, txh Richard! I fixed the feed subscribe hiding/showing and a few smaller changes. hidden="true" isn't a thing - it's a boolean attribute and should be hidden="hidden"
Attachment #9019579 - Attachment is obsolete: true
Attachment #9019579 - Flags: review?(acelists)
Attachment #9019632 - Flags: review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0c7377410adf
Change progressmeter to be html:progress. r=aceman,mkmelin
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Duplicate of this bug: 1502752
You need to log in before you can comment on or make changes to this bug.