Material PDF: Fix inconsistent behaviour in page selector, update styling

Material PDF: Fix inconsistent behaviour in page selector, update styling

* Only change page when a value is committed to the selector with enter
* Prevents PDF keyboard shortcuts from firing when any input or textarea
  is active
* Update colors of the viewer and visual styling of the page selector

BUG=439114, 484291
Committed: https://crrev.com/b4e68c87604b6d3b122fe077f8c9d619974317df
Cr-Commit-Position: refs/heads/master@{#332764}

[tsergeant, 2015-06-01 07:24:07]

tsergeant@chromium.org changed reviewers:
+ raymes@chromium.org

[tsergeant, 2015-06-01 07:24:07]

Raymes, can you please take a look? This is a bunch of related changes, happy to
split them up into multiple CLs if you think it's too much.

[raymes, 2015-06-02 01:12:04]

looks good overall! A few more minor comments. Thanks

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css
(right):

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css:23:
color: #fff;
nit: I have a feeling style-wise it's preferred to use rgb/rgba for colours

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js
(right):

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:14:
index: {
As we discussed I wonder if we could potentially remove this?

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:44:
this.$.input._onInput = null;
Will we be able to remove this before you land this patch then?

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:48:
var page = parseInt(e.target.value);
is e.target.value the same as this.pageNo?

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:208: }
nit: no {} to be consistent

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:213: },
We should be able to move this out of PDFViewer and put it up the top with the
other standalone functions. You could pass in document.activeElement as a
parameter to make it more encapsulated.

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:226: if (this.shouldIgnoreKeyEvents_() ||
e.defaultPrevented) return;
nit: return on a newline

[tsergeant, 2015-06-02 05:00:39]

I've also added a simple test for shouldIgnoreKeyEvents

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css
(right):

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css:23:
color: #fff;
On 2015/06/02 01:12:03, raymes wrote:
> nit: I have a feeling style-wise it's preferred to use rgb/rgba for colours

I got a lint message about this: rgb for everything other than colors which can
be expressed in shorthand hex.

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js
(right):

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:14:
index: {
On 2015/06/02 01:12:03, raymes wrote:
> As we discussed I wonder if we could potentially remove this?

Done.

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:44:
this.$.input._onInput = null;
On 2015/06/02 01:12:03, raymes wrote:
> Will we be able to remove this before you land this patch then?

Yes, since I'll be waiting for the Polymer 1.0 patches to land. Removed.

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:48:
var page = parseInt(e.target.value);
On 2015/06/02 01:12:03, raymes wrote:
> is e.target.value the same as this.pageNo?

Done.

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:208: }
On 2015/06/02 01:12:04, raymes wrote:
> nit: no {} to be consistent

Done.

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:213: },
On 2015/06/02 01:12:03, raymes wrote:
> We should be able to move this out of PDFViewer and put it up the top with the
> other standalone functions. You could pass in document.activeElement as a
> parameter to make it more encapsulated.

Done.

https://codereview.chromium.org/1162863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:226: if (this.shouldIgnoreKeyEvents_() ||
e.defaultPrevented) return;
On 2015/06/02 01:12:03, raymes wrote:
> nit: return on a newline

Done.

[raymes, 2015-06-02 05:45:56]

lgtm

https://codereview.chromium.org/1162863002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js
(right):

https://codereview.chromium.org/1162863002/diff/60001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:29:
pageNoCommitted: function(e) {
nit: you can remove e

[tsergeant, 2015-06-03 06:53:11]

I've added some small fixes related to 1.0, and fixed an issue with
shouldIgnoreKeyEvents.

https://codereview.chromium.org/1162863002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js
(right):

https://codereview.chromium.org/1162863002/diff/60001/chrome/browser/resource...
chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:29:
pageNoCommitted: function(e) {
On 2015/06/02 05:45:56, raymes wrote:
> nit: you can remove e

Done.

[tsergeant, 2015-06-04 00:18:12]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2015-06-04 00:18:12]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1162863002/#ps120001
(title: "Fix rebase mistake")

[commit-bot: I haz the power, 2015-06-04 00:19:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162863002/120001

[tsergeant, 2015-06-04 01:27:06]

The CQ bit was unchecked by tsergeant@chromium.org

[tsergeant, 2015-06-04 01:27:13]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2015-06-04 01:28:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162863002/120001

[commit-bot: I haz the power, 2015-06-04 02:05:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-04 02:06:30]

Patchset 7 (id:??) landed as
https://crrev.com/b4e68c87604b6d3b122fe077f8c9d619974317df
Cr-Commit-Position: refs/heads/master@{#332764}