Closed Bug 1498140 Opened Last year Closed Last year

Print progressbar returns empty like 0% when printing is finished

Categories

(Core :: XUL, defect)

64 Branch
x86_64
Windows 10
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 + verified

People

(Reporter: alice0775, Assigned: surkov)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

+++ 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%.
Attachment #9016204 - Attachment is patch: false
Attachment #9016204 - Attachment mime type: text/plain → video/mp4
[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...
Component: Printing → XUL
Flags: needinfo?(surkov.alexander)
Product: Toolkit → Core
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.
Flags: needinfo?(surkov.alexander)
(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.
Attached patch patch (obsolete) — Splinter Review
switch to HTML:progress
Assignee: nobody → surkov.alexander
Attachment #9017076 - Flags: review?(paolo.mozmail)
Attached patch patchSplinter Review
wrap html:progress by vbox to keep it same height as xul:progressmeter
Attachment #9017076 - Attachment is obsolete: true
Attachment #9017076 - Flags: review?(paolo.mozmail)
Attachment #9017099 - Flags: review?(paolo.mozmail)
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?
Attachment #9017099 - Flags: review?(paolo.mozmail) → review+
(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.
Attached patch patchSplinter Review
reviewer's comment addressed
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e32d6dc9d1e
make print progress dialog to use HTML:progress element, r=paolo
https://hg.mozilla.org/mozilla-central/rev/2e32d6dc9d1e
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Blocks: 1428869
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.