Bugzilla – Full Text Bug Listing
| Summary: | [Win32] Embedded Browser does not forward '<' character to client code | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Thomas Schindl <tom.schindl> | ||||
| Component: | SWT | Assignee: | Thomas Schindl <tom.schindl> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | daniel_megert, jhelming, lufimtse, niraj.modi | ||||
| Version: | 4.8 | Keywords: | triaged | ||||
| Target Milestone: | --- | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 10 | ||||||
| See Also: |
https://git.eclipse.org/r/114484 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b8601fd3fd49a926d02f70b729a3752acbecae21 |
||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Thomas Schindl
New Gerrit change created: https://git.eclipse.org/r/114484 No objections to patch from gtk side. If someone from windows side could verify, that'd be great. (In reply to Eclipse Genie from comment #1) > New Gerrit change created: https://git.eclipse.org/r/114484 Code changes look fine: - Suggest we add both forward '<' and backward '>' characters in the 'KeyTable' If handy, please share a code snippet for this use-case for verification. import org.eclipse.swt.SWT;
import org.eclipse.swt.browser.Browser;
import org.eclipse.swt.layout.FillLayout;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;
public class Test {
public static void main(String[] args) {
Display d = new Display();
d.addFilter(SWT.KeyDown, e -> {
System.err.println(e.character);
});
Shell s = new Shell();
s.setLayout(new FillLayout());
Browser b = new Browser(s, SWT.NONE);
b.setUrl("http://www.google.at");
s.open();
while( ! s.isDisposed() ) {
if( ! d.readAndDispatch() ) {
d.sleep();
}
}
d.dispose();
}
}
and "NO" i don't think > needs to be added because this > is created with a modifier key. Btw, tested with snippet. The issue doesn't occur on Gtk/Webkit2. Dom is handled differently so the patch only affects win/cocoa. I guess it only affects win32 at least on OS-X the '<' '>' are forwarded already today although what is interesting is the the key-code for '<' on cocoa is "188" who is mapped to ';'! (In reply to Thomas Schindl from comment #4) Hi Thomas, Tested with the snippet shared in comment 4, I am not seeing any behavior difference with and without the patch as tested on Win7 or is it Win10 only issue. Have few related queries: 1. To press '<' Shift is required, so how exactly you type CTRL+< ? Please share more detailed steps to try out ? 2. Seeing the KeyCode list on below site: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode On above page the '<' key is mapped to int value 60 and where-as your patch has mapped '<' to 226, please clarify on this. (In reply to Thomas Schindl from comment #7) > I guess it only affects win32 at least on OS-X the '<' '>' are forwarded > already today although what is interesting is the the key-code for '<' on > cocoa is "188" who is mapped to ';'! 3. WebBrowser.java is a common class for all platforms, and if '<' has different value mapping for different platforms, it might be a problem. We should try and put this character key mapping to platform specific files. e.g. for Windows in IE.java Could it be keyboard layout related, we are using a german layout you probably an english one. I can reproduce the Bug on Windows (it does not occur on Linux) and confirm that the fix works for me. It would be really great to get this into Photon! (In reply to Jonas Helming from comment #10) > I can reproduce the Bug on Windows (it does not occur on Linux) and confirm > that the fix works for me. It would be really great to get this into Photon! What about adding '>' in addition to '<' ? Is '>' affected by the issue or only '<'? (In reply to Leo Ufimtsev from comment #11) > (In reply to Jonas Helming from comment #10) > > I can reproduce the Bug on Windows (it does not occur on Linux) and confirm > > that the fix works for me. It would be really great to get this into Photon! > > What about adding '>' in addition to '<' ? > > Is '>' affected by the issue or only '<'? See my comment 5 Dani, can you confirm this problem your windows as well? (In reply to Thomas Schindl from comment #5) > and "NO" i don't think > needs to be added because this > is created with a > modifier key. What's the reasoning here? for me both < > can only be typed if I hold the 'shift' moddifier and press . , (In reply to Leo Ufimtsev from comment #14) > (In reply to Thomas Schindl from comment #5) > > and "NO" i don't think > needs to be added because this > is created with a > > modifier key. > > What's the reasoning here? > for me both < > can only be typed if I hold the 'shift' moddifier and press > . , Btw, if no objections, I intend to merge the patch. But would be beneficial to understand why not also add '>'. (In reply to Thomas Schindl from comment #4) > import org.eclipse.swt.SWT; > import org.eclipse.swt.browser.Browser; > import org.eclipse.swt.layout.FillLayout; > import org.eclipse.swt.widgets.Display; > import org.eclipse.swt.widgets.Shell; > > public class Test { > > public static void main(String[] args) { > Display d = new Display(); > d.addFilter(SWT.KeyDown, e -> { > System.err.println(e.character); > }); > Shell s = new Shell(); > s.setLayout(new FillLayout()); > > Browser b = new Browser(s, SWT.NONE); > b.setUrl("http://www.google.at"); > > s.open(); > while( ! s.isDisposed() ) { > if( ! d.readAndDispatch() ) { > d.sleep(); > } > } > d.dispose(); > } > > } So with this snippet, is it suppose to demonstrate that Ctrl+< is not being filtered properly? I.e, pressing Ctrl+< inside browser doesn't have desired effect? (It's usually good to list detailed steps to reproduce, detailed expected behavior and actual behavior with snippet tests to avoid ambiguity. The context is that we're using different OS's (win/cocoa/linux) and different keyboard layouts (German, English) (I use Colemak keyboard layout for example). It's easy for tester to assume wrong things if no detailed steps are given. (In reply to Thomas Schindl from comment #13) > Dani, can you confirm this problem your windows as well? No problem here. I have a Swiss German keyboard where the '<' key can just be pressed to enter '<' (no Shift or other modifier required). And hence Ctrl+< works just fine even when the Internal Browser is active (tested with Open Type command). (In reply to Leo Ufimtsev from comment #14) > (In reply to Thomas Schindl from comment #5) > > and "NO" i don't think > needs to be added because this > is created with a > > modifier key. > > What's the reasoning here? > for me both < > can only be typed if I hold the 'shift' moddifier and press > . , The reasoning is that there exist many different keyboard (layouts) ;-). Dani, so if you run the snippet does it really print "<" on the console? We don't talk about the fact that < is shown in the Browser itself, the problem is that the browser does not forward the key-event to SWT. (In reply to Dani Megert from comment #17) > (In reply to Thomas Schindl from comment #13) > > > > What's the reasoning here? > > for me both < > can only be typed if I hold the 'shift' moddifier and press > > . , > > The reasoning is that there exist many different keyboard (layouts) ;-). I see. So this is specific to German Layout? What I'm interested in is how does this fix relate in the context of all users with many different layouts, not just those with a specific layout. Can you elaborate? - How does this affect regular qwerty users, - How does this affect German/Swiss users, - Detailed steps to reproduce so I can test the functionality (with relevant keyboard layout info) before/after the patch. In short, has this patch any potential to break anything or is all good. I'm probably being overly paranoid ha ha. I'm just curious how things are impacted. Danke schoen. (In reply to Thomas Schindl from comment #18) > Dani, so if you run the snippet does it really print "<" on the console? We > don't talk about the fact that < is shown in the Browser itself, the problem > is that the browser does not forward the key-event to SWT. I did not fiddle with the snippet. I tested comment 0 by changing the key binding for Open Type to Ctrl+<. (In reply to Leo Ufimtsev from comment #19) > Danke schoen. Should be "Danke schön" ;-). The purpose of the snippet is that if you have the focus is on the Browser-Widget the KeyDown-Filter is not triggered. To test: * Start Snippet * Type < * Type > Expectation: * the SWT.KeyDown filter is triggered twice Result (windows with german keyboard layout): * Filter is not triggered at all At least on my german keyboard only the '<' character needs to be added, for me adding '<' is enough to make the above test succeed. (In reply to Thomas Schindl from comment #22) > The purpose of the snippet is that if you have the focus is on the > Browser-Widget the KeyDown-Filter is not triggered. > > To test: > * Start Snippet > * Type < > * Type > > > Expectation: > * the SWT.KeyDown filter is triggered twice > > Result (windows with german keyboard layout): > * Filter is not triggered at all > > At least on my german keyboard only the '<' character needs to be added, for > me adding '<' is enough to make the above test succeed. On my Swiss German keyboard [1] < and > are triggered twice. [1] https://en.wikipedia.org/wiki/QWERTZ#Switzerland_(German,_French,_Italian,_Romansh),_Liechtenstein,_Luxembourg (In reply to Thomas Schindl from comment #22) > The purpose of the snippet is that if you have the focus is on the > Browser-Widget the KeyDown-Filter is not triggered. > > To test: > * Start Snippet > * Type < > * Type > > > Expectation: > * the SWT.KeyDown filter is triggered twice > > Result (windows with german keyboard layout): > * Filter is not triggered at all > > At least on my german keyboard only the '<' character needs to be added, for > me adding '<' is enough to make the above test succeed. (In reply to Dani Megert from comment #23) > (In reply to Thomas Schindl from comment #22) > > The purpose of the snippet is that if you have the focus is on the > > Browser-Widget the KeyDown-Filter is not triggered. > > > > To test: > > * Start Snippet > > * Type < > > * Type > > > > > Expectation: > > * the SWT.KeyDown filter is triggered twice > > > > Result (windows with german keyboard layout): > > * Filter is not triggered at all > > > > At least on my german keyboard only the '<' character needs to be added, for > > me adding '<' is enough to make the above test succeed. > > On my Swiss German keyboard [1] < and > are triggered twice. > > [1] > https://en.wikipedia.org/wiki/QWERTZ#Switzerland_(German,_French,_Italian, > _Romansh),_Liechtenstein,_Luxembourg Ok, so I tested on my linux and Windows box with regular English layout. '< & >' continue to work as before without visible change. I cannot verify the fix for Austrian keyboard because my English keyboard seems to lack the '<>' button that can be found on Austrian keyboards (I'll attach picture) and none of the other buttons on my keyboard map onto the '<>' button. Based on the fact that this patch doesn't seem to break anything, keycode 226 being used for some random greek letter in ascii table (i.e, not useful in English), but '<' is useful in German/Austrian layout, I don't mind merging this patch. I suppose I could experiment with on-screen keyboards, but based on the fact that it doesn't seem to break anything and fix something, I don't mind just merging the patch in. If there's no objections, I'll merge the patch on Monday. Created attachment 273777 [details]
Austrian / german keyboard. My English keyboard doesn't have that button.
import org.eclipse.swt.SWT;
import org.eclipse.swt.browser.Browser;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.Text;
public class Test {
public static void main(String[] args) {
Display d = new Display();
d.addFilter(SWT.KeyDown, e -> {
System.err.println("keyCode: " + e.keyCode + "; stateMask: " + e.stateMask);
});
Shell s = new Shell();
s.setLayout(new GridLayout());
Text t = new Text(s, SWT.BORDER);
t.setLayoutData(new GridData(GridData.FILL_HORIZONTAL));
Browser b = new Browser(s, SWT.NONE);
b.setLayoutData(new GridData(GridData.FILL_BOTH));
b.setUrl("http://www.google.at");
s.open();
while (!s.isDisposed()) {
if (!d.readAndDispatch()) {
d.sleep();
}
}
d.dispose();
}
}
Sorry for the confusion my previous desciption and the sample was invalid. The above one shows the problem:
* Launch
* Put Fokus on SWT-Textfield
* Hit CTRL+A
* Hit CTRL+,
* Hit CTRL+<
* Put Fokus on Browser-Textfield
* Hit CTRL+A
* Hit CTRL+,
* Hit CTRL+<
This yields:
# CTRL+A
keyCode: 262144; stateMask: 0
keyCode: 97; stateMask: 262144
# CTRL+,
keyCode: 262144; stateMask: 0
keyCode: 44; stateMask: 262144
# CTRL+<
keyCode: 262144; stateMask: 0
keyCode: 60; stateMask: 262144
--------------
# CTRL+A
keyCode: 262144; stateMask: 0
keyCode: 0; stateMask: 262144
keyCode: 97; stateMask: 262144
# CTRL+,
keyCode: 262144; stateMask: 0
keyCode: 44; stateMask: 262144
# CTRL+<
keyCode: 262144; stateMask: 0
keyCode: 0; stateMask: 262144
The following things are interesting:
* CTRL+A in Browser leads to an extra event
* CTRL+< in Browser never transports the correct keyCode
With the patch I get (only the browser output):
# CTRL+A
keyCode: 262144; stateMask: 0
keyCode: 0; stateMask: 262144
keyCode: 97; stateMask: 262144
# CTRL+,
keyCode: 262144; stateMask: 0
keyCode: 44; stateMask: 262144
# CTRL+<
keyCode: 262144; stateMask: 0
keyCode: 60; stateMask: 262144
(In reply to Thomas Schindl from comment #26) > public class Test { > ... Hello Thomas, I haven't looked into the technical details of the issue above. Is there a simple way to amend the patch to address your issue also? Is it good to merge the patch and work on the above as a separate issue or does the patch cause the issue above? The patch solves exactly what present here! (In reply to Thomas Schindl from comment #28) > The patch solves exactly what present here! Just to repeat: The expectation is that one gets on CTRL+< ====> keyCode: 60; stateMask: 262144 (In reply to Thomas Schindl from comment #28) > The patch solves exactly what present here! Ok. So we're good to merge? Feel free to +1 the patch if you think it's good and I'll merge in time for photon release. Does the patch resolve all problems or is there still something that's not working? yes Gerrit change https://git.eclipse.org/r/114484 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b8601fd3fd49a926d02f70b729a3752acbecae21 Cool, danke schön for patch! I don't have the keyboard to verify this bug. Could someone confirm that this issue is fixed in today's iBuild? Earliest on Friday because I don't have my Windows laptop with me (In reply to Thomas Schindl from comment #35) > Earliest on Friday because I don't have my Windows laptop with me ok. |