Print Preview: Fix issues with large font sizes

Print Preview: Fix issues with large font sizes

BUG=432870
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2771823005
Cr-Commit-Position: refs/heads/master@{#460438}
Committed: https://chromium.googlesource.com/chromium/src/+/7ec0996477a106294857be99b48aa6d7b6425b42

[rbpotter, 2017-03-24 19:59:40]

Description was changed from

==========
Print Preview: Fix issues with large font sizes

BUG=432870, 561411
==========

to

==========
Print Preview: Fix issues with large font sizes

BUG=432870, 561411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2017-03-24 19:59:53]

Description was changed from

==========
Print Preview: Fix issues with large font sizes

BUG=432870, 561411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Fix issues with large font sizes

BUG=432870, 561411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2017-03-24 19:59:53]

rbpotter@chromium.org changed reviewers:
+ dpapad@chromium.org

[rbpotter, 2017-03-27 18:17:20]

Description was changed from

==========
Print Preview: Fix issues with large font sizes

BUG=432870, 561411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Fix issues with large font sizes

BUG=432870
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-03-27 20:48:42]

Can you attach a screenshot of before and after? It will make it easier to
visualize the changes happening in this CL.

[rbpotter, 2017-03-28 22:10:32]

On 2017/03/27 20:48:42, dpapad wrote:
> Can you attach a screenshot of before and after? It will make it easier to
> visualize the changes happening in this CL.

Captured screenshots here:
https://docs.google.com/a/google.com/presentation/d/1B-tzL7P94fb0_3tMm00ffx6E...

[dpapad, 2017-03-28 22:22:44]

https://codereview.chromium.org/2771823005/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_search.css
(left):

https://codereview.chromium.org/2771823005/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.css:29: left:
40px;
Is this not needed anymore? Did you try RTL?

https://codereview.chromium.org/2771823005/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/search/destination_search.css
(right):

https://codereview.chromium.org/2771823005/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/search/destination_search.css:35:
margin-left: 18px;
How does this work in RTL? Should it be the following instead?

-webkit-margin-start: 18px;

[rbpotter, 2017-03-28 23:49:33]

On 2017/03/28 22:22:44, dpapad wrote:
>
https://codereview.chromium.org/2771823005/diff/1/chrome/browser/resources/pr...
> File chrome/browser/resources/print_preview/search/destination_search.css
> (left):
> 
>
https://codereview.chromium.org/2771823005/diff/1/chrome/browser/resources/pr...
> chrome/browser/resources/print_preview/search/destination_search.css:29: left:
> 40px;
> Is this not needed anymore? Did you try RTL?
> 
>
https://codereview.chromium.org/2771823005/diff/1/chrome/browser/resources/pr...
> File chrome/browser/resources/print_preview/search/destination_search.css
> (right):
> 
>
https://codereview.chromium.org/2771823005/diff/1/chrome/browser/resources/pr...
> chrome/browser/resources/print_preview/search/destination_search.css:35:
> margin-left: 18px;
> How does this work in RTL? Should it be the following instead?
> 
> -webkit-margin-start: 18px;

I had not thought about trying RTL, thank you for the suggestions.
I made a few changes and it now seems to look okay in RTL at least
for positioning.

[dpapad, 2017-03-28 23:53:17]

https://codereview.chromium.org/2771823005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_search.css
(right):

https://codereview.chromium.org/2771823005/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.css:20:
-webkit-padding-bottom: 14px;
I don't think webkit-padding-bottom exists, does it? Could not find any
occurrences. AFAIK only -webkit-padding-start and -webkit-padding-end exist (and
same for margin instead of padding).

[rbpotter, 2017-03-29 00:00:22]

https://codereview.chromium.org/2771823005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/search/destination_search.css
(right):

https://codereview.chromium.org/2771823005/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/search/destination_search.css:20:
-webkit-padding-bottom: 14px;
On 2017/03/28 23:53:17, dpapad wrote:
> I don't think webkit-padding-bottom exists, does it? Could not find any
> occurrences. AFAIK only -webkit-padding-start and -webkit-padding-end exist
(and
> same for margin instead of padding).

I had tried -webkit-padding-after and the presubmit suggested it as the
alternative. But, I realized that padding-bottom appears to do the same thing
for RTL and left to right and is used elsewhere so I have changed it to that.

[dpapad, 2017-03-29 00:04:05]

LGTM

[rbpotter, 2017-03-29 00:13:17]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 00:14:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 01:49:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 01:49:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rbpotter, 2017-03-29 17:21:37]

The CQ bit was checked by rbpotter@chromium.org

[commit-bot: I haz the power, 2017-03-29 17:21:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 17:27:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490808097283130,
"parent_rev": "58584e833494875506a8bf21df00022ff21db495", "commit_rev":
"7ec0996477a106294857be99b48aa6d7b6425b42"}

[commit-bot: I haz the power, 2017-03-29 17:28:06]

Description was changed from

==========
Print Preview: Fix issues with large font sizes

BUG=432870
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Fix issues with large font sizes

BUG=432870
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2771823005
Cr-Commit-Position: refs/heads/master@{#460438}
Committed:
https://chromium.googlesource.com/chromium/src/+/7ec0996477a106294857be99b48a...
==========

[commit-bot: I haz the power, 2017-03-29 17:28:07]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7ec0996477a106294857be99b48a...