print_preview: Workaround of focus-scroll issue.

print_preview: Workaround of focus-scroll issue.

* Make margin-control-textbox elements unfocusable if not in custom margin mode.

* Reset the scroll position of preview-area when margin-control-textbox is
  focused.

BUG=601341
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/dedbae45b850e09e8de724bc678cbec27a7ed6c8
Cr-Commit-Position: refs/heads/master@{#390902}

[tkent, 2016-04-27 06:43:59]

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

[tkent, 2016-04-27 06:43:59]

dpapad@, would you review this please?

I think accessing preview-area in MarginControl is too dirty.

[dpapad, 2016-04-27 18:07:35]

On 2016/04/27 at 06:43:59, tkent wrote:
> dpapad@, would you review this please?
> 
> I think accessing preview-area in MarginControl is too dirty.

+thestig,dbeam

I am very reluctant accepting this fix, for two reasons:

1) It is not sufficient. I am still able to reproduce the problem, although this
   time instead of the automatic scrolling to happen towards the bottom, it
   happens towards the right.

   Repro steps: Open print preview. Zoom in enough until a horizontal scrollbar
   appears in the preview area. Hit the Tab key a couple of times, observer
   unwanted gray rectangle. Let me know if you have problems reproducing
2) The workaround in margin_control.js seems hacky and without a clear plan on
   how to fix it the right way in the future, so this TODO is likely to remain
   in the code indefinitely.

Is there enough evidence to support that the original change to Blink is a solid
improvement? Specifically I am thinking whether it should be modified to ignore
elements that are marked with "position: absolute" like in the print preview
case. I don't see how automatically scrolling an absolutely positioned element
will ever produce the expected behavior.

[tkent, 2016-04-28 02:08:17]

On 2016/04/27 at 18:07:35, dpapad wrote:
> On 2016/04/27 at 06:43:59, tkent wrote:
> > dpapad@, would you review this please?
> > 
> > I think accessing preview-area in MarginControl is too dirty.
> 
> +thestig,dbeam
> 
> I am very reluctant accepting this fix, for two reasons:
> 
> 1) It is not sufficient. I am still able to reproduce the problem, although
this
>    time instead of the automatic scrolling to happen towards the bottom, it
>    happens towards the right.

Yeah, we need to reset scrollLeft too.

> 2) The workaround in margin_control.js seems hacky and without a clear plan on
>    how to fix it the right way in the future, so this TODO is likely to remain
>    in the code indefinitely.

The TODO comment contains better approaches.  Aren't they enough?

> Is there enough evidence to support that the original change to Blink is a
solid
> improvement? Specifically I am thinking whether it should be modified to
ignore
> elements that are marked with "position: absolute" like in the print preview
> case. I don't see how automatically scrolling an absolutely positioned element
> will ever produce the expected behavior.


Yes, it's solid.  I'm confident that the current Blink behavior is correct.
In this case, positioning parent of margin-control-textbox is preview-area, not
the document, because preview-area has position:relative. So Blink should scroll
preview-area.
Even if we remove position:relative from preview-area, then the whole document
will be scrolled.
This behavior conforms to CSS standard, and interoperable.

[tkent, 2016-04-28 02:20:39]

On 2016/04/28 at 02:08:17, tkent wrote:
> > 1) It is not sufficient. I am still able to reproduce the problem, although
this
> >    time instead of the automatic scrolling to happen towards the bottom, it
> >    happens towards the right.
> 
> Yeah, we need to reset scrollLeft too.

Done in Patch Set 2.

[Dan Beam, 2016-04-28 17:29:29]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-04-28 17:29:29]

i don't really understand all the factors at play here, but if there's a
focusable element that's visually hidden, we should not be doing that in general
(against the a11y guidelines).

if the element is simply offscreen but visible, and tabbing to it scrolls the
frame: this happens across all of webui and the web at large.  it's funky but
probably what some people want/expect.

also a note: aria-* is useful to customize an element's behavior with regard to
accessibility -- would using something like aria-hidden help here / be respected
by blink?

[Dan Beam, 2016-04-28 17:29:43]

