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

HTML5 Improvements #69

Closed
totaam opened this issue Apr 10, 2019 · 34 comments
Closed

HTML5 Improvements #69

totaam opened this issue Apr 10, 2019 · 34 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Apr 10, 2019

Issue migrated from trac ticket # 2269

component: html5 | priority: major | resolution: worksforme

2019-04-10 11:42:49: mjharkin created the issue


I've been working on improving the HTML5 client for our own needs, namely:

  • minimally invasive draggable menu (to replace top bar)
  • window selection menu
  • start menu
  • small ui changes (dark background, white window headers)

It's functional but rough around the edges. Not sure if you're interested in taking it on? I can tidy it up a bit more if you are.

There's a lot of file changes here so I'd recommend copying from head at:
[https://github.com/mjharkin/Xpra/tree/dev]

@totaam
Copy link
Collaborator Author

totaam commented Apr 10, 2019

2019-04-10 11:43:29: mjharkin uploaded file xpra_menu.png (32.0 KiB)

Floating Menu
xpra_menu.png

@totaam
Copy link
Collaborator Author

totaam commented Apr 10, 2019

2019-04-10 11:43:40: mjharkin uploaded file open_windows_menu.png (8.2 KiB)

open windows menu
open_windows_menu.png

@totaam
Copy link
Collaborator Author

totaam commented Apr 10, 2019

I would totally take all of the above if you can submit it.
It might be a lot easier to merge before the refactoring and move to typescript.

@totaam
Copy link
Collaborator Author

totaam commented Apr 10, 2019

2019-04-10 12:45:41: mjharkin commented


Great, I'll tidy up and make a few more changes and hopefully submit a patch sometime next week.

Do you want to keep the top bar as an option alongside these changes or can I replace it?

@totaam
Copy link
Collaborator Author

totaam commented Apr 10, 2019

Great, I'll tidy up and make a few more changes and hopefully submit a patch sometime next week.

Excellent, thanks!

Do you want to keep the top bar as an option alongside these changes or can I replace it?

I'm not sure yet.
I was thinking of adding a few more items in the top bar, but maybe this won't be needed. Sorry, I don't know at this point.

@totaam
Copy link
Collaborator Author

totaam commented Apr 10, 2019

I was thinking of adding a few more items in the top bar, but maybe this won't be needed.

Sorry, I don't know at this point.

No problem, I'll keep them both as mutex options.

@totaam
Copy link
Collaborator Author

totaam commented Apr 16, 2019

2019-04-16 15:00:51: mjharkin uploaded file html5_pr.zip (1345.0 KiB)

@totaam
Copy link
Collaborator Author

totaam commented Apr 16, 2019

2019-04-16 15:10:31: mjharkin commented


There quite a lot here, I'm hoping you'll accept it all.
The git diff is at mjharkin/Xpra@7d5a4f0
I'll try and list the changes:

  • connect.html hitting enter on password field submits form
  • double click on window title bar toggles maximize
  • local minimize windows (not sending to server yet)
  • fix for mouse events happening on windows under other windows title bar
  • screen background now dark blue with white windows
  • fix for background in fullscreen
  • open url directly and fallback to notification if blocked by popup-blocker
  • es6-shim updated for IE11 compat (more log cats) in connect.html
  • Danish browser language mapping ("da" mapped to "dk")
  • workaround for Java JIDE popups forced to "NORMAL" window

float menu bar:

  • draggable and autohides
  • main menu with basic functions (taken from top_bar) and start menu
  • open window menu
    • holding ctrl over menu allows you to focus on hover
    • otherwise click to focus
    • minimize, maximize, close from menu
  • support tray icons

I think the easiest way to submit this was zipping the html5 folder (html5_pr.zip​).
the code was branched from 22434

@totaam
Copy link
Collaborator Author

totaam commented Apr 16, 2019

2019-04-16 20:54:07: mjharkin commented


Sorry, already one quick patch needed on top of the zip, forgot to test taskbar mouse actions.

mjharkin/Xpra@0dcea2f

@totaam
Copy link
Collaborator Author

totaam commented Apr 17, 2019

I've started merging some of this stuff (ignoring quite a bit of whitespace or line swaps helped):

  • r22438 es6-shim update for IE11
  • r22439 maximize on header double click
  • r22440 convert favicon from string to Uint8Array if needed - I assume this is needed for IE or something?
  • r22442 open URL: try to use a popup, only show the notification when that fails

I haven't merged the submit-on-enter-in-password field because, that already works in 2.5

As for the rest, something isn't working.
Can you rebase and submit more piecemeal patches? Splitting up the different changes, ie:

  • css changes
  • floating menu (minimal bits)
  • minimize, etc
  • start-command xdg menu (and constify 30 somewhere)
  • JidePopup workaround

@totaam
Copy link
Collaborator Author

totaam commented Apr 17, 2019

2019-04-17 12:49:38: mjharkin commented


Replying to [comment:7 Antoine Martin]:

I've started merging some of this stuff (ignoring quite a bit of whitespace or line swaps helped):
That's weird, I thought I cleaned up any whitespace at least in the git diff, submitted from a Linux box so maybe it's a line endings issue.

  • r22438 es6-shim update for IE11
  • r22439 maximize on header double click
  • r22440 convert favicon from string to Uint8Array if needed - I assume this is needed for IE or something?
    Yeah think this was an IE issue
  • r22442 open URL: try to use a popup, only show the notification when that fails

I haven't merged the submit-on-enter-in-password field because, that already works in 2.5

As for the rest, something isn't working.
Strange, definitely working on head at​https://github.com/mjharkin/Xpra/tree/dev
Can you rebase and submit more piecemeal patches? Splitting up the different changes, ie:

  • css changes
  • floating menu (minimal bits)
  • minimize, etc
  • start-command xdg menu (and constify 30 somewhere)
  • JidePopup workaround

Yeah I'll split it up as best i can and resubmit, probably towards the end of next week due to other commitments.

@totaam
Copy link
Collaborator Author

totaam commented Apr 18, 2019

2019-04-18 07:40:08: mjharkin uploaded file patches.zip (4.9 KiB)

small patches

@totaam
Copy link
Collaborator Author

totaam commented Apr 18, 2019

2019-04-18 07:45:58: mjharkin commented


@antoine Martin
Can you try these patches (attached) for the small stuff and then I'll rebase again for the floating menu changes. Patches are from git so hopefully they should work.
1 New window theme
2 Jide popup fixes
3 Fix for no mouse events on windows that aren't in focus
4 Force current window focus on server
5 Increase hello timeout as this happens alot with proxy server
6 Danish language fixes (double entry in keycodes not sure how to properly handle this)
7 Catch incorrect window resize event

@totaam
Copy link
Collaborator Author

totaam commented Apr 18, 2019

Updates:

  • 1: r22459 new window theme + move background image to css, I also moved the border definition to css rather than adding the style in-line in javascript (new class=border)
  • 2: not applied the "Jide popup fixes" yet, can you give me a sample application to reproduce? this looks like a symptom of a modal bug of some sort - maybe we can fix that more reliably without hardcoding a specific window title?
  • 3: applied the window_set_focus part only: the other part of the patch would prevent pointer tracking on a window which doesn't have focus, it would be better to track the window-move event and toggle a flag during that time
  • 4: this was added in r8730, there must have been a reason for the dont call set focus unless the focus has actually changed - how do I reproduce the problem? Setting the focus unnecessarily is not much of a problem since clicks are sparse events, but I would rather fix the root cause if we can find it.
  • 5 merged in r22461. Do you really need that long? I would have thought that users would have already given up after 10 seconds.
  • 6 merge using a lookup table in r22462.

PS: your patches were in UTF-16LE, not the easiest format to deal with. I've converted them using:

iconv -f UTF-16LE -t UTF-8 2.patch > 2-utf8.patch

@totaam
Copy link
Collaborator Author

totaam commented Apr 18, 2019

  • 1: r22459 new window theme + move background image to css, I also moved the border definition to css rather than adding the style in-line in javascript (new class=border)

Great, thanks.

  • 2: not applied the "Jide popup fixes" yet, can you give me a sample application to reproduce? this looks like a symptom of a modal bug of some sort - maybe we can fix that more reliably without hardcoding a specific window title?

I'll try and get something togther next week

  • 3: applied the window_set_focus part only: the other part of the patch would prevent pointer tracking on a window which doesn't have focus, it would be better to track the window-move event and toggle a flag during that time

Sorry didn't realize mouse events are allowed on unfocused windows. I'll try and get something that's specific to window titlebars for next week

  • 4: this was added in r8730, there must have been a reason for the dont call set focus unless the focus has actually changed - how do I reproduce the problem? Setting the focus unnecessarily is not much of a problem since clicks are sparse events, but I would rather fix the root cause if we can find it.

It's a java windows that is shown, removed and shown again very quickly. I'll try and get a test case together for it.

  • 5 merged in r22461. Do you really need that long? I would have thought that users would have already given up after 10 seconds.

Probably not but my proxy spawned servers take quite a while, 30secs would probably be ok.

  • 6 merge using a lookup table in r22462.
    Thanks. Any idea on the other part of it? I just want Oslash but Ooblique is being sent. I just reordered to send Oslash instead.

PS: your patches were in UTF-16LE, not the easiest format to deal with. I've converted them using:

iconv -f UTF-16LE -t UTF-8 2.patch > 2-utf8.patch

I'll try and diff as UTF-8 or convert before uploading next time

@totaam
Copy link
Collaborator Author

totaam commented Apr 18, 2019

Sorry didn't realize mouse events are allowed on unfocused windows.

Try r22463. It should be better already.

.. 30secs would probably be ok.

Reduced to 30s in r22464.

Any idea on the other part of it?

Oh right, forgot about that one. Done: r22465.
(I wished there was a better way... keyboard mapping is always hit or miss, mostly miss)

@totaam
Copy link
Collaborator Author

totaam commented Apr 25, 2019

2019-04-25 09:33:06: mjharkin uploaded file float_menu_changes.zip (1345.0 KiB)

@totaam
Copy link
Collaborator Author

totaam commented Apr 25, 2019

2019-04-25 09:42:42: mjharkin commented


@antoine Martin
Can you try and overlay float_menu_changes.zip on the html5 folder.
(Don't think I can create a patch with binary files)

This is rebased from r22546 and should contain "the rest", float menu, start menu and minimize. excluding the patches previously submitted.
Github diff is here:
mjharkin/Xpra@5b8faf4

@totaam
Copy link
Collaborator Author

totaam commented Apr 25, 2019

2019-04-25 16:38:51: antoine uploaded file floating-menu.patch (42.4 KiB)

work in progress patch

@totaam
Copy link
Collaborator Author

totaam commented Apr 25, 2019

I like this a lot, it works beautifully so I think I will drop the top bar.

Some questions / notes:

  • fullscreen doesn't work for me
  • the handle is a little bit small and hard to grab
  • I can lose the menu if I resize the browser to be too small (menu handle is off-screen)
  • I don't like code like this: this.parentElement.parentElement.parentElement.parentElement.parentElement.parentElement.className="-hide";

Here's an updated work in progress patch with:

  • fixes for svg icons, icon types (you want this server-side fix too: r22547)
  • refactoring of duplicated code (mostly to do with icons)

@totaam
Copy link
Collaborator Author

totaam commented Apr 25, 2019

2019-04-25 19:09:53: mjharkin commented


These changes are relatively small so just to be quick I'll point you to the github diffs:

I like this a lot, it works beautifully so I think I will drop the top bar.

Great, yeah I don't think the top bar is needed after this.

  • fullscreen doesn't work for me

Damn, was a little eager with the clean up, fix is here:
mjharkin/Xpra@6c5517d

  • the handle is a little bit small and hard to grab

Increased to 20px and a little rework here:
mjharkin/Xpra@fe6b046

  • I can lose the menu if I resize the browser to be too small (menu handle is off-screen)

Constrained here:
mjharkin/Xpra@ccc8037

  • I don't like code like this: this.parentElement.parentElement.parentElement.parentElement.parentElement.parentElement.className="-hide";

Agreed, a super grandparent!, changed here:
mjharkin/Xpra@ae69e1f

@totaam
Copy link
Collaborator Author

totaam commented Apr 29, 2019

Updates:

  • merged floating menu in r22549
  • whitespace cleanup in r22550
  • add autohide option for floating menu: r22551
  • don't click through the floating menu when dragging it (partial solution): r22552
  • removed old top bar: r22553

Things left to fix:

  • stop all clicks on the floating menu from propagating to the window that may sit under it (similar to Stop event propagation from jquery Draggable stop to onclick?)
  • the new containment stuff prevents us from moving the floating menu off-screen, but it doesn't keep it within the screen when we shrink the browser window (you have to enlarge the browser window again to find it)

@totaam
Copy link
Collaborator Author

totaam commented Apr 29, 2019

With r22556, the tray shows up again, but clicking on it doesn't do anything.
And when "autohide" is enabled, the tray icon cannot be clicked on as the line wraps outside the floating menu.

r22557 fixes fullscreeen (was using the top-bar element, now removed).

@totaam
Copy link
Collaborator Author

totaam commented Apr 30, 2019

The tray location was not getting synced, fixed in r22558.
They needed a little bit more space to prevent wrapping: r22559.
(I profoundly dislike adding hard-coded values like this.. but couldn't find a better fix)

@totaam
Copy link
Collaborator Author

totaam commented Apr 30, 2019

2019-04-30 10:47:08: mjharkin commented


@antoine

I tried to do some testing on this but it looks like the tray is broken server side in the latest centos beta and possibly also head.

Traceback (most recent call last):
  File "/usr/lib64/python2.7/site-packages/xpra/x11/gtk_x11/tray.py", line 202, in do_xpra_client_message_event
    window = x11_foreign_new(xid)
  File "/usr/lib64/python2.7/site-packages/xpra/gtk_common/gtk_util.py", line 484, in x11_foreign_new
    return gdk.window_foreign_new_for_display(xid)
TypeError: window_foreign_new_for_display() argument 1 must be gtk.gdk.Display, not int

@totaam
Copy link
Collaborator Author

totaam commented Apr 30, 2019

TypeError: window_foreign_new_for_display() argument 1 must be gtk.gdk.Display, not int

Oops, sorry, try r22567.

@totaam
Copy link
Collaborator Author

totaam commented May 2, 2019

2019-05-02 08:21:46: mjharkin commented


Tray and autohide are still a bit messy I'll come back to this, but here's a potential fix for mouse events on underlying windows:
mjharkin/Xpra@400348d

@totaam
Copy link
Collaborator Author

totaam commented May 2, 2019

here's a potential fix for mouse events on underlying windows:

Hmm, wouldn't that prevent ALL motion events from being reported against the root?
Some applications (ie: xeyes) want to track the pointer even when it isn't confined to their own window.

@totaam
Copy link
Collaborator Author

totaam commented May 2, 2019

2019-05-02 12:06:52: mjharkin commented


Replying to [comment:23 Antoine Martin]:

here's a potential fix for mouse events on underlying windows:
Hmm, wouldn't that prevent ALL motion events from being reported against the root?
Some applications (ie: xeyes) want to track the pointer even when it isn't confined to their own window.
Yeah fair enough, I'm sure there's a better way of doing this and I'm just hacking now but something like this would be close but would need somthing similar for titlebar buttons.

	var targetId = e.target.id;
	if(targetId.startsWith("head")){
		wid = targetId.replace("head", '');
	}else if(targetId.startsWith("title")){
		wid = targetId.replace("title", '');
	}else if(targetId.startsWith("windowicon")){
		wid = targetId.replace("windowicon", '');
	}else if(targetId!="screen" && e.target.localName != "canvas"){
		return;
	}

I'm trying to stop the mouse events hitting the window under another windows titlebar, so I guess the events should be send to the window of that titlebar instead... and then of course avoid window events when using the float menu.

@totaam
Copy link
Collaborator Author

totaam commented Jun 3, 2019

2019-06-03 12:06:16: antoine commented


See also Xpra-org/xpra#2292, Xpra-org/xpra#2312, Xpra-org/xpra#1844.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2019

The help I was hoping to get did not materialize, so I don't have time to take this further... sorry.
This will have to do for 3.0, will follow up in #15

@totaam totaam closed this as completed Jul 25, 2019
@totaam
Copy link
Collaborator Author

totaam commented Sep 13, 2019

2019-09-13 19:04:45: sali.andrew commented


We also ran into the titlebar "click through" issue. A workaround we are using is to add the following in Window.js at the end when the window is created (right after the "// assign some interesting callbacks" comment):

jQuery(this.div).mousedown(function (e) {
			e.stopPropagation();
		})

This solves the issue as the "ghost click" is created from the titlebar click propagating up the parent to "#screen" where an event listener sends it to the server. Using the above the event is not propagated from the window to the screen div and there are no "phantom clicks". Clicks landing outside the div are not affected. Clicks inside the div are not affected.

@totaam
Copy link
Collaborator Author

totaam commented Sep 14, 2019

This solves the issue as the "ghost click" is created from the titlebar click propagating up the parent to "#screen" where an event listener sends it to the server.

Awesome, thank you!
Applied in r23788 with one minor change: this change stops both mousedown and mouseup from propagating, the vast majority of applications won't handle mouseup if they don't first see a mousedown, but some do.

@totaam
Copy link
Collaborator Author

totaam commented Sep 20, 2019

Applied in r23788 with one minor change: this change stops both mousedown and mouseup from propagating, the vast majority of applications won't handle mouseup if they don't first see a mousedown, but some do.

The minor change caused a bug: Xpra-org/xpra#2418, so that part got reverted in r23826.

@totaam totaam transferred this issue from Xpra-org/xpra Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant