165620 - Size of progress bar in print progress dialog depends on length of page's title

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P3)

Description

[Scott Gifford]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1) Gecko/20020826
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1) Gecko/20020826

When printing a page with Mozilla 1.1, the length of the progress bar depends on
the length of the page's title.  A fairly long title will use the whole dialog
for the progress bar, as it should.  A one-character title will display a goofy
looking little tiny progress bar in a normal-sized progress window.
 

Reproducible: Always

Steps to Reproduce:
1. View longtitle.html (attached)
2. Print the page (you can print to a file to save paper. :-) )
3. Watch the progress dialog

4. View sometitle.html (attached)
5. Print the page
6. Watch the progress dialog

7. View shorttitle.html (attached)
8. Print the page
9. Watch the progress dialog
Actual Results:  
The progress bar got progressively shorter, until it finally looked puny and
ridiculous.



Expected Results:  
Displayed a reasonably sized title bar for each print dialog.

Comment 1

[Scott Gifford]

Attachment: Web page with a long title

Comment 2

[Scott Gifford]

Attachment: Web page with a medium length title

Comment 3

[Scott Gifford]

Attachment: Web page with a short title

Comment 4

[Scott Gifford]

Attachment: Screenshot of longtitle.html printing

Comment 5

[Scott Gifford]

Attachment: Screenshot of sometitle.html printing

Comment 6

[Scott Gifford]

Attachment: Screenshot of shorttitle.html printing

Comment 7

[Scott Gifford]

I still see this in 2002091016, and since nobody's marked it a DUP yet setting
to New.

Comment 8

[Scott Gifford]

With 2003020308, this bug still exists, but it depends on the length of the
page's URL instead of it's title.  I can't create long or short URLs as test
cases, but compare one of the links above to http://www.mozilla.org/ which has a
shorter URL.

Comment 9

[Scott Gifford]

Still present in 20030624 (1.4rc3).

Comment 10

[Scott Gifford]

Still present in FireBird 20030703.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b)
Gecko/20040329 Firefox/0.8.0+

Comment 12

[Karsten Düsterloh]

With Mozilla 2004-03-29, it's still the title that the progress bar length
depends upon...

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Confirming that the length of the title is taken to size the progress bar.

-> Update summary

Comment 14

[Karsten Düsterloh]

Attachment: make progress bar as wide as possible

Both the print progress dialogs in SM/XPFE and Fx/Toolkit just lacked the flex
attribute at the respective grid column.

Comment 15

[[e-mail]]

Comment on attachment 145190 [details] [diff] [review]
make progress bar as wide as possible

Eww... how on earth does this grid work?

Comment 16

[Karsten Düsterloh]

Attachment: Major dialog clean-up (Seamonkey)

Comment 17

[Karsten Düsterloh]

Comment on attachment 149066 [details] [diff] [review]
Major dialog clean-up (Seamonkey)

Neil, what do you think? I corrected the XUL and got rid of plenty unused
cruft.
(I tried not to touch everything, but diff still disagrees. I'll provide a diff
-w if necessary.)

Comment 18

[[e-mail]]

Comment on attachment 149066 [details] [diff] [review]
Major dialog clean-up (Seamonkey)

I wasn't able to apply your diff; it looked like three diffs pasted together.
Also it looks as if you were killing trailing whitespace which is a good idea
in the actual file but not in the - lines in the patch.

>+ *   Karsten "Mnyromyr" Duesterloh <[e-mail]>
We accept umlauted letters (ue = ü in UTF-8, of course...)

>+// deck index constants
>+const gkiTitleURL          = 0;
>+const gkiTitleComplete     = 1;
>+const gkiProgressPreparing = 0;
>+const gkiProgressMeter     = 1;
You only use half of these... plus just use a "k" prefix for these, and "a" for
function arguments.

>+      // Put progress meter at 100%.
>+      dialog.progress.setAttribute( "value", 100 );
>+      dialog.progress.setAttribute( "mode", "normal" );
>+      SetProgressPercentage(100);
Why not make setProgressPercentage set the value as well?

