Note: There are a few cases of duplicates in user autocompletion which are being worked on.

move the synth mouse move handling from the view manager to the pres shell

RESOLVED FIXED

Status

()

Core
Layout: View Rendering
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 530641 [details] [diff] [review]
patch
Attachment #530641 - Flags: review?(roc)
(Assignee)

Comment 2

6 years ago
This also adds a check for activeness of the document when searching for views that the mouse is over which will be needed when hidden tabs aren't in the hidden view they get from decks.
Comment on attachment 530641 [details] [diff] [review]
patch

Review of attachment 530641 [details] [diff] [review]:
-----------------------------------------------------------------

All that Find...View code is ugly! Can we just use the frame tree instead? Note the code in PresShell::HandleEvent that uses nsXULPopupManager to find the popup containing a point.

::: layout/base/nsPresShell.cpp
@@ +6091,5 @@
> +{
> +  // allow new event to be posted while handling this one only if the
> +  // source of the event is a scroll (to prevent infinite reflow loops)
> +  if (aFromScroll)
> +    mSynthMouseMoveEvent.Forget();

{}

@@ +6465,5 @@
> +    nsPresContext* rootPresContext = mPresContext->GetRootPresContext();
> +    if (rootPresContext) {
> +      PresShell* rootPresShell =
> +        static_cast<PresShell*>(rootPresContext->PresShell());
> +      if (rootPresShell) {

Combine this code with the previous occurrence using a helper method GetRootPresShell()?
(Assignee)

Comment 4

6 years ago
I couldn't figure out how to use Splinter to reply.

(In reply to comment #3)
> All that Find...View code is ugly! Can we just use the frame tree instead?
> Note the code in PresShell::HandleEvent that uses nsXULPopupManager to find
> the popup containing a point.

The only important thing about finding the right view is that we call DispatchSynthMouseMove on the right presshell (see bug 302561 comment 32). So in the future I plan to make this code iterate any popups (both comboboxes and XUL popups) and then iterate over subdocuments to see where to dispatch the event. I didn't want to make this code traverse the entire cross doc frame tree. I just wanted to straight forwardly move this code from the view manager to the pres shell. This moves us in the direction we want to go, doesn't make any code worse, and limits risk.

> ::: layout/base/nsPresShell.cpp
> @@ +6091,5 @@
> > +{
> > +  // allow new event to be posted while handling this one only if the
> > +  // source of the event is a scroll (to prevent infinite reflow loops)
> > +  if (aFromScroll)
> > +    mSynthMouseMoveEvent.Forget();
> 
> {}

Ok.

> @@ +6465,5 @@
> > +    nsPresContext* rootPresContext = mPresContext->GetRootPresContext();
> > +    if (rootPresContext) {
> > +      PresShell* rootPresShell =
> > +        static_cast<PresShell*>(rootPresContext->PresShell());
> > +      if (rootPresShell) {
> 
> Combine this code with the previous occurrence using a helper method
> GetRootPresShell()?

Done.
(Assignee)

Comment 5

6 years ago
Created attachment 531071 [details] [diff] [review]
patch v2
Attachment #530641 - Attachment is obsolete: true
Attachment #530641 - Flags: review?(roc)
Attachment #531071 - Flags: review?(roc)
(In reply to comment #4)
> The only important thing about finding the right view is that we call
> DispatchSynthMouseMove on the right presshell (see bug 302561 comment 32).
> So in the future I plan to make this code iterate any popups (both
> comboboxes and XUL popups) and then iterate over subdocuments to see where
> to dispatch the event.

I think PresShell::HandleEvent already does both of those for you --- almost. The code to locate a popup and redirect events to it is under "if (framePresContext == rootPresContext && frame == FrameManager()->GetRootFrame())". The code to redirect to the correct subdocument is around "// Handle the event in the correct shell."

The one thing that I think is missing is that nsLayoutUtils::GetPopupFrameForEventCoordinates doesn't know about nsComboboxControlFrames because they don't interact with nsXULPopupManager. So something would need to be fixed there.

> I didn't want to make this code traverse the entire
> cross doc frame tree. I just wanted to straight forwardly move this code
> from the view manager to the pres shell. This moves us in the direction we
> want to go, doesn't make any code worse, and limits risk.

Yes, fair enough.
Comment on attachment 531071 [details] [diff] [review]
patch v2

Review of attachment 531071 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531071 - Flags: review?(roc) → review+
(Assignee)

Comment 8

6 years ago
(In reply to comment #6)
> (In reply to comment #4)
> > The only important thing about finding the right view is that we call
> > DispatchSynthMouseMove on the right presshell (see bug 302561 comment 32).
> > So in the future I plan to make this code iterate any popups (both
> > comboboxes and XUL popups) and then iterate over subdocuments to see where
> > to dispatch the event.
> 
> I think PresShell::HandleEvent already does both of those for you ---
> almost. The code to locate a popup and redirect events to it is under "if
> (framePresContext == rootPresContext && frame ==
> FrameManager()->GetRootFrame())". The code to redirect to the correct
> subdocument is around "// Handle the event in the correct shell."

The things that I think are important to happen in the correct presshell are the things in PresShell::DispatchSynthMouseMove, specifically the hover generation and flush on hover generation change. Of course PresShell::HandleEvent on the root document can handle the mouse event, it handles all our mouse events!

> The one thing that I think is missing is that
> nsLayoutUtils::GetPopupFrameForEventCoordinates doesn't know about
> nsComboboxControlFrames because they don't interact with nsXULPopupManager.
> So something would need to be fixed there.

Yes, I didn't make this clear enough in my previous comment.
Ok, sorry.

Hopefully you can share code with HandleEvent, though.
(Assignee)

Comment 10

6 years ago
(In reply to comment #9)
> Hopefully you can share code with HandleEvent, though.

Yes.
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/mozilla-central/rev/916987d88134
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 679356
You need to log in before you can comment on or make changes to this bug.