1498140 - Print progressbar returns empty like 0% when printing is finished

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Alice0775 White]

Attachment: Screencast (applied the patch attachment 9016172)

+++ This bug was initially created as a clone of Bug #1498075 +++


Reproducible: always

Steps To Reproduce:
1. Print

Actual Results:
While printing in progress, the progress label and progress bar is working as expected.
However, when printing is finished, the progress label indicates as 100%, but the progressbar  returns empty like 0%.

See attached screencast


There is 2 regression.
#1, Progress meter is always 100% and rendered top half.
#2, This Bug, Progress meter is too wide when undetermined state. and Print dialog is too wide.



Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=150a7a5613b7cfc2adac4c801f29391c414042ff&tochange=ccbd30a9a23a7b979abb27211a095aa43d424bcc

Regressed by: ccbd30a9a23a	Alexander Surkov — Bug 1496189 - Password quality meter show always undetermined, r=paolo

Before landing Bug 1496189, the progressbar indicates top half as 100%, bottom half as 0% due to Bug 1491197.
After landing Bug 1496189, the progressbar indicates completely empty as 0%.

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Regressions from some XBL refactoring work.

:surkov, can you investigate this? Sorry the progressmeter stuff is turning into a bit of a nightmare...

Comment 2

[alexander :surkov (:asurkov)]

Do you have STR for OS X?

(In reply to :Gijs (he/him) from comment #1)
> [Tracking Requested - why for this release]:
> Regressions from some XBL refactoring work.
> 
> :surkov, can you investigate this? Sorry the progressmeter stuff is turning
> into a bit of a nightmare...

yeah, apparently there's a whole bunch of progressmeter implementations are in use, which makes the things complicated.

Comment 3

[:Gijs (he/him)]

(In reply to alexander :surkov (:asurkov) from comment #2)
> Do you have STR for OS X?

I do not, I haven't looked for this myself. If you're not in a position to test anywhere else, perhaps you can ask your reviewers for help.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

switch to HTML:progress

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

wrap html:progress by vbox to keep it same height as xul:progressmeter

Comment 6

[:Paolo Amadini]

Comment on attachment 9017099 [details] [diff] [review]
patch

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

I could only test this partially on Windows, but it mostly looks good, so let's land this patch and see if it fixes the bug we've observed for everyone.

::: toolkit/components/printing/content/printProgress.js
@@ +84,5 @@
>  
>      onProgressChange(aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress) {
>        if (switchUI) {
>          dialog.tempLabel.setAttribute("hidden", "true");
> +        dialog.progress.removeAttribute("hidden");

I haven't been able to test this case specifically, but if you have a <vbox> wrapping the <html:progress>, it is the <vbox> that should be hidden and shown again, since it's the direct child of <row>.

(It would've been better if we didn't show and hide individual cells, but this is what we have...)

::: toolkit/components/printing/content/printProgress.xul
@@ +48,5 @@
>          </row>
>          <row class="thin-separator">             
>            <hbox pack="end">
> +            <html:label id="dialog.progressLabel" for="dialog.progress"
> +                        style="margin-right: 1em;">&progress;</html:label>

Is this required because a XUL label linked to an HTML element won't work for accessibility?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to :Paolo Amadini from comment #6)
> Comment on attachment 9017099 [details] [diff] [review]
> patch
> 
> Review of attachment 9017099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I could only test this partially on Windows, but it mostly looks good, so
> let's land this patch and see if it fixes the bug we've observed for
> everyone.
> 
> ::: toolkit/components/printing/content/printProgress.js
> @@ +84,5 @@
> >  
> >      onProgressChange(aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress) {
> >        if (switchUI) {
> >          dialog.tempLabel.setAttribute("hidden", "true");
> > +        dialog.progress.removeAttribute("hidden");
> 
> I haven't been able to test this case specifically, but if you have a <vbox>
> wrapping the <html:progress>, it is the <vbox> that should be hidden and
> shown again, since it's the direct child of <row>.

afaik, vbox takes nothing if no content, but I can hide vbox instead if you think it's safer to do so

> (It would've been better if we didn't show and hide individual cells, but
> this is what we have...)
> 
> ::: toolkit/components/printing/content/printProgress.xul
> @@ +48,5 @@
> >          </row>
> >          <row class="thin-separator">             
> >            <hbox pack="end">
> > +            <html:label id="dialog.progressLabel" for="dialog.progress"
> > +                        style="margin-right: 1em;">&progress;</html:label>
> 
> Is this required because a XUL label linked to an HTML element won't work
> for accessibility?

yes, alternatively I could do aria-labelledby, if it makes anything nicer.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch

reviewer's comment addressed

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e32d6dc9d1e
make print progress dialog to use HTML:progress element, r=paolo

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2e32d6dc9d1e

Comment 11

[laszlo.bialis]

Hi,

I've reproduced the issue on Win 10 x64 using Firefox Nightly 64.0a1 (2018-10-13). 
Also tested on Win 10 x64 using the latest Nightly 64.0a1 (2018-10-22) and the issue is fixed.