search

LadybirdBrowser / ancient-history

The Ladybird web browser
BSD 2-Clause "Simplified" License
1.62k stars 102 forks source link

Don't propagate unrecognized button clicks to the web engine #37

Closed diegoiast closed 2 years ago

diegoiast commented 2 years ago

There are a lot of unsupported mouse click events that the engine cannot handle. It such event (0) reaches the web engine - it will assert.

Don't even propagate them - this is the safe way. As of today!

I also added back/forward buttons to the translation.

NOTE that I removed one button related function. I am under assumption that we always need to send a bitmask. I may be wrong... in such case - I will revert part of this PR.

Should fix #27

romainreignier commented 2 years ago

I also wanted to add the support for Back and Forward mouse buttons and though it would be an easy first contribution to Ladybird. But adding the two ifdid not work.

I have tried your version and now, even a simple left click on a link make the browser crash. Actually it is the mouse release event that sends an unhandled 0 value that failed the assert in determine_button() from MouseEvent.cpp.

Anyway, it seems to me that the back and forward events are not yet used in LibWeb to navigate the history.

As a side note, I did not manage to see the value of the button or buttons variables in any function with gdb and QtCreator on Ubuntu 22.04. It is like the variable does not exist while I have configured a Debug build type in CMake. If someone has any idea about this issue?

diegoiast commented 2 years ago

@romainreignier - can you show me a backtrace? I want to know where this crashes. See the new commit (I rebased/rewrote) the patch - does it still crash for you at all?

Regarding the handling of back/forward mouse buttons - yeap, seems like you are right: https://github.com/SerenityOS/serenity/blob/8ff7c52cf43273b9a7e2a63c0c12cf2dd0379d5e/Userland/Libraries/LibWeb/Page/EventHandler.cpp#L344

Feel free to ship me a cool button, so I can fix this properly ;-)

romainreignier commented 2 years ago

With your latest version, I don't have a crash anymore, just no action on Back and Forward buttons press.

(I have deleted my previous message because I had forgotten to apply your patch and was on master branch)

diegoiast commented 2 years ago

Handling back/forward will be done in another PR. I am unsure if I was to capture these buttons on the app - or library level.

diegoiast commented 2 years ago

Rebased