Update PDF accessibility when zoom level changes

Update PDF accessibility when zoom level changes

There are three pieces needed for this to work:

1. Encode the page scale and offset using a transformation matrix
at the root of the PDF document, rather than by taking it into
account when computing each object's bounding box. (This wasn't
possible when PDF support first landed, but it's possible now.)

2. Update the root of the PDF tree with a new matrix whenever
the zoom level changes - this required a new public interface.

3. Minor fix to the GetBoundsForRange function in automation
internal bindings - it was converting from local to global
coordinates before walking character offsets, but this doesn't
work when there's a scale factor. Switch it to adjust for
character offsets first and then convert from local to global
last, applying matrix transformations like scale factors.

Tested manually by navigating with ChromeVox or VoiceOver
to some text in the PDF, then press Ctrl+Plus/Minus to zoom,
navigate forward/back to refresh the bounding rect and confirm
that it's updated to reflect the new bounds of the text.

BUG=649850
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[dmazzoni, 2016-09-23 22:34:45]

Description was changed from

==========
Update PDF accessibility when zoom level changes

BUG=649850
==========

to

==========
Update PDF accessibility when zoom level changes

BUG=649850
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2016-09-26 17:39:44]

Description was changed from

==========
Update PDF accessibility when zoom level changes

BUG=649850
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update PDF accessibility when zoom level changes

There are three pieces needed for this to work:

1. Encode the page scale and offset using a transformation matrix
at the root of the PDF document, rather than by taking it into
account when computing each object's bounding box. (This wasn't
possible when PDF support first landed, but it's possible now.)

2. Update the root of the PDF tree with a new matrix whenever
the zoom level changes - this required a new public interface.

3. Minor fix to the GetBoundsForRange function in automation
internal bindings - it was converting from local to global
coordinates before walking character offsets, but this doesn't
work when there's a scale factor. Switch it to adjust for
character offsets first and then convert from local to global
last, applying matrix transformations like scale factors.

Tested manually by navigating with ChromeVox or VoiceOver
to some text in the PDF, then press Ctrl+Plus/Minus to zoom,
navigate forward/back to refresh the bounding rect and confirm
that it's updated to reflect the new bounds of the text.

BUG=649850
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2016-09-26 17:39:57]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, raymes@chromium.org

[dmazzoni, 2016-09-26 17:39:58]

raymes: pdf/
aboxhall: accessibility

[aboxhall, 2016-09-26 18:07:45]

lgtm for accessibility/

[raymes, 2016-09-27 05:05:31]

I defer to aboxhall for 	components/pdf/renderer/pdf_accessibility_tree.h 

otherwise lgtm

https://codereview.chromium.org/2371483002/diff/20001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2371483002/diff/20001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:612: void
OutOfProcessInstance::LoadAccessibility() {
We should make sure these functions are in the same order as the header if
possible

https://codereview.chromium.org/2371483002/diff/20001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.h (right):

https://codereview.chromium.org/2371483002/diff/20001/pdf/out_of_process_inst...
pdf/out_of_process_instance.h:185: // Send the accessibility information about
the current viewport.
nit: fill 80 chars

[dmazzoni, 2016-09-27 16:19:33]

https://codereview.chromium.org/2371483002/diff/20001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/2371483002/diff/20001/pdf/out_of_process_inst...
pdf/out_of_process_instance.cc:612: void
OutOfProcessInstance::LoadAccessibility() {
On 2016/09/27 05:05:31, raymes wrote:
> We should make sure these functions are in the same order as the header if
> possible

I rearranged these three to match. The rest of the file isn't
as consistent so I don't want to do too much in this change.

If you'd like I could follow up with a change to rearrange
the whole file.

https://codereview.chromium.org/2371483002/diff/20001/pdf/out_of_process_inst...
File pdf/out_of_process_instance.h (right):

https://codereview.chromium.org/2371483002/diff/20001/pdf/out_of_process_inst...
pdf/out_of_process_instance.h:185: // Send the accessibility information about
the current viewport.
On 2016/09/27 at 05:05:31, raymes wrote:
> nit: fill 80 chars

Done

[dmazzoni, 2016-09-27 16:19:36]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-09-27 16:20:05]

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, 2016-09-27 16:29:20]

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