>+  QueryInterface : function(iid)
>+  {
>+    if (iid.equals(Components.interfaces.nsIWebProgressListener) || iid.equals(Components.interfaces.nsISupportsWeakReference))
>+      return this;
>+    throw Components.results.NS_NOINTERFACE;
Note that timeless is touching this (Or trying to get sr...).

>+function SetProgressTitle(aoProgressParams)
You might as well use the global progressParams here.

>+    const ksDocTitle = aoProgressParams.docTitle;
>+    const ksDocURL   = aoProgressParams.docURL;
I don't see the point of these...

>+    if (docURL != ksDocURL && !dialog.title.value)
The previous code tested docTitle rather than dialog.title.value

>+  dialog.progressText.basevalue = dialog.progressText.getAttribute("basevalue");
Please create a global variable for this.

>           <hbox pack="end">
I think if you put align="end" on the columns you can get rid of the hboxes.

>     <separator/>
>     <hbox id="CancelButton" pack="end">
>-      <button id="cancel" label="&dialogCancel.label;"
>-        oncommand="doCancelButton()"/>
>+      <button id="cancel"
>+              label="&dialogCancel.label;"
>+              oncommand="doCancelButton()"
>+              disabled="true"/>
>     </hbox>
> </window>
Did you consider converting this to a <dialog/>?

Comment 19

[Karsten Düsterloh]

Attachment: Major dialog clean-up V2 (Seamonkey)

All remarks of comment #18 are addressed, except the QueryInterface bit.
Apart from that: after changing from <window class="dialog"> to <dialog>, I
wasn't able to get the dialog's getButton function in the onLoad handler - the
binding does not seem to be loaded yet. So I dropped the disabling of the
cancel button before the printing progress actually starts.

Comment 20

[Karsten Düsterloh]

Attachment: Dialog overhaul v3 (Seamonkey)

All remarks of comment #18 are addressed now, except the QueryInterface bit,
but including the cancel button disabling that was missing in the patch v2.

The diff is bigger than the actual files (and whitespace diffing doesn't help).
:(

Comment 21

[[e-mail]]

Comment on attachment 152955 [details] [diff] [review]
Dialog overhaul v3 (Seamonkey)

>+// attribute names
>+const ksMode     = "mode";
>+const ksValue    = "value";
>+const ksDisabled = "disabled";
>+// attribute values
>+const ksUndetermined = "undetermined";
Just use the literal strings, please.

>+  onStateChange: function(aWebProgressIgnored, aRequestIgnored, aStateFlags, aStatusIgnored)
Please don't suffix Ignored to variables.

>+function SetProgressPercentage(adPercentage, asMode)
>+{ // set progress meter mode
>+  if (asMode)
>+    dialog.progress.setAttribute(ksMode, asMode);
>+  else
>+    dialog.progress.removeAttribute(ksMode);
>+  // set percentage as text if non-negative
>+  if (adPercentage < 0)
I've noticed that the mode is always "undetermined" if and only if the
percentage is -1, so you could save yourself a parameter. Speaking of which,
we're not Hungarian, a simple "a" prefix suffices.

>+  dialog = {titleDeck   : document.getElementById("dialog.titleDeck"),
Please split this after the {, and use 2-space indent for the properties.

Comment 22

[Karsten Düsterloh]

Attachment: Dialog overhaul v4 (Seamonkey)

Addressed all remarks of comment #21.

Comment 23

[[e-mail]]

Comment on attachment 152996 [details] [diff] [review]
Dialog overhaul v4 (Seamonkey)

>+ *   Karsten "Mnyromyr" Düsterloh <[e-mail]>
Oops ;-)

>+  dialog = {
>+              titleDeck   : document.getElementById("dialog.titleDeck"),
Actually I mean 2 pixels indent, not extra indent, i.e.
dialog = {
  titleDeck ...

>+        buttonpack="end"
IIRC this is actually the default anyway.

Comment 24

[[e-mail]]

Comment on attachment 152996 [details] [diff] [review]
Dialog overhaul v4 (Seamonkey)

>+function SetProgressTitle()
I've just remembered that bug 160939 has the correct solution.

Comment 25

[Karsten Düsterloh]

Attachment: Dialog overhaul v5 (Seamonkey)

Adressed comments #23 and #24 (amidst loathing my non-UTF8-editor).
This is going to be the best reviewed unimportant bug fix ever. ;-)

Comment 26

[jag (Peter Annema)]

Comment on attachment 153037 [details] [diff] [review]
Dialog overhaul v5 (Seamonkey)

>Index: global/resources/content/printProgress.js
>===================================================================

>+// deck index constants
>+const kiTitleComplete = 1;
>+const kiProgressMeter = 1;

For constants we typically use all uppercase names, e.g.

const TITLE_COMPLETE_DECK = 1;
const PROGRESS_METER_DECK = 1;


>+  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus)
>+  {
>+    if (aStateFlags & Components.interfaces.nsIWebProgressListener.STATE_START)
>+    { // put progress meter in undetermined mode
>+      SetProgressPercentage(-1);
>     }
>+    if (aStateFlags & Components.interfaces.nsIWebProgressListener.STATE_STOP)
>+    { // we are done printing; indicate completion in title area
>+      dialog.titleDeck.selectedIndex = kiTitleComplete;
>+      SetProgressPercentage(100);
>+      window.close();
>+    }
>+  },

Please put comments on their own line instead of after the opening curly.


>+    if (progressParams)
>     {
>+      dialog.title.crop  = progressParams.docTitle ? "end" : "center";
>+      dialog.title.value = progressParams.docTitle || progressParams.docURL;
>+    }

Can both docTitle and docURL be null or empty? Empty is probably ok, but we
shouldn't display "null" in the title. I guess docURL should never be null
though.

Also, it looks like the old code stored the existing values so it could compare
the "new" ones and wouldn't update the title more often than needed. Especially
if they don't coalesce some of these progress notifications, you could end up
doing a lot more work here than needed.


>+    // calculate percentage and update progress meter
>+    if (aMaxTotalProgress > 0)
>     {
>+      var iPercent = Math.min(Math.round(aCurTotalProgress * 100 / aMaxTotalProgress), 100);

Given that aCurTotalProgress <= aMaxtotalProgress, you shouldn't need the
Math.min here.
And please use |percentage| instead of |iPercent|. When working in existing
code, try to use the file's coding, style, and naming conventions.


>+function SetProgressPercentage(aPercentage)

Please use setProgressPercentage to stay within the file's style.


>+    dialog.progress.mode = "determined";

I think that should be "normal". I've seen a few other places use "determined",
though. And the XUL reference at XUL planet uses "determined".


>+  if (window.arguments[1])

This check should probably be something like

  if (window.arguments.length >= 2 && window.arguments[1])

Unless we always have two arguments passed in, in which case I wonder if the
second argument will ever be null.


>+    if (progressParams)
>+    {
>+      dialog.title.crop  = progressParams.docTitle ? "end" : "center";
>+      dialog.title.value = progressParams.docTitle || progressParams.docURL;
>     }

This code should be factored out into a helper function instead of having it
twice.


>Index: global/resources/locale/en-US/printProgress.dtd
>===================================================================

>+   - The Original Code is Mozilla Communicator client code, released
>+   - March 31, 1998.
>+   -
>+   - The Initial Developer of the Original Code is
>+   - Netscape Communications Corporation.
>+   - Portions created by the Initial Developer are Copyright (C) 1998-2000
>+   - the Initial Developer. All Rights Reserved.

Right now I would just not bother adding this license. I think some sections
would need to be changed, e.g. the release date (could actually be March 31,
1998, though), and probably the copyright part... Not sure, and I'd rather not
add that until we're sure.

If you really want to add it, talk to gerv ([e-mail], I think).

Your changes look good, despite all these comments :-) Please address them and
attach a new patch.

Comment 27

[jag (Peter Annema)]

>I think that should be "normal". I've seen a few other places use "determined",
>though. And the XUL reference at XUL planet uses "determined".

I forgot to complete that train of thought. As far as I can tell, the code only
really looks for "undetermined", anything else counts as "normal". Since the
majority of our code uses "normal", I suggest you use it too.

Comment 28

[Karsten Düsterloh]

[e-mail] aber hob zu reden an und schrieb:
> Can both docTitle and docURL be null or empty? Empty is probably ok,
> but we shouldn't display "null" in the title. I guess docURL should
> never be null though.

I guess so, too.

> Also, it looks like the old code stored the existing values so it 
> could compare the "new" ones and wouldn't update the title more often
> than needed. Especially if they don't coalesce some of these progress
> notifications, you could end up doing a lot more work here than
> needed.

Neil and bz seem to agree that since bug 209634 is fixed, this needs no longer
be taken care of here.
 
>>+	var iPercent = Math.min(Math.round(aCurTotalProgress * 100 /
> aMaxTotalProgress), 100);
> 
> Given that aCurTotalProgress <= aMaxtotalProgress, you shouldn't need the
> Math.min here.

I actually wondered if the old code wanted to prevent rounding errors...

>>+    dialog.progress.mode = "determined";
> 
> I think that should be "normal". I've seen a few other places use
> "determined", though. And the XUL reference at XUL planet uses
> "determined".

I used "determined", because:
(1) It's used most of the times for standalone progressMeters - "normal" seems
to be restricted to tree cells of type=progressmeter.
(2) The XUL reference
(3) The winstripe theme is using it (but that's Firefox, I know). 
Actually, I even tried /deleting/ the mode attribute when not in "undetermined"
mode and it worked well with Seamonkey (classic and modern), so this may be an
alternative.

>   if (window.arguments.length >= 2 && window.arguments[1])
> 
> Unless we always have two arguments passed in, in which case I wonder
> if the second argument will ever be null.

I couldn't find proof for it (not) being null, so we'd better check.

> Right now I would just not bother adding this license. I think some sections
> would need to be changed, e.g. the release date (could actually be March 31,
> 1998, though), and probably the copyright part... Not sure, and I'd rather not
> add that until we're sure.
> 
> If you really want to add it, talk to gerv ([e-mail], I think).

I mailed Gerv.

> Please address them and attach a new patch.

I'll do.

Comment 29

[Gervase Markham [:gerv]]

It's generally fine to add the license to files you find without it, as long as
you are using the boilerplate linked from http://www.mozilla.org/MPL/. 

(There are a couple of files we don't have permission to do yet - I should
probably mark them explicitly. Hmm...)

Gerv

Comment 30

[Karsten Düsterloh]

I prepared a patch with all of jag's comments addressed but the progressmeter
mode issue. I'm still quite uncomfortable with using mode="normal".

There are old docs like <http://www.mozilla.org/xpfe/xptoolkit/meters.html> that
use "normal" and "undetermined", or
<http://www.mozilla.org/xpfe/xulref/progressmeter.html#mode> which lists
"normal", "determined" and "undetermined", but without any explanation.
More modern docs like the Mozdev book "Creating Applications with Mozilla"
<http://books.mozdev.org/html/appc-77249.html> and Nigel McFarlane's "Rapid
Application Development with Mozilla" (chapter 8) suggest only "determined" and
"undetermined".

Most of the old Mozilla code uses "normal", but newer code uses "determined".

AFAICT from an extended lxr session:
The mode attribute has no "justification" in an interface, it's just part the
progressmeter.xml and is purely used as a hint for CSS styling.
The value "normal" is *never* used for progressmeters in CSS.
The value "determined" is used only *once* in CSS (winstripe), but that's in
Firefox and not in Seamonkey.
The value "undetermined" is used regularly in CSS.

I'd prefer setting "determined" (like I already did) to be consistent with
modern documentation. Alternatively, I suggest *not* setting the attribute at
all - that would have the same effect and even save us HD and RAM space. ;-)

Neil, Jag: what do you think?

Comment 31

[[e-mail]]

Just to be different, trees like to use "normal" and "undetermined".
If only winstripe uses "determined", then that could be a bug in winstripe...

Comment 32

[jag (Peter Annema)]

http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.xul#350

350       <progressmeter class="progressmeter-statusbar" id="statusbar-icon"
mode="normal" value="0"/>

From your patch:

-            <progressmeter id="dialog.progress" mode="normal" value="0"/>

And
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/browser/resources/content/nsBrowserStatusHandler.js&rev=1.1&root=/cvsroot#124

That is v1.1 of nsBrowserStatusHandler.js, the |.mode = ...| lines have since
been removed, but I'm now pretty sure it's supposed to be "normal".

I take those as authorative on what the correct values are.

I wonder if "determined" is a bug that snuck in and was copied since, both in
source and print. As far as I know "normal" is the correct value and the book(s)
and XUL reference have it wrong. I guess Mozilla.org's XUL Reference allows
"determined" as a value because it's been used in so many places now. I don't
think it's the "new" way of doing this though.

Go ahead and just remove the attribute though, if you'd rather do that than use
"normal".

Looking forward to your updated patch so I can slap sr= on it :-)

Comment 33

[[e-mail]]

Well, I've trawled through the code, and it does some weird things, but
basically we can assume that the second parameter is a non-null nsISupports,
which is a pity because then we have to QueryInterface it, rather than getting
XPConnect to do the legwork for us.

Comment 34

[Karsten Düsterloh]

Attachment: Dialog overhaul v6 (Seamonkey)

Addressed comment #26; and the "mode" attribute is now "undetermined" or not
set at all. See also comment #28.

Comment 35

[jag (Peter Annema)]

Comment on attachment 153345 [details] [diff] [review]
Dialog overhaul v6 (Seamonkey)

sr=jag

Comment 36

[[e-mail]]

Comment on attachment 153345 [details] [diff] [review]
Dialog overhaul v6 (Seamonkey)

// XXX this test should never fail
>+  if (window.arguments.length > 1 && window.arguments[1])
>+  {
>+    progressParams = window.arguments[1].QueryInterface(Components.interfaces.nsIPrintProgressParams);
>+    setProgressTitle();
>+  }

Comment 37

[Karsten Düsterloh]

Seamonkey part checked in (2004-07-16 05:56).
I'll provide a respective aviary patch.

Comment 38

[Karsten Düsterloh]

Attachment: Dialog overhaul v6 (Aviary)

This patch makes exactly the same changes to the /toolkit printing progress
files as the Seamonkey version does to those in /xpfe, but with different
chrome-URIs and modifications for the preprocessor where necessary.

Comment 39

[Mike Connor [:mconnor]]

as a note, pch is away from the project, and isn't responding to review
requests.  Also, SR isn't needed for toolkit, and ben doesn't respond to SR
requests.

Comment 40

[Karsten Düsterloh]

Comment on attachment 153444 [details] [diff] [review]
Dialog overhaul v6 (Aviary)

Re comment #39:
> pch is away from the project

Then /owners.html should reflect that. :(

Trying bryner; and if any  of you aviary folks knows someone more suitable,
please adjust. I don't care enough for the *birds to hunt down any reviewers
for that. Feel free to add any keywords necessary.

Comment 41

[Mike Connor [:mconnor]]

if this gets review in time, great, but this isn't a blocker.  Removing
needed-aviary1.0 flag to go with this.

Comment 42

[Karsten Düsterloh]

Attachment: Dialog overhaul v7 (Aviary) [updated against trunk /toolkit]

Bitrot removal (locales directories were moved), otherwise unchanged.

Comment 43

[Adam Guthrie]

*** Bug 296393 has been marked as a duplicate of this bug. ***

Comment 44

[Karsten Düsterloh]

Marking as fixed and clearing FF review request after over 14 months of
inactivity - they're very obviously not interested.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mconner, any reason why you didn't reviewed the patch for Fx? It's a shame that
this wonderful UI fix won't come into Fx! :/

Comment 46

[Juan Jimenez]

Oh brother, removed and marked as fixed for lack of interest? Sorry I even
bothered to try reporting it....

Comment 47

[Karsten Düsterloh]

> Oh brother, removed and marked as fixed for lack of interest? Sorry I even
> bothered to try reporting it....

Hey, I fixed it for SeaMonkey and that's about all that matters to me.
Why should I care for FF if they ain't even interested?

Comment 48

[Mike Connor [:mconnor]]

Its a big patch, in code I don't know that's not well-documented, and any of the
times I've taken a crack at understanding the patch well enough to review it,
you haven't been around to explain certain bits.  Its a big step to assume that
we don't want it just because there's a review backlog.

Comment 49

[Karsten Düsterloh]

> Its a big patch, in code I don't know that's not well-documented,

Granted, ...

> and any of the times I've taken a crack at understanding the patch well
> enough to review it, you haven't been around 

Huh?!

> to explain certain bits.
> Its a big step to assume that we don't want it just because there's a 
> review backlog.

This bug hasn't seen *any* review-related comment since 2004-07-16. 
If you had questions, why didn't you ask?
If you feel that you can't review it, why didn't you pass it on?

Sorry, but I really don't have a working crystal ball...