Closed Bug 1506508 Opened 6 years ago Closed 5 years ago

Event is not fire at the first click when position:relative is set on :active

Categories

(Core :: DOM: Events, defect, P3)

Product:
Core
Component:
DOM: Events
Version:
65 Branch
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox65 --- wontfix
firefox66 --- ?
firefox67 --- fixed

People

(Reporter: karlcow, Assigned: masayuki)

References

(
URL
)

Details

(Whiteboard: [webcompat])

Attachments

(2 files)

Automated test
2.96 KB, patch
Details | Diff | Splinter Review
Bug 1506508 - Make PresShell::EventHandler::HandleEvent() flush any dirty region before deciding to event target of eMouseDown and eMouseUp
47 bytes, text/x-phabricator-request
Details | Review 
See https://webcompat.com/issues/16238

<section class="position"><a href="https://mozilla.org/" class="btn">Try to click me</a></section>

<section class="noposition"><a href="https://mozilla.org/" class="btn">Try to click me</a></section>


.position .btn:active {
    position: relative;
    top: 1px;
}


Click on the first link and this is not working.
Click a second time and this is working


Maybe related to https://bugzilla.mozilla.org/show_bug.cgi?id=764589

Chrome is firing the event at first click.
Flags: webcompat?
nsLayoutUtils::GetFrameForPoint() called in FindFrameTargetedByInputEvent() returns text frame for eMouseDown, but oddly, it returns block frame for parent <section> element for eMouseUp (even though the point is same as both calls):
https://searchfox.org/mozilla-central/rev/06d5d5ae4396be85f26e8548323ee6c12e7bce4e/layout/base/PresShell.cpp#7154-7157

Therefore, EventStateManager does not dispatch eMouseClick event.

Smaug, do you have some idea which causes this or way to fix simply? The caller of PresShell::HandleEvent() is nsViewManager::DispatchEvent() and it flushes pending notifications before calling PresShell::HandleEvent() when coming event is eMouseDown or eMouseUp. So, it should return same frame for eMouseUp unless user clicks top of the link's bonding box.
Flags: needinfo?(bugs)
Attached patch Automated test — — Splinter Review 
Here is automated test which I investigated with.
I'd expect bug 1089326 to help here. I need to get back to that after clearing out my review queue... so in a week or so.
Flags: needinfo?(bugs)
Hmm, but the problem here is, PresShell failed to get proper event target frame which is changed by setting position to relative when it handles eMouseUp. (Actually, my new automated test still fails with your patch for bug 1089326.)
bug 1089326 makes us not care about nsIFrames, but DOM nodes.
EventStateManager computes |mouseContent| from EventStateManager::mCurrentTarget. This is set by EventStateManager::PreHandleEvent() to the its parameter. PresShell sets the parameter to the result of PresShell::FindFrameTargetedByInputEvent(), but that is oddly parent <section> element's frame.
When preceding mouse event is handled, that causes changing style of the
target.  Therefore, when an eMouseDown or eMouseUp event is handled, handlers
require the latest information.  Currently, nsViewManager::DispatchEvent()
tries to guarantee that with calling nsIPresShell::FlushPendingNotifications()
with FlushType::Layout.  However, this is not enough if click target "position"
is changed, e.g., from "static" to "relative".  Therefore, this patch makes it
call nsViewManager::ProcessPendingUpdates() instead to flush any pending
updates in the refresh driver.
I'm not 100% sure if this approach is correct because I'm not familiar with pending reflow handling in these days...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Smaug:

Did you mean (the resigning without comment) that I should look for another reviewer in the layout team? Or something else?
Flags: needinfo?(bugs)
What happened? I did comment that ProcessPendingUpdates(); doesn't actually flush layout, so it does less, if I read the code correctly, so the click happens because layout isn't flushed, and that doesn't look right.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (high review load) from comment #14)
> What happened? I did comment that ProcessPendingUpdates(); doesn't actually
> flush layout, so it does less, if I read the code correctly, so the click
> happens because layout isn't flushed, and that doesn't look right.

Thanks. But I don't see your (inline?) comment...

And do you mean that it should call both nsIPresShell::FlushPendingNotifications() and ProcessPendingUpdate()?