[commit-bot: I haz the power, 2016-09-27 16:29:22]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2016-09-27 17:06:48]

dmazzoni@chromium.org changed reviewers:
+ jam@chromium.org

[dmazzoni, 2016-09-27 17:06:48]

+jam for content/public

[jam, 2016-09-27 19:20:16]

why does content/public have to have special casing for pdf?

[dmazzoni, 2016-09-27 20:44:30]

On 2016/09/27 19:20:16, jam wrote:
> why does content/public have to have special casing for pdf?

The PDF code is in chrome/renderer but the rest of accessibility is in
content and we never previously had a need to expose
accessibility APIs to chrome/renderer before.

To make the contents of the PDF plug-in accessible, we need to
hook it up to the embedding page's accessibility tree at the
point of the <embed> element.

The code added is pretty generic, it wouldn't be hard to abstract it
to be a general feature allowing you to augment a node in the DOM
with a foreign accessibility tree - I just don't have any planned
use-cases other than PDF.

It comes down to a philosophy of how to design interfaces: is it
worse to "leak" the concept of PDF plug-in support into content, or is
it worse to design a generic interface when there aren't any plans
to use it for more than one purpose.

Happy to push it towards more generic instead, up to you.

[dmazzoni, 2016-09-29 19:33:02]

PTAL.

I renamed all PDF APIs in content/renderer to
plugin instead, and added a comment indicating that
they currently only work for a document with a single
EMBED element (which is all we need for PDF support).

[dmazzoni, 2016-09-29 19:34:01]

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

[commit-bot: I haz the power, 2016-09-29 19:34:25]

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

[jam, 2016-09-29 19:54:05]

lgtm

[commit-bot: I haz the power, 2016-09-29 20:18:50]

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

[commit-bot: I haz the power, 2016-09-29 20:18:51]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-09-29 20:26:09]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-09-29 20:26:10]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/2371483002/#ps40001
(title: "Renamed PDF -> Plugin in content/renderer/")

[commit-bot: I haz the power, 2016-09-29 20:26:35]

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, 2016-09-29 20:33:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-29 20:36:06]

Description was changed from

==========
Update PDF accessibility when zoom level changes

There are three pieces needed for this to work:

1. Encode the page scale and offset using a transformation matrix
at the root of the PDF document, rather than by taking it into
account when computing each object's bounding box. (This wasn't
possible when PDF support first landed, but it's possible now.)

2. Update the root of the PDF tree with a new matrix whenever
the zoom level changes - this required a new public interface.

3. Minor fix to the GetBoundsForRange function in automation
internal bindings - it was converting from local to global
coordinates before walking character offsets, but this doesn't
work when there's a scale factor. Switch it to adjust for
character offsets first and then convert from local to global
last, applying matrix transformations like scale factors.

Tested manually by navigating with ChromeVox or VoiceOver
to some text in the PDF, then press Ctrl+Plus/Minus to zoom,
navigate forward/back to refresh the bounding rect and confirm
that it's updated to reflect the new bounds of the text.

BUG=649850
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update PDF accessibility when zoom level changes

There are three pieces needed for this to work:

1. Encode the page scale and offset using a transformation matrix
at the root of the PDF document, rather than by taking it into
account when computing each object's bounding box. (This wasn't
possible when PDF support first landed, but it's possible now.)

2. Update the root of the PDF tree with a new matrix whenever
the zoom level changes - this required a new public interface.

3. Minor fix to the GetBoundsForRange function in automation
internal bindings - it was converting from local to global
coordinates before walking character offsets, but this doesn't
work when there's a scale factor. Switch it to adjust for
character offsets first and then convert from local to global
last, applying matrix transformations like scale factors.

Tested manually by navigating with ChromeVox or VoiceOver
to some text in the PDF, then press Ctrl+Plus/Minus to zoom,
navigate forward/back to refresh the bounding rect and confirm
that it's updated to reflect the new bounds of the text.

BUG=649850
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-09-29 20:36:07]

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