Closed Bug 1305458 Opened 8 years ago Closed 8 years ago

Changing -moz-appearence on hover breaks change event

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Version:
42 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: dspeyer, Assigned: stone)

Details

Attachments

(15 files, 35 obsolete files)

1305458.html
473 bytes, text/html
Details
Part1: Rename nsIDOMEventTarget::PreHandleEvent to nsIDOMEventTarget::GetEventTargetParent. r=smaug
78.89 KB, patch
stone
: review+
Details | Diff | Splinter Review
Part2: Add nsIDOMEventTarget::PreHandleEvent. r=smaug
7.70 KB, patch
stone
: review+
Details | Diff | Splinter Review
Part3: Add EventTargetChainItem::GetFirstEventTarget. r=smaug
4.96 KB, patch
stone
: review+
Details | Diff | Splinter Review
Part4: Call EventTargetChainItem::PreHandleEvent even it sets mCanHandle=false. r=smaug
10.25 KB, patch
stone
: review+
Details | Diff | Splinter Review
Part5: Move form control frame focus/blur from nsGenericHTMLFormElement::GetEventTargetParent to PreHandleEvent. r=smaug
3.24 KB, patch
stone
: review+
Details | Diff | Splinter Review
Part6: Move fire change event from HTMLTextAreaElement::GetEventTargetParent to PreHandleEvent. r=smaug
2.75 KB, patch
stone
: review+
Details | Diff | Splinter Review
Part7: Refine nsXULElement::GetEventTargetParent. r=smaug
7.41 KB, patch
stone
: review+
Details | Diff | Splinter Review
Part8: Move dispatch XUL command from nsXULElement::GetEventTargetParent to PreHandleEvent. r=smaug
4.98 KB, patch
stone
: review+
Details | Diff | Splinter Review
Part9: Move fire events and set value from HTMLInputElement::GetEventTargetParent to PreHandleEvent. r=smaug
7.87 KB, patch
stone
: review+
Details | Diff | Splinter Review
Bug 1305458 Part10: Add test case. r=smaug
2.49 KB, patch
stone
: review+
Details | Diff | Splinter Review
Bug 1305458 Part11: Let HTMLInputElement delegate event handling to it's parent class. r=smaug
2.12 KB, patch
stone
: review+
Details | Diff | Splinter Review
Bug 1305458 Part12: Refine EventTargetChain flags to reduce overheads. r=smaug
5.98 KB, patch
stone
: review+
Details | Diff | Splinter Review
Bug 1305458 Part13: Refine event target chain creation. r=smaug
5.70 KB, patch
stone
: review+
Details | Diff | Splinter Review
Bug 1305458 Part14: Refine assertion in EventTargetChainItem::Create r=smaug
1.89 KB, patch
stone
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

Steps to reproduce:

Open the following webpage:

<html>
<head>
  <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.2.0/jquery.min.js"></script>
  <style>
   input[type=number] {
     -moz-appearance: textfield;
   }
   input[type=number]:focus,
   input[type=number]:hover {
     -moz-appearance: number-input;
   }
  </style>
</head>
<body>
  <input id="foo" type="number">
  <script>
   $('#foo').on('change', function(){
       $('body').append('X');
   });
  </script>
</body>
</html>

Click on the input, leave the mouse there, enter a number, hit tab.
Click on the input, take the mouse out, enter a number, hit tab.



Actual results:

An "X" appeared on the page only the first time


Expected results:

"X"s should have appeared on the page both times.
Attached file 1305458.html — 

    
  
Component: Untriaged → Event Handling
Product: Firefox → Core

    
    

    
  
Hi Stone, are you interested in taking a look?
Flags: needinfo?(sshih)
Priority: -- → P3

    
  
Assignee: nobody → sshih
Flags: needinfo?(sshih)

    
    

    
  
