Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

machines: scroll noVNC into view #6950

Conversation

Projects
None yet
3 participants
@mareklibra
Copy link
Contributor

commented Jun 15, 2017

The noVNC frame and related components are scrolled into view when
Console subtab is opened.

Fixes: #6939

@mareklibra

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2017

@garrett , can you please try? I think this behavior meets the requirements:

  • do nothing, if noVNC frame is fully visible
  • maximalize its visible part otherwise

Control components, including VM name, are always visible.

@mareklibra mareklibra force-pushed the mareklibra:issue-6939.scrollGraphicsConsoleToViewable branch 2 times, most recently from fc82deb to 31cab20 Jun 15, 2017

@martinpitt
Copy link
Member

left a comment

That works nicely, thanks! Just one question about the ID change (and that should be a separate commit, but that's mostly just bikeshedding).


const encrypt = window.location.protocol === "https:";
const port = consoleDetail.tlsPort || consoleDetail.port;

let params = `?host=${consoleDetail.address}&port=${port}&encrypt=${encrypt}&true_color=1&resize=true&containerId=${encodeURIComponent(uniqueFrameContainerId)}`;
let params = `?host=${consoleDetail.address}&port=${port}&encrypt=${encrypt}&true_color=1&resize=true&containerId=${encodeURIComponent(vmIdPrefix)}`;

This comment has been minimized.

Copy link
@martinpitt

martinpitt Jun 22, 2017

Member

That change is unrelated to the scrolling (I think). Passing a different ID here as URL parameter than the div id seems a bit odd - why is that done?

This comment has been minimized.

Copy link
@mareklibra

mareklibra Jun 23, 2017

Author Contributor

In fact, it's no more element id but prefix to compose id for different elements within vnc.html.
I think we can rename the containerId URL param to containerIdPrefix.

Before this fix, access to just a single element (former containerId) was needed within vnc.html.
But to have scrolling work, I need to adjust multiple of them to achieve maximum visibility of the iframe and related components (like VM name).

Please let me know, I can rename the URL param.

This comment has been minimized.

Copy link
@martinpitt

martinpitt Jun 23, 2017

Member

Ah, thanks for the explanation! Makes sense, don't bother to rename it.

However, there are three test failures on testInlineConsole (check_machines.TestMachines) that time out waiting for the VNC status to be "normal" - in all three screenshots the console still shows "loading". It appears unrelated, but I will retry the tests first, and we should fix them (we might need to wait longer there).

@martinpitt martinpitt added question and removed question labels Jun 22, 2017

@martinpitt

This comment has been minimized.

Copy link
Member

commented Jun 23, 2017

@mareklibra: That flaky test is hopefully fixed by the "Give sub-VM more time to boot in check-machines" commit in https://github.com/martinpitt/cockpit/tree/test-races . I have some other tests to look at before I propose it for merging.

@mareklibra

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2017

@martinpitt , thanks. Seems to be more magic than development :-)

@martinpitt martinpitt referenced this pull request Jun 23, 2017

Closed

Test fixes #7100

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

The tests don't pass here:

Traceback (most recent call last):
  File "/build/cockpit/bots/../test/verify/check-machines", line 272, in testInlineConsole
    b.wait(lambda: b.eval_js("document.documentElement.outerHTML.indexOf('noVNC_status_normal') >= 0"))
@stefwalter
Copy link
Contributor

left a comment

Tests fail, need updating.

@martinpitt

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

@stefwalter: I thought I fixed that in #7100; I'll retry the tests.

@mareklibra mareklibra force-pushed the mareklibra:issue-6939.scrollGraphicsConsoleToViewable branch from 31cab20 to 7cf5bba Jun 28, 2017

machines: scroll noVNC into view
The noVNC frame and related components are scrolled into view when
Console subtab is opened.

Fixes: #6939

@mareklibra mareklibra force-pushed the mareklibra:issue-6939.scrollGraphicsConsoleToViewable branch from 7cf5bba to e01f28c Jul 12, 2017

@mareklibra

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2017

Rebased

@martinpitt
Copy link
Member

left a comment

Looks good now and the tests are happy. Thanks!

@martinpitt martinpitt merged commit b21c0f0 into cockpit-project:master Jul 12, 2017

15 of 16 checks passed

verify/ubuntu-stable 1 tests failed
Details
avocado/fedora-24 Tests passed
Details
container/kubernetes Tests passed
Details
selenium/chrome Tests passed
Details
selenium/firefox Tests passed
Details
semaphoreci The build passed on Semaphore.
Details
verify/centos-7 Tests passed
Details
verify/debian-stable Tests passed
Details
verify/debian-testing Tests passed
Details
verify/fedora-26 Tests passed
Details
verify/fedora-atomic Tests passed
Details
verify/fedora-i386 Tests passed
Details
verify/rhel-7 Tests passed
Details
verify/rhel-7-4 Tests passed
Details
verify/rhel-atomic Tests passed
Details
verify/ubuntu-1604 Tests passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.