Event is not fire at the first click when position:relative is set on :active
Categories
(Core :: DOM: Events, defect, P3)
Tracking
()
People
(Reporter: karlcow, Assigned: masayuki)
References
()
Details
(Whiteboard: [webcompat])
Attachments
(2 files)
2.96 KB,
patch
|
Details | Diff | Splinter Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
Assignee | ||
Comment 10•6 years ago
|
||
Assignee | ||
Comment 11•6 years ago
|
||
Assignee | ||
Comment 12•6 years ago
|
||
Assignee | ||
Comment 13•6 years ago
|
||
Assignee | ||
Comment 15•6 years ago
|
||
Assignee | ||
Comment 17•6 years ago
|
||
Assignee | ||
Comment 18•6 years ago
|
||
Assignee | ||
Comment 19•6 years ago
|
||
Assignee | ||
Comment 20•6 years ago
|
||
Assignee | ||
Comment 21•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
Assignee | ||
Comment 25•6 years ago
|
||
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.
Oh, it definitely wasn't in my phabricator queue. But I see it now in bugzilla. Looking.
Assignee | ||
Comment 27•6 years ago
|
||
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?
do you know what exactly under ProcessPendingUpdates() call makes this to work? (ProcessPendingUpdates() does so many different things)
Assignee | ||
Comment 29•6 years ago
•
|
||
(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.
Comment 30•6 years ago
|
||
(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.
Comment 31•6 years ago
|
||
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.
Assignee | ||
Comment 32•6 years ago
|
||
Assignee | ||
Comment 33•6 years ago
|
||
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!
Assignee | ||
Comment 34•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 35•6 years ago
|
||
Assignee | ||
Comment 36•6 years ago
|
||
Assignee | ||
Comment 37•6 years ago
|
||
Comment 38•6 years ago
|
||
(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?
Assignee | ||
Comment 39•6 years ago
|
||
(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.
Comment 40•6 years ago
|
||
Comment 41•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Assignee | ||
Comment 42•6 years ago
|
||
Verified that today's Nightly for Android is available on the Japanese EC site, BOOTH.
Assignee | ||
Comment 43•6 years ago
|
||
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.
Updated•6 years ago
|
Description
•