Hover state changes will induce style changes. When anonymous input (text) element pre-handles blur event (user hit tap), it [1] delegates event to nsGenericHTMLFormElement::PreHandleEvent which gets its from control frame [2] and then induce flushing pending restyles [3]. After flush pending notifications, the nsAutoScriptBlocker in [4] is destructed, the blocked runnables are executed. The old anonymous elements are unbind from tree. When the call stack pop back to EventDispatcher::Dispatch, the event is stopped propagating to the non-anonymous input (number) element. That's why the listener is not triggered.

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#3880
[2] http://searchfox.org/mozilla-central/source/dom/html/nsGenericHTMLElement.cpp#2204
[3] http://searchfox.org/mozilla-central/source/dom/html/nsGenericHTMLElement.cpp#1014
[4] http://searchfox.org/mozilla-central/source/layout/base/nsPresShell.cpp#4115

    
    

    
  
When user hits tab with mouse doesn't hover on input
Anonymous input element PreHandleEvent function handles blur event and fire change event on itself
Anonymous input element PreHandleEvent function trigger flushing pending restyles
Anonymous elements unbind from tree
Call stack pop back to EventDispatcher::Dispatch (dispatch blur event) and continue ask the parent of anonymous input element to handle the blur event.
At this moment, the anonymous input element is unbind from tree so the non-anonymous input element won't handle the blur event and have no chance to fire change event.

    
    

    
  
The pending restyles are caused in [1]. Trying to flush pending styles earlier [2] and it will induce the NAC unbind from tree earlier while the target of blur event is already set to the anonymous input element. Not work for this bug.

I'm wondering what is the style applied to frames should be when content handling focus / blur events? When element is blurred, if we update its style before dispatching event, the event target may be unbind from tree. Or we have to recalculate the event target.

If we delay flushing pending styles after dispatching blur event. Get styles in the listeners of content will return styles before blur happened.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1663
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1711

    
    

    
  
Whenever JS accesses some styling information, layout is flushed.

For flushing earlier I was thinking something like keep a strong reference to the first non-NAC element, and if flushing removes NAC, target focus changes to the non-NAC element.

    
    

    
  
In the case of blur, nsFocusManager::Blur triggers NotifyFocusStateChange (triggers pending restyles) and SendFocusOrBlurEvent. That means we have to flush pending restyles after NotifyFocusStateChange and before SendFocusOrBlurEvent. We may NotifyFocusStateChange on NAC element and fire blur event to non-NAC element when the NAC element is unbind from tree. 

In the case of focus, it's similar to blur but additionally change focus to the non-NAC element. Is my understanding correct?

    
    

    
  
yeah. That is something I'd try, unless there are other better options.
One other possibility is to add a new callback param to EventDispatcher::Dispatch which would be called right after creating event target chain but before handling the event, and flush layout using that callback.

    
    

    
  
Collected ::PreHandleEvent which may fire events or flush pending restyles

HTMLInputElement::PreHandleEvent
  May fire change event in [1]

HTMLButtonElement::PreHandleEvent
  May notify form observers in [2] and trigger js code [3]

HTMLTextAreaElement::PreHandleEvent
  May fire change event when handling blur event [4]

nsGenericHTMLFormElement::PreHandleEvent
  May flush pending restyles in [5]

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#3933
[2] http://searchfox.org/mozilla-central/source/dom/html/HTMLFormElement.cpp#1588
[3] http://searchfox.org/mozilla-central/source/toolkit/components/satchel/formSubmitListener.js#119
[4] http://searchfox.org/mozilla-central/source/dom/html/HTMLTextAreaElement.cpp#539
[5] http://searchfox.org/mozilla-central/source/dom/html/nsGenericHTMLElement.cpp#2204

    
    

    
  


  

    
    

    
  


  
Considering the case that an element wants PreHandleEvent but set mCanHandle=false, we can't destroy the EventTargetChainItem immediately. So I keep the EventTargetChainItem in the event target chain (if mCanHandle=true) and call its PreHandleEvent after the event target chain is created. And also ignore dispatching event to its listener.

        Attachment #8805408 -
        Attachment is obsolete: true

    
    

    
  
Tested performance overheads with [1] and the results are
before: avg of 30 results (total tests 50) : 102.96666666666667ms
after: avg of 30 results (total tests 50) : 113.16666666666667ms