(The flushing has started from bug 1398196, but it didn't add automated test sigh... I'll create new test for it too.)
I'm saying that doesn't the removing flush effective "fix" the case because we don't actually to any reframing, so the layout state is the same for mousedown and up. But such fix is a regression and fixing this bug just accidentally. Or am I missing something.
Wow, surprisingly, bug 1398196 was regressed. I see failure with writing testcase for bug 1398196 comment 2.

(In reply to Olli Pettay [:smaug] (high review load) from comment #16)
> I'm saying that doesn't the removing flush effective "fix" the case because
> we don't actually to any reframing, so the layout state is the same for
> mousedown and up. But such fix is a regression and fixing this bug just
> accidentally. Or am I missing something.

Ah, I see. If the element is focusable, nsFocusManager calls FlushPendingNotifications() at mousedown. Therefore, I didn't realize that nsViewManager::ProcessPendingUpdate() may not cause reflow.
The stack trace of calling FlushPendingNotifications() is like this:

>>	xul.dll!mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush aFlush) Line 3908	C++
>  	xul.dll!nsIDocument::FlushPendingNotifications(mozilla::ChangesToFlush aFlush) Line 7144	C++
>  	xul.dll!nsFocusManager::CheckIfFocusable(mozilla::dom::Element * aElement, unsigned int aFlags) Line 1483	C++
>  	xul.dll!nsFocusManager::SetFocusInner(mozilla::dom::Element * aNewContent=0x00000240d25e52f0, int aFlags=0x00001002, bool aFocusChanged=true, bool aAdjustWidget=0x02) Line 1125	C++
>  	xul.dll!nsFocusManager::SetFocus(mozilla::dom::Element * aElement=0x00000240d25e52f0, unsigned int aFlags=0x00001002) Line 457	C++
>  	xul.dll!mozilla::EventStateManager::PostHandleEvent(nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, nsIFrame * aTargetFrame, nsEventStatus * aStatus=0x000000721f9fbed4, nsIContent * aOverrideClickTarget=0x0000000000000000) Line 3127	C++
>  	xul.dll!mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent * aEvent, nsEventStatus * aStatus, bool aIsHandlingNativeEvent, nsIContent * aOverrideClickTarget=0x0000000000000000) Line 7292	C++
>  	xul.dll!mozilla::PresShell::HandleEvent(nsIFrame * aFrame, mozilla::WidgetGUIEvent * aEvent=0x000000721f9fbfa8, bool aDontRetargetEvents, nsEventStatus * aEventStatus=0x000000721f9fbed4) Line 6884	C++

So, my previous comment was wrong. It may be called even if the clicked element is not focusable, but it's called with FlushType::EnsurePresShellInitAndFrames. So, anyway, nsViewManager::DispatchEvent() needs to keep calling it with FlushType::Layout.
I guess the review request is not in your queue.
Flags: needinfo?(bugs)
Hmm, I don't see this in either of the review queues (bugzilla or phabricator).
Is it because I resigned as reviewer earlier?
Flags: needinfo?(bugs)
If phabricator does something unexpected, feel free to always use bugzilla's review flags :)
(In reply to Olli Pettay [:smaug] (high review load) from comment #22)
> Hmm, I don't see this in either of the review queues (bugzilla or
> phabricator).
> Is it because I resigned as reviewer earlier?

Hmm, not sure but perhaps, expected operation by Phabricator is "Request Changes". Anyway, I feel Phabricator is really not useful when review request should be raised again. E.g., it should send email with [Request] rather than [Updated].

(In reply to Olli Pettay [:smaug] (high review load) from comment #23)
> If phabricator does something unexpected, feel free to always use bugzilla's
> review flags :)

Sure.

Comment on attachment 9029387 [details]
Bug 1506508 - Make PresShell::EventHandler::HandleEvent() flush any dirty region before deciding to event target of eMouseDown and eMouseUp

Perhaps, still not in your review queue.

Attachment #9029387 - Flags: review?(bugs)

Oh, it definitely wasn't in my phabricator queue. But I see it now in bugzilla. Looking.

I investigate more for this bug. With nsIFrame::DumpFrameTree(), I got the following results:

First, before dispatching eMouseDown, a link in the automated test is:

line 25846807880: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x108] {0,2880,75720,1440} <
  Block(section)(6)@258468073a0 parent=25846806eb0 next=258468075c0 {0,2880,75720,1440} [state=0200020000100200] [content=2583a934c10] [cs=258468aec08]<
    line 25846807570: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,1610,1440} vis-overflow=0,0,1644,1440 scr-overflow=0,0,1610,1440 <
      Inline(a)(0)@25846807450 parent=258468073a0 {0,0,1610,1440} vis-overflow=0,0,1644,1440 [state=0200000000000200] [content=25846387be0] [cs=258468cf008]<
        Text(0)"link"@258468074e0 parent=25846807450 {0,0,1610,1440} vis-overflow=0,0,1644,1440 [state=02000040b0600000] [content=2584687cc00] [cs=25848477308:-moz-text] [run=258484a4880][0,4,T]
      >
    >
  >
>

Then, when nsViewManager::DispatchEvent() receives eMouseUp, it becomes:

line 25846807880: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x108] {0,2880,75720,1440} <
  Block(section)(6)@258468073a0 parent=25846806eb0 next=258468075c0 {0,2880,75720,1440} [state=0200020000101200] [content=2583a934c10] [cs=258468aec08]<
    line 25846807570: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,0,0,0} <
      Inline(a)(0)@25846807450 parent=258468073a0 {0,0,0,0} [state=0000002000000602] [content=25846387be0] [cs=25848478608]<
        Text(0)"link"@258468074e0 parent=25846807450 {0,0,0,0} [state=0000000000000402] [content=2584687cc00] [cs=258468cf008:-moz-text] [run=0][0,4,T]
      >
    >
  >
>

The size of line-box, inline frame and text frame have not been restored yet.

Then, we call nsIPresShell::FlushPendingNotifications(FlushType::Layout) before dispatching the eMouseUp event.

line 25846807880: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x108] {0,2880,75720,1440} <
  Block(section)(6)@258468073a0 parent=25846806eb0 next=258468075c0 {0,2880,75720,1440} [state=0200020000101200] [content=2583a934c10] [cs=258468aec08]<
    line 25846807570: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x1] {0,0,0,0} <
      Inline(a)(0)@25846807450 parent=258468073a0 {0,0,0,0} [state=0000002000000602] [content=25846387be0] [cs=25848478608]<
        Text(0)"link"@258468074e0 parent=25846807450 {0,0,0,0} [state=0000000000000402] [content=2584687cc00] [cs=258468cf008:-moz-text] [run=0][0,4,T]
      >
    >
  >
>

However, the sizes are still empty, not yet recomputed.

Finally, if we call nsViewManager::ProcessPendingUpdates(), we'll get:

line 25846807880: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x148] {0,2880,75720,1440} vis-overflow=0,2880,75720,1500 scr-overflow=0,2880,75720,1500 <
  Block(section)(6)@258468073a0 parent=25846806eb0 next=258468075c0 {0,2880,75720,1440} vis-overflow=0,0,75720,1500 scr-overflow=0,0,75720,1500 [state=0200020000100200] [content=2583a934c10] [cs=258468aec08]<
    line 25846807570: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,1610,1440} vis-overflow=0,0,1644,1500 scr-overflow=0,0,1610,1500 <
      Inline(a)(0)@25846807450 parent=258468073a0 {0,60,1610,1440} vis-overflow=0,0,1644,1440 scr-overflow=0,0,1610,1440 [state=0000002800000200] [content=25846387be0] [cs=25848478608]<
        Text(0)"link"@258468074e0 parent=25846807450 {0,0,1610,1440} vis-overflow=0,0,1644,1440 [state=02000040b0600000] [content=2584687cc00] [cs=258468cf008:-moz-text] [run=25848515f80][0,4,T]
      >
    >

I'm still not sure why we cannot flush the position change with nsIPresShell::FlushPendingNotifications() though, looks like that this is stopped by somebody in layout unexpectedly.

Cameron, do you have any ideas? Is this expected behavior by layout? If so, how to flush the layout forcibly with minimum risk?

Flags: needinfo?(cam)

do you know what exactly under ProcessPendingUpdates() call makes this to work? (ProcessPendingUpdates() does so many different things)

(In reply to Olli Pettay [:smaug] from comment #28)

do you know what exactly under ProcessPendingUpdates() call makes this to work? (ProcessPendingUpdates() does so many different things)

A call of nsViewManager::CallWillPaintOnObservers() in ProcessPendingUpdates() flushes the positioned element as expected.

It just calls nsIPresShell::WillPaint(), then, it makes paint observers in root precContext and calls FlushPendingNotifications(ChangesToFlush(FlushType::InterruptibleLayout, false));.

I'm still not sure which observer flushes it.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #27)

Cameron, do you have any ideas? Is this expected behavior by layout? If so, how to flush the layout forcibly with minimum risk?

In nsViewManager::DispatchEvent, the pres shell that FlushPendingNotifications is being called on is the one for the aView that is passed in to the function, and that view is the one for the widget that is targeted. In the mochitest, I think that's the top level content document (the mochitest test runner, which contains the iframe for the test). When I was testing in the layout debugger, it's the layoutdebug.xul window. So the FlushPendingNotifications doesn't do anything since there are no pending layout/style flushes in that parent document.

nsViewManager::ProcessPendingUpdates() iterates over all the descendant views and flushes their pres shells, so that's why that is working. I don't know what other work nsViewManager::ProcessPendingUpdates() does so I'll leave it to Olli to say whether that's too heavyweight or unsafe to call at this point.

In theory we do want to flush layout in the parent document, since the position of our iframe may be about to change. (In a Fission world I'm not sure, since we might have process boundaries here, and that's kind of related to bug 1440537.) After flushing the top widget's pres shell (the one for the parent document in this case), we could traverse down into each view -- if a given view's rect intersects with the point we're dispatching the event for (and is the top-most view? do we need to check for pointer-events somehow?), then flush its pres shell, and continue iterating down into it. And if not, then we can cut off the traversal.

Or, easier would be to just flush the pres shells of all views in the widget. That would be a more limited version of what nsViewManager::ProcessPendingUpdates() is doing, and would be simpler.

Flags: needinfo?(cam)

I guess an alternative would be to only flush layout in the pres shell whose view is the deepest one at the coordinate we're dispatching the event for. Maybe it's not as important for the ancestors' layout to have been flushed at this point.

Thank you for your really deep investigation.

I tried to flush all children of the PresShell from nsViewManager::DispatchEvent(), but it doesn't solve this bug. Therefore, I thought that nsIPresShell::HandleEvent() is not enough to flush position changed elements.

Finally, I got it. When we get a target in PresShell::EventHandler::HandleEvent(), the target may be in different PresShell. Inserting a method to flush layout of the child PresShell here, and recompute the target, then, I see green for the new mochitest!

Attachment #9029387 - Attachment description: Bug 1506508 - Make nsViewManager::DispatchEvent() flush any dirty region before handling eMouseDown and eMouseUp → Bug 1506508 - Make PresShell::EventHandler::HandleEvent() flush any dirty region before deciding to event target of eMouseDown and eMouseUp

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #33)

I tried to flush all children of the PresShell from nsViewManager::DispatchEvent(), but it doesn't solve this bug. Therefore, I thought that nsIPresShell::HandleEvent() is not enough to flush position changed elements.

I am curious why this doesn't work. Did you verify that when traversing down into the descendant views that you find the right document? Does FlushPendingNotifications on that document's PresShell do anything?

(In reply to Cameron McCormack (:heycam) from comment #38)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #33)

I tried to flush all children of the PresShell from nsViewManager::DispatchEvent(), but it doesn't solve this bug. Therefore, I thought that nsIPresShell::HandleEvent() is not enough to flush position changed elements.

I am curious why this doesn't work. Did you verify that when traversing down into the descendant views that you find the right document? Does FlushPendingNotifications on that document's PresShell do anything?

Unfortunately, the test patch does not remain even in my local. Therefore, it might have been just buggy patch though, I guess that the document was not a child/descendant of the PresShell received the event.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0ace82a1db85
Make PresShell::EventHandler::HandleEvent() flush any dirty region before deciding to event target of eMouseDown and eMouseUp r=smaug

https://hg.mozilla.org/mozilla-central/rev/0ace82a1db85

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Verified that today's Nightly for Android is available on the Japanese EC site, BOOTH.

Status: RESOLVED → VERIFIED

status-firefox66: --- → ?

If we should take this into Beta especially for Android, I need to rewrite the patch due to bug 1466208. Let me know if you think that this should be uplifted. I'll write it.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: