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

Selecting text in a text input deletes text instead #22345

Open
jdm opened this issue Dec 1, 2018 · 23 comments
Open

Selecting text in a text input deletes text instead #22345

jdm opened this issue Dec 1, 2018 · 23 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Dec 1, 2018

./mach run -d 'data:text/html,<input value="hi">'
Focus the text input with the mouse, then use shift+left to select the previous character. Instead the character appears to be deleted. We should start by determining if the code to recognise shift is working, then figure out why the horizontal adjustment code is behaving strangely.

@JRegimbal
Copy link

@JRegimbal JRegimbal commented Dec 5, 2018

I can't seem to replicate this on my computer (08bbf4f on Debian testing). The characters are selected and don't appear to be deleted.

@jdm
Copy link
Member Author

@jdm jdm commented Dec 19, 2018

Maybe it's a macOS-only issue?

@jdm jdm added the P-mac label Dec 19, 2018
@learning
Copy link
Contributor

@learning learning commented Jan 16, 2019

Maybe it's a macOS-only issue?

I think so, I can reproduce this on my mac

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 26, 2019

Yes, I can reproduce this bug in my Mac Pro. I want to participate in this task, can you assign it to me?

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 26, 2019

After adding some println in the src, I think the code can recognize the shift button.

When I use shift+left to select, the logs show that it received these key events.

key: Shift, mods: SHIFT
key: ArrowLeft, mods: SHIFT
key: Character("\u{f702}"), mods: (empty)

But U+F702 is Private Use Area in Unicode, it's weird.
I will try this on my Linux and see what will happen.

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 27, 2019

After testing in my Ubuntu, I didn't see any strange characters.

Instead, I found all the arrow keys, F1-F12 keys, HOME, PgUp, PgDn, End, Delete will emit those strange Unicode, which overwrites the original characters. And those characters are not visible, so it seems you delete the characters.

OK, I have verified my guess. When you use an arrow key, it will insert an invisible characters and overwrite the original one.

content: DOMString("hi", PhantomData), key: ArrowLeft, mods: SHIFT
content: DOMString("hi", PhantomData), key: Character("\u{f702}"), mods: (empty)
content: DOMString("h\u{f702}", PhantomData), key: Character("b"), mods: (empty)
@yuguorui
Copy link

@yuguorui yuguorui commented Jan 27, 2019

With debugging with lldb, I found the strange character event exists in script::dom::document::Document::dispatch_key_event.
But I don't know how to go further to track the source of this strange character.

Could you please give me some advice?

Here are the call stacks when handle_keydown is called.

* thread #35, stop reason = breakpoint 1.1
  * frame #0: 0x0000000101834283 servo`_$LT$script..textinput..TextInput$LT$T$GT$$GT$::handle_keydown::h78439969eeed37a8(self=&0x124a5d800, event=&0x124a9b0e0) at textinput.rs:721
    frame #1: 0x0000000100b3f860 servo`_$LT$script..dom..htmlinputelement..HTMLInputElement$u20$as$u20$script..dom..virtualmethods..VirtualMethods$GT$::handle_event::h08f22329e382989b(self=&0x124a5d600, event=&0x124a9b0e0) at htmlinputelement.rs:1488
    frame #2: 0x0000000102ef40d5 servo`script::dom::event::Event::dispatch::h1863ea39c173cac8(self=&0x124a9b0e0, target=&0x124a5d600, target_override=Option<&script::dom::eventtarget::EventTarget> {
    frame #3: 0x0000000102a86580 servo`script::dom::eventtarget::EventTarget::dispatch_event::he60ea47a7c58a741(self=&0x124a5d600, event=&0x124a9b0e0) at eventtarget.rs:381
    frame #4: 0x0000000102ef459b servo`script::dom::event::Event::fire::h3c12fd8fa83f24e1(self=&0x124a9b0e0, target=&0x124a5d600) at event.rs:241
    frame #5: 0x0000000101b23b9f servo`script::dom::document::Document::dispatch_key_event::h74274ab96eb06ce3(self=&0x124a39e00, keyboard_event=KeyboardEvent {
    frame #6: 0x0000000101cd4b53 servo`script::script_thread::ScriptThread::handle_event::hd132ca5030c16724(self=&0x70000f8da038, pipeline_id=PipelineId {
    frame #7: 0x0000000101cbfc48 servo`script::script_thread::ScriptThread::handle_msg_from_constellation::h94ba7a739f21ee2f(self=&0x70000f8da038, msg=ConstellationControlMsg {
    frame #8: 0x0000000101340b54 servo`script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::h01b656c9ae18175e at script_thread.rs:1295
    frame #9: 0x0000000101341151 servo`script::script_thread::ScriptThread::profile_event::h05f5e07f396d6718(self=&0x70000f8da038, category=DomEvent, pipeline_id=Option<msg::constellation_msg::PipelineId> {
    frame #10: 0x0000000101cbd426 servo`script::script_thread::ScriptThread::handle_msgs::h03678e2785582ce7(self=&0x70000f8da038) at script_thread.rs:1289
    frame #11: 0x0000000101cbadfd servo`script::script_thread::ScriptThread::start::hd830f224512f80a8(self=&0x70000f8da038) at script_thread.rs:1134
    frame #12: 0x000000010133dea5 servo`_$LT$script..script_thread..ScriptThread$u20$as$u20$script_traits..ScriptThreadFactory$GT$::create::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::he8ad85a0f8cd6f5c at script_thread.rs:712
    frame #13: 0x0000000101bf9c3b servo`profile_traits::mem::ProfilerChan::run_with_memory_reporting::hde370a1aacb4213e(self=&0x70000f8da024, f=closure(&0x70000f8da038), reporter_name="script-reporter-(1,1)", channel_for_reporter=Sender<script::script_thread::MainThreadScriptMsg> {
    frame #14: 0x000000010133e49a servo`_$LT$script..script_thread..ScriptThread$u20$as$u20$script_traits..ScriptThreadFactory$GT$::create::_$u7b$$u7b$closure$u7d$$u7d$::hb908c1410c9ef7ef at script_thread.rs:710
    frame #15: 0x0000000100eafd13 servo`std::sys_common::backtrace::__rust_begin_short_backtrace::hed6e3c4172e8d5d5(f=<unavailable>) at backtrace.rs:135
    frame #16: 0x0000000101441213 servo`std::thread::Builder::spawn_unchecked::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::hedaac791183527af at mod.rs:469
    frame #17: 0x000000010316b713 servo`_$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::hb5af1dd620145e16(self=<unavailable>, _args=<unavailable>) at panic.rs:309
    frame #18: 0x00000001012845e3 servo`std::panicking::try::do_call::h01ca1b2cdcae7208(data=&0x70000f8db268) at panicking.rs:297
    frame #19: 0x0000000107754d3f servo`__rust_maybe_catch_panic at lib.rs:92 [opt]
    frame #20: 0x0000000101159f35 servo`std::panicking::try::h0eba457841a02e08(f=<unavailable>) at panicking.rs:276
    frame #21: 0x00000001031a8bc3 servo`std::panic::catch_unwind::h7d93f6a04cc072ee(f=<unavailable>) at panic.rs:388
    frame #22: 0x00000001014409cc servo`std::thread::Builder::spawn_unchecked::_$u7b$$u7b$closure$u7d$$u7d$::hc1be035583625fba at mod.rs:468
    frame #23: 0x0000000101441b14 servo`_$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h2f255016ba8dceb8(self=&0x12402f200, args=<unavailable>) at boxed.rs:734
    frame #24: 0x00000001077542ac servo`std::sys::unix::thread::Thread::new::thread_start::hc1f19dfe8b570103 [inlined] _$LT$alloc..boxed..Box$LT$$LP$dyn$u20$alloc..boxed..FnBox$LT$A$C$$u20$Output$u3d$R$GT$$u20$$u2b$$u20$$u27$a$RP$$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h00b5ffb03ca3ce46 at boxed.rs:744 [opt]
    frame #25: 0x00000001077542a9 servo`std::sys::unix::thread::Thread::new::thread_start::hc1f19dfe8b570103 [inlined] std::sys_common::thread::start_thread::h3189696793c667a2 at thread.rs:14 [opt]
    frame #26: 0x000000010775422e servo`std::sys::unix::thread::Thread::new::thread_start::hc1f19dfe8b570103 at thread.rs:81 [opt]
    frame #27: 0x00007fff7b1af305 libsystem_pthread.dylib`_pthread_body + 126
    frame #28: 0x00007fff7b1b226f libsystem_pthread.dylib`_pthread_start + 70
    frame #29: 0x00007fff7b1ae415 libsystem_pthread.dylib`thread_start + 13
@jdm
Copy link
Member Author

@jdm jdm commented Jan 27, 2019

Key input starts in

fn handle_keyboard_input(
&self,
input: KeyboardInput,
) {
let event = keyboard_event_from_winit(input);
if event.state == KeyState::Down && event.key == Key::Unidentified {
// If pressed and probably printable, we expect a ReceivedCharacter event.
self.last_pressed.set(Some(event));
} else if event.key != Key::Unidentified {
self.last_pressed.set(None);
self.event_queue
.borrow_mut()
.push(WindowEvent::Keyboard(event));
}
}
and https://github.com/servo/servo/blob/master/ports/servo/glutin_app/keyutils.rs, and eventually ends up in
KeyboardEvent(key_event) => {
let document = match { self.documents.borrow().find_document(pipeline_id) } {
Some(document) => document,
None => return warn!("Message sent to closed pipeline {}.", pipeline_id),
};
document.dispatch_key_event(key_event);
},
. I do not believe any other processing happens along the way.

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 27, 2019

Now I think it may be an upstream related problem?

Anyway, I will check the upstream project, glutin.

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 27, 2019

OK, I am sure it is an upstream related issue.

When I press Left Arrow in the demo window, I have found these logs in it.

WindowEvent { window_id: WindowId(Id(140193353970816)), event: KeyboardInput { device_id: DeviceId(DeviceId), input: KeyboardInput { scancode: 123, state: Pressed, virtual_keycode: Some(Left), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } } } }
WindowEvent { window_id: WindowId(Id(140193353970816)), event: ReceivedCharacter('\u{f702}') }
WindowEvent { window_id: WindowId(Id(140193353970816)), event: KeyboardInput { device_id: DeviceId(DeviceId), input: KeyboardInput { scancode: 123, state: Released, virtual_keycode: Some(Left), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } } } }

According to my tests, this bug does not exist with linux.

So what should I do now? Do you need me to ask an issue to help them fix the bug? Or just close our issue?

@jdm
Copy link
Member Author

@jdm jdm commented Jan 27, 2019

Filing an issue in the upstream repository with a simple testcase would be useful. We should leave this issue open until it's fixed, though.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 27, 2019

That being said, the ReceivedCharacter handling is this code, so it still might be our fault.

@jdm jdm removed the E-less easy label Jan 27, 2019
@jdm
Copy link
Member Author

@jdm jdm commented Jan 27, 2019

@mskwon
Copy link

@mskwon mskwon commented Jan 27, 2019

Please note that #22344 is extremely likely to be due to this extra event as well.

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 27, 2019

Yes, I also realized that it was a problem there, but how fast you are!🤣

We can, of course, fix this problem. But I think the point is the different behavior on OS X of the library, I believe this is also a problem.

But no matter what, I am willing to contribute code to Servo, so do I need to fix it in Servo?

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 27, 2019

Please note that #22344 is extremely likely to be due to this extra event as well.

Yes, I think their reasons should be similar.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 27, 2019

We should try to fix it upstream, rather than working around it in Servo.

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 27, 2019

OK, I will file an issue in the upstream repository.

@yuguorui
Copy link

@yuguorui yuguorui commented Jan 27, 2019

@nicoabie
Copy link

@nicoabie nicoabie commented Jul 29, 2020

Upstream issue is closed but this is still happening on macOS 10.15.6

@jdm
Copy link
Member Author

@jdm jdm commented Jul 29, 2020

It's possible that we still have not updated to a recent version of winit that includes the fix.

@atouchet
Copy link
Contributor

@atouchet atouchet commented Jul 29, 2020

That issue looks to have been fixed in winit 0.20 and Servo is still using 0.19.3.

winit update issue: #26394

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.