Description was changed from

==========
print_preview: Workaround of focus-scroll issue.

* Make margin-control-textbox elements unfocusable if not in custom margin mode.

* Reset the scroll position of preview-area when margin-control-textbox is
  focused.

BUG=601341
==========

to

==========
print_preview: Workaround of focus-scroll issue.

* Make margin-control-textbox elements unfocusable if not in custom margin mode.

* Reset the scroll position of preview-area when margin-control-textbox is
  focused.

BUG=601341
==========

[Dan Beam, 2016-04-28 17:29:44]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[dpapad, 2016-04-29 17:59:02]

On 2016/04/28 at 17:29:29, dbeam wrote:
> i don't really understand all the factors at play here, but if there's a
focusable element that's visually hidden, we should not be doing that in general
(against the a11y guidelines).
> 
> if the element is simply offscreen but visible, and tabbing to it scrolls the
frame: this happens across all of webui and the web at large.  it's funky but
probably what some people want/expect.
> 
> also a note: aria-* is useful to customize an element's behavior with regard
to accessibility -- would using something like aria-hidden help here / be
respected by blink?

aria-hidden is already being used at line 339.

[dpapad, 2016-04-29 17:59:13]

lgtm

https://codereview.chromium.org/1917413002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/previewarea/margin_control.js
(right):

https://codereview.chromium.org/1917413002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/previewarea/margin_control.js:392: var
opacity = parseInt(elStyle.getPropertyValue('opacity'), 10);
Nit: Instead of comparing opacity to 0 twice, how about the following

var disabled = parseInt(elStyle.getPropertyValue('opacity'), 10) == 0;
this.textbox_.setAttribute('aria-hidden', disabled);
this.textbox_.disabled = disabled;

[tkent, 2016-05-02 04:58:41]

Description was changed from

==========
print_preview: Workaround of focus-scroll issue.

* Make margin-control-textbox elements unfocusable if not in custom margin mode.

* Reset the scroll position of preview-area when margin-control-textbox is
  focused.

BUG=601341
==========

to

==========
print_preview: Workaround of focus-scroll issue.

* Make margin-control-textbox elements unfocusable if not in custom margin mode.

* Reset the scroll position of preview-area when margin-control-textbox is
  focused.

BUG=601341
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tkent, 2016-05-02 08:05:19]

https://codereview.chromium.org/1917413002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/print_preview/previewarea/margin_control.js
(right):

https://codereview.chromium.org/1917413002/diff/20001/chrome/browser/resource...
chrome/browser/resources/print_preview/previewarea/margin_control.js:392: var
opacity = parseInt(elStyle.getPropertyValue('opacity'), 10);
On 2016/04/29 at 17:59:13, dpapad wrote:
> Nit: Instead of comparing opacity to 0 twice, how about the following
> 
> var disabled = parseInt(elStyle.getPropertyValue('opacity'), 10) == 0;
> this.textbox_.setAttribute('aria-hidden', disabled);
> this.textbox_.disabled = disabled;

Done.

[tkent, 2016-05-02 08:05:21]

The CQ bit was checked by tkent@chromium.org

[tkent, 2016-05-02 08:05:22]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/1917413002/#ps40001
(title: "|disabled| variable")

[commit-bot: I haz the power, 2016-05-02 08:05:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1917413002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1917413002/40001

[commit-bot: I haz the power, 2016-05-02 08:18:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-02 08:20:03]

Description was changed from

==========
print_preview: Workaround of focus-scroll issue.

* Make margin-control-textbox elements unfocusable if not in custom margin mode.

* Reset the scroll position of preview-area when margin-control-textbox is
  focused.

BUG=601341
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
print_preview: Workaround of focus-scroll issue.

* Make margin-control-textbox elements unfocusable if not in custom margin mode.

* Reset the scroll position of preview-area when margin-control-textbox is
  focused.

BUG=601341
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/dedbae45b850e09e8de724bc678cbec27a7ed6c8
Cr-Commit-Position: refs/heads/master@{#390902}
==========

[commit-bot: I haz the power, 2016-05-02 08:20:04]

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