[1] http://mozilla.pettay.fi/moztests/events/event_speed_2.1_nolistener.html

    
  

        Attachment #8805405 -
        Flags: review?(bugs)

    
  

        Attachment #8805406 -
        Flags: review?(bugs)

    
  

        Attachment #8805407 -
        Flags: review?(bugs)

    
  

        Attachment #8805486 -
        Flags: review?(bugs)

    
  

        Attachment #8805409 -
        Flags: review?(bugs)

    
  

        Attachment #8805410 -
        Flags: review?(bugs)

    
  

        Attachment #8805411 -
        Flags: review?(bugs)

    
  

        Attachment #8805412 -
        Flags: review?(bugs)

    
  

        Attachment #8805413 -
        Flags: review?(bugs)

    
  

        Attachment #8805414 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Ming-Chou Shih [:stone] from comment #22)
> Tested performance overheads with [1] and the results are
> before: avg of 30 results (total tests 50) : 102.96666666666667ms
> after: avg of 30 results (total tests 50) : 113.16666666666667ms

I assume that is with opt builds.
Surprisingly high overhead, given that there is the NS_TARGET_CHAIN_WANTS_PRE_HANDLE_EVENT optimization.
I wonder if we could improve the performance somehow.

Have you perhaps profiled the builds? I don't think Gecko Profiler gives good enough data for this kind of case.
I use Zoom on linux. I think Instruments works well on OSX.

        Attachment #8805405 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8805406 [details] [diff] [review]
Part2: Add nsIDOMEventTarget::PreHandleEvent

>+nsresult
>+nsINode::PreHandleEvent(const EventChainPreVisitor& aVisitor)
>+{
>+  NS_ABORT();
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}
Hmm, why NS_ABORT and NS_ERROR_NOT_IMPLEMENTED?
I would expect subclasses to call this.
Or, perhaps there shouldn't be this at all, but just dummy implementation in nsIDOMEventTarget.idl, using
%{C++ stuff there.

>-        // Event target chain is created. Handle the chain.
>+        // Event target chain is created. PreHandle the chain.
>+        for (uint32_t i = 0; i < chain.Length(); ++i) {
>+          if (chain[i].WantsPreHandleEvent()) {
>+            chain[i].PreHandleEvent(preVisitor);
>+          }
Since this is rather hot path, I actually wonder if WantsPreHandleEvent() could be called inside
PreHandleEvent and if WantsPreHandleEvent() returns false, return early from the method.
That should optimize out one chain[i] access.  (Perhaps compiler optimizes that out already, but is it guaranteed on all the platforms?)

>+++ b/dom/interfaces/events/nsIDOMEventTarget.idl
>@@ -222,16 +222,24 @@ interface nsIDOMEventTarget : nsISupport
>    *                 event target chain for event dispatching.
>    *
>    * @note Only EventDispatcher should call this method.
>    */
>   [noscript, nostdcall]
>   void GetEventTargetParent(in EventChainPreVisitorRef aVisitor);
> 
>   /**
>+   * Called before the capture phase of the event flow and after event target
>+   * chain creation. This is used to handle those stuffs must be executed before
>+   * dispatch event to DOM
>+   */
>+  [noscript, nostdcall]
>+  void PreHandleEvent([const] in EventChainPreVisitorRef aVisitor);
So this could become something like

%{C++
  virtual nsresult PreHandleEvent(EventChainPreVisitor& aVisitor)
  {
    return NS_OK;
  }
%}


Those fixed, r+

        Attachment #8805406 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8805407 [details] [diff] [review]
Part3: Add EventTargetChainItem::GetFirstEventTarget

># HG changeset patch
># User Stone Shih <sshih@mozilla.com>
># Date 1477099300 -28800
>#      å­ 10æ 22 09:21:40 2016 +0800
># Node ID ef14c598deb8683afc485278e1305e61295d0c77
># Parent  c4e06d027eaba780eb2570691bd886ca221e4e92
>Bug 1305458 Part3: Add EventTargetChainItem::GetFirstEventTarget
>
>MozReview-Commit-ID: EssfUw8QQqH
>
>diff --git a/dom/events/EventDispatcher.cpp b/dom/events/EventDispatcher.cpp
>--- a/dom/events/EventDispatcher.cpp
>+++ b/dom/events/EventDispatcher.cpp
>@@ -161,16 +161,27 @@ public:
>   static void DestroyLast(nsTArray<EventTargetChainItem>& aChain,
>                           EventTargetChainItem* aItem)
>   {
>     uint32_t lastIndex = aChain.Length() - 1;
>     MOZ_ASSERT(&aChain[lastIndex] == aItem);
>     aChain.RemoveElementAt(lastIndex);
>   }
> 
>+
>+  static int GetFirstEventTargetIdx(nsTArray<EventTargetChainItem>& aChain)
Not sure how useful this method is.
Could you just define some static const uint32_t kFirstEventTargetIdx = 0;


And then GetFirstEventTarget could be

static EventTargetChainItem* GetFirstEventTarget(
                               nsTArray<EventTargetChainItem>& aChain)
{
  return &aChain[kFirstEventTargetIdx];
>   uint32_t chainLength = aChain.Length();
>+  uint32_t firstEventTargetIdx =
>+    EventTargetChainItem::GetFirstEventTargetIdx(aChain);
Then you'd not need this, since kFirstEventTargetIdx could be used everywhere.

>+    MOZ_ASSERT(EventTargetChainItem::GetFirstEventTargetIdx(chain) == 0);
This could be then removed

Those fixed, r+

        Attachment #8805407 -
        Flags: review?(bugs) → review+

        Attachment #8805409 -
        Flags: review?(bugs) → review+

        Attachment #8805410 -
        Flags: review?(bugs) → review+

        Attachment #8805411 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8805412 [details] [diff] [review]
Part8: Move dispatch XUL command from nsXULElement::GetEventTargetParent to PreHandleEvent

Good that mItemFlags gives this flexibility.

        Attachment #8805412 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8805406 [details] [diff] [review]
Part2: Add nsIDOMEventTarget::PreHandleEvent

Oh, wait, why is EventTargetChainItem::PreHandleEvent right?
Nothing sets the right mItemFlags or mItemData.

        Attachment #8805406 -
        Flags: review+ → review-

    
    

    
  
Comment on attachment 8805406 [details] [diff] [review]
Part2: Add nsIDOMEventTarget::PreHandleEvent

Actually, I think PreHandleEvent shouldn't take EventChainPreVisitor& as param, since some of the properties it has aren't valid when PreHandleEvent is called.
Perhaps better to pass EventChainVisitor&.

    
    

    
  
Comment on attachment 8805413 [details] [diff] [review]
Part9: Move fire events and set value from HTMLInputElement::GetEventTargetParent to PreHandleEvent

>+HTMLInputElement::PreHandleEvent(const EventChainPreVisitor& aVisitor)
>+{
>+  if (!aVisitor.mPresContext) {
>+    return nsGenericHTMLElement::PreHandleEvent(aVisitor);
>+  }
Should use nsGenericHTMLFormElementWithState here. The existing code has a bug. But can be fixed also in a followup.

        Attachment #8805413 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8805486 [details] [diff] [review]
Part4: Call EventTargetChainItem::PreHandleEvent even it sets mCanHandle=false



>+EventTargetChainItem::PreHandleEvent(EventChainPreVisitor& aVisitor)
> {
>+  aVisitor.Reset();
>+  aVisitor.mItemFlags = mItemFlags;
>+  aVisitor.mItemData = mItemData;
>   Unused << mTarget->PreHandleEvent(aVisitor);
> }
Ah, this is fixed here. nm my earlier comment then


>   // Capture
>   aVisitor.mEvent->mFlags.mInCapturePhase = true;
>   aVisitor.mEvent->mFlags.mInBubblingPhase = false;
>   for (uint32_t i = chainLength - 1; i > firstEventTargetIdx; --i) {
>     EventTargetChainItem& item = aChain[i];
>+    if (item.PreHandleEventOnly()) {
>+      continue;
>+    }
>     if ((!aVisitor.mEvent->mFlags.mNoContentDispatch ||
>          item.ForceContentDispatch()) &&
>         !aVisitor.mEvent->PropagationStopped()) {
>       item.HandleEvent(aVisitor, aCd);
>     }
> 
>     if (item.GetNewTarget()) {
>       // item is at anonymous boundary. Need to retarget for the child items.
>@@ -423,16 +452,19 @@ EventTargetChainItem::HandleEventTargetC
>   if (aVisitor.mEvent->mFlags.mInSystemGroup) {
>     targetItem.PostHandleEvent(aVisitor);
>   }
> 
>   // Bubble
>   aVisitor.mEvent->mFlags.mInCapturePhase = false;
>   for (uint32_t i = firstEventTargetIdx + 1; i < chainLength; ++i) {
>     EventTargetChainItem& item = aChain[i];
>+    if (item.PreHandleEventOnly()) {
>+      continue;
>+    }
Why the target phase doesn't need to check PreHandleEventOnly()? Though, that wouldn't be quite right either.
Basically I don't understand why firstEventTargetIdx is right in the loops. What if both 0 and 1 indexes set PreHandleEventOnly()?
Shouldn't GetFirstEventTargetIdx loop from chain until it finds one entry which returns false for PreHandleEventOnly().


(or nm my earlier comments about sFirstEventTargetIdx)

        Attachment #8805486 -
        Flags: review?(bugs) → review-

        Attachment #8805414 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8805406 [details] [diff] [review]
Part2: Add nsIDOMEventTarget::PreHandleEvent

ok, this is fine since mItemFlags or mItemData are set in a different patch.
But still I think PreHandleEvent shouldn't take EventChainPreVisitor& as param, if possible, since some of the properties it has aren't valid when PreHandleEvent is called.
Perhaps better to pass EventChainVisitor&.

That fixed, or explained why it can't be done, r+

        Attachment #8805406 -
        Flags: review- → review+

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8805486 [details] [diff] [review]
> Part4: Call EventTargetChainItem::PreHandleEvent even it sets
> mCanHandle=false
> Why the target phase doesn't need to check PreHandleEventOnly()? Though,
> that wouldn't be quite right either.
It's because I thought the target node must be the node that set EventChainPreVisitor::mCanHandle=true. Is my understanding correct?

> Basically I don't understand why firstEventTargetIdx is right in the loops.
> What if both 0 and 1 indexes set PreHandleEventOnly()?
> Shouldn't GetFirstEventTargetIdx loop from chain until it finds one entry
> which returns false for PreHandleEventOnly().
In current implementation, if the hit node set EventChainPreVisitor::mCanHandle=false, we try to propagate event to chrome when EventChainPreVisitor::mAutomaticChromeDispatch=true. If the chrome target still can not handle the event, then we won't continue to dispatch the event. When dispatching events to DOM, either the original target node or the chrome target can handle the event. So there should be up to 1 node that is PreHandleEventOnly in the chain. But it's less readability so I'd refine the patch to use a loop to find the target entry.

    
    

    
  
(In reply to Ming-Chou Shih [:stone] from comment #32)
> refine the patch to use a loop to find the target entry.
Maybe I should use sFirstEventTargetIdx with less overhead

    
    

    
  
Ming-Chou Shih [:stone] from comment #33)
> Maybe I should use sFirstEventTargetIdx with less overhead
Nah, that comment was for previous patches before I saw how you're going to use the index.


(In reply to Ming-Chou Shih [:stone] from comment #32)
> In current implementation, if the hit node set
> EventChainPreVisitor::mCanHandle=false, we try to propagate event to chrome
> when EventChainPreVisitor::mAutomaticChromeDispatch=true. If the chrome
> target still can not handle the event, then we won't continue to dispatch
> the event. When dispatching events to DOM, either the original target node
> or the chrome target can handle the event. So there should be up to 1 node
> that is PreHandleEventOnly in the chain. But it's less readability so I'd
> refine the patch to use a loop to find the target entry.
But it may not be the first (non-chrome) item in the chain which sets PreHandleEventOnly and mCanHandle=false

    
    

    
  


  
Refine GetFirstEventTargetIdx to loop from chain until it finds the first entry which is not PreHandleEventOnly

        Attachment #8805486 -
        Attachment is obsolete: true

    
    

    
  


  
Tested with patch 1~11
avg of 30 results (total tests 50) : 111.6ms
avg of 30 results (total tests 50) : 111.4ms
avg of 30 results (total tests 50) : 111.96666666666667ms
avg of 30 results (total tests 50) : 112ms
avg of 30 results (total tests 50) : 112.23333333333333ms

Tested with patch 1~12
avg of 30 results (total tests 50) : 109.33333333333333ms
avg of 30 results (total tests 50) : 110.03333333333333ms
avg of 30 results (total tests 50) : 109.56666666666666ms
avg of 30 results (total tests 50) : 109.43333333333334ms
avg of 30 results (total tests 50) : 110.3ms

Tested with patch 1~13
avg of 30 results (total tests 50) : 110.2ms
avg of 30 results (total tests 50) : 109.76666666666667ms
avg of 30 results (total tests 50) : 109.16666666666667ms
avg of 30 results (total tests 50) : 109.56666666666666ms
avg of 30 results (total tests 50) : 109.63333333333334ms

    
    

    
  
Profiling with zoom on Linux, looks like the extra overheads are induced by accessing to the chain, invocation of PreHandleEvent and PreHandleEventOnly.

    
  

        Attachment #8806669 -
        Flags: review?(bugs)

    
    

    
  


  
Refined patch to use inline functions and explicit constructor.

        Attachment #8806668 -
        Attachment is obsolete: true

        Attachment #8806968 -
        Flags: review?(bugs)

    
  

        Attachment #8806665 -
        Flags: review?(bugs)

    
    

    
  


  
Refine the logic of calling PreHandleEvent when the original target and chrome target set mCanHandle=false

        Attachment #8806663 -
        Attachment is obsolete: true

        Attachment #8806997 -
        Flags: review?(bugs)

    
    

    
  


  
Fix typo

        Attachment #8806968 -
        Attachment is obsolete: true

        Attachment #8806968 -
        Flags: review?(bugs)

        Attachment #8806998 -
        Flags: review?(bugs)

    
    

    
  


  

        Attachment #8807037 -
        Attachment is obsolete: true

    
  

        Attachment #8807037 -
        Flags: review?(bugs)

        Attachment #8806665 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8806669 [details] [diff] [review]
Part13: Define functions access to WidgetEvent::mFlags as inline functions

This isn't need, based on some comments I found. Apparently "all member functions defined inside class definition are inline"

        Attachment #8806669 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8806998 [details] [diff] [review]
Part12: Refine EventTargetChain flags to reduce overheads

Drop 'inline' everywhere.

s/wihch/which/

I think having the separate flags in EventTargetChainItem would have been a bit nicer, but this is ok too.

        Attachment #8806998 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8806997 [details] [diff] [review]
Part4: Call EventTargetChainItem::PreHandleEvent even it sets mCanHandle=false


>-  static uint32_t GetFirstEventTargetIdx(nsTArray<EventTargetChainItem>& aChain)
>+  static uint32_t GetFirstCanHandleEventTargetIdx(nsTArray<EventTargetChainItem>& aChain)
>   {
>+    // aChain[i].PreHandleEventOnly() = true only when the original target
Why 'original target'? Shouldn't it be just 'target'.


>+    // element wants PreHandleEvent and set mCanHandle=false. So we find the
>+    // first element which can handle the event.
>+    for (uint32_t i = 0; i < aChain.Length(); ++i) {
>+      if (!aChain[i].PreHandleEventOnly()) {
So shouldn't mCanHandle be true here. Could we assert that.

        Attachment #8806997 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8807037 [details] [diff] [review]
Part14: Refine event target chain creation

oh, this was still in my queue, but marked as obsolete.

        Attachment #8807037 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #47)
> Comment on attachment 8806997 [details] [diff] [review]
> Part4: Call EventTargetChainItem::PreHandleEvent even it sets
> mCanHandle=false
> >+    // element wants PreHandleEvent and set mCanHandle=false. So we find the
> >+    // first element which can handle the event.
> >+    for (uint32_t i = 0; i < aChain.Length(); ++i) {
> >+      if (!aChain[i].PreHandleEventOnly()) {
> So shouldn't mCanHandle be true here. Could we assert that.
We don't cache mCanHandle in EventTargetChainItem. Should I add one more bool flag to keep it?

    
  

        Attachment #8807087 -
        Flags: review?(bugs)

        Attachment #8807087 -
        Flags: review?(bugs) → review+

    
    

    
  


  

        Attachment #8805405 -
        Attachment is obsolete: true

        Attachment #8805406 -
        Attachment is obsolete: true

        Attachment #8805407 -
        Attachment is obsolete: true

        Attachment #8805409 -
        Attachment is obsolete: true

        Attachment #8805410 -
        Attachment is obsolete: true

        Attachment #8805411 -
        Attachment is obsolete: true

        Attachment #8805412 -
        Attachment is obsolete: true

        Attachment #8805413 -
        Attachment is obsolete: true

        Attachment #8805414 -
        Attachment is obsolete: true

        Attachment #8806665 -
        Attachment is obsolete: true

        Attachment #8806669 -
        Attachment is obsolete: true

        Attachment #8806997 -
        Attachment is obsolete: true

        Attachment #8806998 -
        Attachment is obsolete: true

        Attachment #8807087 -
        Attachment is obsolete: true

        Attachment #8808866 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part10: Add test case. r=smaug (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8808875 -
        Flags: review+

    
    

    
  
Comment on attachment 8810758 [details] [diff] [review]
Bug 1305458 Part14: Refine assertion in EventTargetChainItem::Create

EventTargetChainItem::Create will check if the child item is the last item in the array. However, the patches of this bug may keep some items which are PreHandleEventOnly in the array so I have to refine the assertion.

        Attachment #8810758 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8810758 [details] [diff] [review]
Bug 1305458 Part14: Refine assertion in EventTargetChainItem::Create

s/Fine/Find/

        Attachment #8810758 -
        Flags: review?(bugs) → review+

    
    

    
  


  
Rebased

        Attachment #8808867 -
        Attachment is obsolete: true

        Attachment #8811658 -
        Flags: review+

    
    

    
  


  
Rebased

        Attachment #8808868 -
        Attachment is obsolete: true

        Attachment #8811659 -
        Flags: review+

    
    

    
  


  
Rebased

        Attachment #8808872 -
        Attachment is obsolete: true

        Attachment #8811663 -
        Flags: review+

    
    

    
  


  
Rebased

        Attachment #8808875 -
        Attachment is obsolete: true

        Attachment #8811666 -
        Flags: review+

    
    

    
  


  
Rebased

        Attachment #8808878 -
        Attachment is obsolete: true

        Attachment #8811669 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8842555071
Part1: Rename nsIDOMEventTarget::PreHandleEvent to nsIDOMEventTarget::GetEventTargetParent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c53113dd77
Part2: Add nsIDOMEventTarget::PreHandleEvent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4c31d96593
Part3: Add EventTargetChainItem::GetFirstEventTarget. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf729308c40
Part4: Call EventTargetChainItem::PreHandleEvent even it sets mCanHandle=false. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/76bc134bda00
Part5: Move form control frame focus/blur from nsGenericHTMLFormElement::GetEventTargetParent to PreHandleEvent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d88da6f4b03
Part6: Move fire change event from HTMLTextAreaElement::GetEventTargetParent to PreHandleEvent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/be4e001288d0
Part7: Refine nsXULElement::GetEventTargetParent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a75a459c545
Part8: Move dispatch XUL command from nsXULElement::GetEventTargetParent to PreHandleEvent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fbc6dab40e3
Part9: Move fire events and set value from HTMLInputElement::GetEventTargetParent to PreHandleEvent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5db030d2903a
Part10: Add test case. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52950131023
Part11: Let HTMLInputElement delegate event handling to it's parent class. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a961e6f68831
Part12: Refine EventTargetChain flags to reduce overheads. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2078069f8c16
Part13: Refine event target chain creation. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbab4273091a
Part14: Refine assertion in EventTargetChainItem::Create r=smaug
Keywords: checkin-needed
Component: Event Handling → User events and focus handling

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: