LibWeb: Unbreak (help://) links in the Help app
#25662
Conversation
github-actions
bot
added
the
👀 pr-needs-review
MacDue
force-pushed
the
help_links
branch
2 times, most recently
from
debdd48 to
80bd342
Compare
The switch to using navigables to load links in c7a6d41 and 38461a7 started the breakage of `help://` URLs. The old FrameLoader (now gone) used to just do nothing when presented with a `help://`, navigables (while attempting to follow the spec) crash. Supporting `help://` URLS (in the Help app) does not need the complexity of navigables, so we can just bail out at a reasonable point. Note that 38461a7 removed sending the `page_did_click_link` to the client, without this event the Help app does not know about any link clicks, so can't handle the navigation itself (which is needed for custom schemes).
d0e18b8 revived sending the `page_did_click_link` event, but only for a chrome feature where ctrl is also clicked. For the Help app to work normally (not just when pressing ctrl), we need this event to be sent no matter the modifier. The modifiers are sent as part of the event so chromes can ignore non-control clicks.
|
@trflynn89 Does this look like a good approach? |
| @@ -333,8 +333,7 @@ EventResult EventHandler::handle_mouseup(CSSPixelPoint viewport_position, CSSPix | |||
| JS::NonnullGCPtr<DOM::Document> document = *m_navigable->active_document(); | |||
| auto href = link->href(); | |||
| auto url = document->parse_url(href); | |||
|
|
|||
| if (button == UIEvents::MouseButton::Primary && (modifiers & UIEvents::Mod_PlatformCtrl) != 0) { | |||
| if (button == UIEvents::MouseButton::Primary) { | |||
| m_navigable->page().client().page_did_click_link(url, link->target().to_byte_string(), modifiers); | |||
There was a problem hiding this comment.
What happens now when we click a normal link? IIRC non-ctrl clicks go through the anchor element's activation behavior, so does this cause that navigation to happen twice?
There was a problem hiding this comment.
It does navigate twice, though I don't think for the reason you mention here. The problem is all the chromes still call load_url (when ctrl is not set) within there handler for on_link_click, which has been (partially) dead code for a while now. It's made slightly awkward by some questionable reuse of the on_link_click callback too (it's called by places other than the web content client, which means in some places may still needs the load_url behaviour.
There was a problem hiding this comment.
I've pushed a fix for Ladybird and Browser (can't really do the macOS stuff, since I don't have mac).
There was a problem hiding this comment.
If I were to do the mac bits, what would I do to test I haven't broken anything (or rather, that I fixed something)?
There was a problem hiding this comment.
I think the changes needed are just:
The on_link_click callback should only do anything when the control modifier is set (to match that previously LibWeb only sent this event when control is held).
serenity/Ladybird/AppKit/UI/LadybirdWebView.mm
Lines 616 to 628 in 970c025
This openLink function should direly call loadURL rather than reuse the on_link_click callback.
serenity/Ladybird/AppKit/UI/LadybirdWebView.mm
Lines 1221 to 1224 in 970c025
Then to check if "openLink" works, try a context menu event (like "Open Image"). For the on_link_click callback, just try clicking any link (and check it opens without requesting the page twice).
The `on_link_click` callback has not been called from LibWeb for anything other than control clicks for a while now. So to avoid double loads, now that this event has been restored for all link clicks, this callback should ignore anything other than control clicks. Other (slightly improper) uses of the callback have been updated to use the underlying methods to preserve the correct behaviour.
See commits for details.
Fixes #25288 (note: this did not just affect "see also" links, but any
help://link)