LibWeb: Prevent a popover crash and implement popovertarget#3285
LibWeb: Prevent a popover crash and implement popovertarget#3285ADKaster merged 3 commits intoLadybirdBrowser:masterfrom
Conversation
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
github-actions
Bot
added
the
conflicts
github-actions
Bot
removed
conflicts
Gingeh
force-pushed
the
popovertarget
branch
2 times, most recently
from
5e9dabe to
6931acb
Compare
| auto const* popover_invoker_element = dynamic_cast<PopoverInvokerElement const*>(node.ptr()); | ||
| GC::Ptr<HTMLElement> popover_element = verify_cast<HTMLElement>(popover_invoker_element->m_popover_target_element.ptr()); |
There was a problem hiding this comment.
If node isn't a PopoverInvokerElement, this is a null dereference.
If we know it is one, then use verify_cast instead of dynamic_cast.
There was a problem hiding this comment.
That function is only ever called on FormAssociatedElements which are also PopoverInvokerElements, so neither dynamic_cast should fail. I don't know how to convince the compiler that a verify_cast is valid, so the best I can do is a VERIFY(popover_invoker_element).
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
github-actions
Bot
removed
the
conflicts
|
FYI I've opened #3354 which update a bunch of the spec comments. Between us we're going to cause conflicts but I personally think it makes sense to get the spec comments up-to-date and then add the adhoc fixes in between after the fact. Up to the maintainers which they want to merge first. |
I agree |
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
github-actions
Bot
removed
the
conflicts
|
I've swapped the two commits in this PR because the "ignore DOM state" change affects the PopoverInvoker implementation |
|
I've added another commit to hide visible popovers when changing the |
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
Using whatwg/html#9457 (with some changes made to catch up with the current spec) to fix a spec bug and a crash when removing a visible popover.
github-actions
Bot
removed
the
conflicts
4 participants
The first commit is based on whatwg/html#9457 and prevents a crash when removing a visible popover.
That spec PR hasn't been merged yet and is currently slightly outdated, but the changes are spread throughout the code so I'm not sure what the best place to add a
FIXME/AD-HOCwould be.The second commit implements
popovertargetbuttons (and includes a change to top layer elements to prevent a crash when hiding and showing a popover in the same frame).This PR also fixes 29 crashing tests and gains at least 1656 new subtest passes.
(I may start a followup PR to update and implement the remaining parts of the popover spec)