The buttons will be visible both when accessibility is enabled or disabled. I think it's preferable to have all accessibility features enabled by default assuming they don't interfere with non-screen reader users (by performance or otherwise). This is one of those cases.

The problem is that calling <x-screen>.focus() causes selection to jump to the page up element which makes it visible on the screen. I have a patch to fix this.

not entirely sure what you mean, but these buttons should never be visible to the user when a11y (e.g. screen reader) is not active.

The thing is that  website doesn't really know if the screen reader is active or not. Nassh is an extension so there is an API which gives it an indication. But generally websites are meant to have the same features exposed regardless of whether a11y is 'enabled' as there is no way to detect this.

I was trying to achieve this whenever it made sense in hterm. That is, if a11y features would work without interfering with normal operation then we should enable them. I think the buttons fall into that category. They will only show up on-screen if they gain selection but this should generally never happen unless a screen reader is enabled. 

Compare https://www.facebook.com/. If you hit shift+tab a few times on the login page, you will see a bar appear across the top with a menu that says "accessibility help". We were trying to employ the same pattern with these buttons.

when I say "visible", I mean I should never see them. I see them currently. I don't mean the css display setting.

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/e5d48988ad60d346defb28e1ed3dd32fab3ff750

commit e5d48988ad60d346defb28e1ed3dd32fab3ff750
Author: Raymes Khoury <raymes@chromium.org>
Date: Thu Aug 02 06:33:19 2018

hterm: Ensure the Page Up button isn't shown when the ScrollPort is focussed

Currently when the ScrollPort is focussed (e.g. when pasting data from
the clipboard) it will cause the PageUp button to be shown. This is
because selection is set to the start of the <x-screen> element. Instead
we set selection to the caret position, or if the caret is offscreen to
the last row in the terminal. This also improves things from an
accessibility standpoint.

Bug:  863668 
Change-Id: Id87335b63a4e799e3c2ace4012cb6c3b7a97e707
Reviewed-on: https://chromium-review.googlesource.com/1137965
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Tested-by: Raymes Khoury <raymes@chromium.org>

[modify] https://crrev.com/e5d48988ad60d346defb28e1ed3dd32fab3ff750/hterm/js/hterm_terminal.js
[modify] https://crrev.com/e5d48988ad60d346defb28e1ed3dd32fab3ff750/hterm/js/hterm_scrollport.js
[modify] https://crrev.com/e5d48988ad60d346defb28e1ed3dd32fab3ff750/hterm/js/hterm_terminal_tests.js
[modify] https://crrev.com/e5d48988ad60d346defb28e1ed3dd32fab3ff750/hterm/js/hterm_scrollport_tests.js

This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Pls apply appropriate OSs label.

once this is approved for merging, the process isn't like other repos.  you'll want to update the pin in the manifest & manifest-internal repos.

see these lines:
  https://chromium.googlesource.com/chromiumos/manifest/+/release-R69-10895.B/full.xml#145
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/release-R69-10895.B/external_full.xml#145

you'll want to pull it up to e5d48988ad60d346defb28e1ed3dd32fab3ff750.  i wouldn't worry about rolling all the other changes too.

I just tested on ToT and it works well

Thanks for the testing update. Approving merge to M69.

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/14a1c1ec6496b748a66de6413fa33aa20df3d412

commit 14a1c1ec6496b748a66de6413fa33aa20df3d412
Author: Raymes Khoury <raymes@chromium.org>
Date: Wed Aug 08 02:53:42 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/manifest/+/84e712cd03341028f56ee08edfdc7cdaac317761

commit 84e712cd03341028f56ee08edfdc7cdaac317761
Author: Raymes Khoury <raymes@chromium.org>
Date: Wed Aug 08 02:53:42 2018

Update libapps commit in full.xml to merge hterm changes to M69

This updates the commit to
https://chromium.googlesource.com/apps/libapps/+/e5d48988ad60d346defb28e1ed3dd32fab3ff750

Bug:  863668 
Change-Id: Idcf022b639f43bc97b3bfde5d7ca6031bfd73132
Reviewed-on: https://chromium-review.googlesource.com/1166279
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Commit-Queue: Raymes Khoury <raymes@chromium.org>
Tested-by: Raymes Khoury <raymes@chromium.org>

[modify] https://crrev.com/84e712cd03341028f56ee08edfdc7cdaac317761/full.xml

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot