1499593 - C-C: Change progressmeter to be html:progress

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Regression range for that is:

  https://hg.mozilla.org/comm-central/pushloghtml?fromchange=dd917dc324b68599974ff6db50d115b67d118bbe&tochange=e08d005eaad474e65113799e852723dc440c2282

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: screencast.ogv

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

Comment 5

[Jorg K (CEST = GMT+2)]

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

Comment 6

[Jorg K (CEST = GMT+2)]

The range from comment #2 coincides with bug 1491197 and the one from comment #1 with bug 1496189.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

This looks like bug 1497837, will confirm once that's in Daily whether this is fixed.

Comment 8

[Brian Grinstead [:bgrins]]

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.

Comment 9

[Richard Marti (:Paenglab)]

Taking this bug to convert all the progressmeter in mail and mailnewsto to html:progress.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: 1499593-progressmeter.patch

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?

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Richard Marti (:Paenglab)]

Attachment: 1499593-progressmeter.patch

It slipped a unrelated change into the previous patch.

Comment 13

[:aceman]

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 "?"?

Comment 14

[Richard Marti (:Paenglab)]

Attachment: 1499593-progressmeter.patch v3

(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?

Comment 15

[Richard Marti (:Paenglab)]

Attachment: 1499593-progressmeter.patch v4

Updated after landing of bug 1496632.

Comment 16

[Magnus Melin [:mkmelin]]

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

Comment 17

[Richard Marti (:Paenglab)]

Attachment: 1499593-progressmeter.patch v5

Added the braces.

Comment 18

[:aceman]

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;

Comment 19

[:aceman]

(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.

Comment 20

[alexander :surkov (:asurkov)]

(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.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: 1499593-progressmeter.patch v6

(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.

Comment 22

[:aceman]

(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 23

[alta88]

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.

Comment 24

[Richard Marti (:Paenglab)]

Attachment: 1499593-progressmeter.patch v7

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.

Comment 25

[Magnus Melin [:mkmelin]]

Attachment: 1499593-progressmeter.patch

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"

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0c7377410adf
Change progressmeter to be html:progress. r=aceman,mkmelin