Print Preview: Fading in/out printing options.

Print Preview: Fading in/out printing options.

Also fixing a minor rtl regression issue.

BUG=83421
TEST=See bug description.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87127

[Lei Zhang, 2011-05-26 01:13:28]

Mostly ok, just 1 question below. Maybe Chris knows the answer.

http://codereview.chromium.org/6979024/diff/2001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6979024/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:481:
fadeOutElement($('landscape-option'));
I wonder if we should still disable it though? Can users with a screen reader
still get to faded out elements?

[Chris Guillory, 2011-05-26 01:36:40]

On 2011/05/26 01:13:28, Lei Zhang wrote:
> Mostly ok, just 1 question below. Maybe Chris knows the answer.
> 
>
http://codereview.chromium.org/6979024/diff/2001/chrome/browser/resources/pri...
> File chrome/browser/resources/print_preview.js (right):
> 
>
http://codereview.chromium.org/6979024/diff/2001/chrome/browser/resources/pri...
> chrome/browser/resources/print_preview.js:481:
> fadeOutElement($('landscape-option'));
> I wonder if we should still disable it though? Can users with a screen reader
> still get to faded out elements?
Perhaps. I could see all the hidden elements on the print preview page. We can
address those in crbug.com/83407.

[Lei Zhang, 2011-05-27 22:58:01]

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.html (right):

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.html:51: <hr id="hr-before-copies"
class="inivisible">
typo: inivisible

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:579: $('print-button').disabled =
(!isNumberOfCopiesValid() ||
nit: add parenthesis.

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:651: var printToPDF =
getSelectedPrinterName() == PRINT_TO_PDF;
move this to the top and set copies based on printToPDF.

[dpapad, 2011-05-27 23:16:17]

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.html (right):

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.html:51: <hr id="hr-before-copies"
class="inivisible">
On 2011/05/27 22:58:01, Lei Zhang wrote:
> typo: inivisible

Done.

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:579: $('print-button').disabled =
(!isNumberOfCopiesValid() ||
On 2011/05/27 22:58:01, Lei Zhang wrote:
> nit: add parenthesis.

Done. BTW I thought we prefer to use parenthesis only when required.

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:651: var printToPDF =
getSelectedPrinterName() == PRINT_TO_PDF;
On 2011/05/27 22:58:01, Lei Zhang wrote:
> move this to the top and set copies based on printToPDF.

Done.

[Lei Zhang, 2011-05-27 23:27:30]

LGTM

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:579: $('print-button').disabled =
(!isNumberOfCopiesValid() ||
On 2011/05/27 23:16:17, dpapad wrote:
> On 2011/05/27 22:58:01, Lei Zhang wrote:
> > nit: add parenthesis.
> 
> Done. BTW I thought we prefer to use parenthesis only when required.

Multi-line else statement = required.

[Lei Zhang, 2011-05-27 23:28:55]

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/6979024/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:579: $('print-button').disabled =
(!isNumberOfCopiesValid() ||
On 2011/05/27 23:27:30, Lei Zhang wrote:
> On 2011/05/27 23:16:17, dpapad wrote:
> > On 2011/05/27 22:58:01, Lei Zhang wrote:
> > > nit: add parenthesis.
> > 
> > Done. BTW I thought we prefer to use parenthesis only when required.
> 
> Multi-line else statement = required.

Err, I meant to say braces. that is:

if () {
...
} else {
...
}

[commit-bot: I haz the power, 2011-05-28 01:06:07]

Change committed as 87127