Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Description • 7 years ago

The spec is currently wrong
https://github.com/w3c/touch-events/issues/92

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 1 • 6 years ago

This is pretty hard to review, I think.
And partially because https://github.com/w3c/touch-events/issues/92 for example is still an open issue in the spec.
But the basic idea (per DOM spec) is to retarget Touch objects similarly to other targets. touchEvent.targetTouches makes this a bit more complicated, since we need
to modify the number of touch objects in that list.
Also, dispatching touch events from JS is somewhat unspecified - so I decided to keep the old behavior in case shadow boundary isn't crossed (so targetTouches list isn't modified).


The code in nsIContent::GetEventTargetParent should be somewhat clear, we just
retarget Touch objects like we retarget other stuff too.
EventTargetChainItem::HandleEventTargetChain also tries to follow existing pattern, so look around the surrounding code.
To be able to restore the original target, I added mOriginalTarget to Touch objects.


remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/fdf3a57a89ee4c586b56048bd059539cc1c019a2
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdf3a57a89ee4c586b56048bd059539cc1c019a2
remote: recorded changegroup in replication log in 0.113s


I thought about splitting this to several patches, but the only somewhat separate piece is NAC support in
TouchEvent::TargetTouches() and that is just couple of lines.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 2 • 6 years ago

Comment on attachment 8979128 [details] [diff] [review]
shadow_touch_target.diff

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

Well, I still need to review EventDispatcher.cpp carefully.

::: dom/base/FragmentOrElement.cpp
@@ +1105,5 @@
> +      // Retarget touch objects.
> +      MOZ_ASSERT(aVisitor.mRetargetedTouchTargets.IsEmpty());
> +      WidgetTouchEvent* touchEvent = aVisitor.mEvent->AsTouchEvent();
> +      WidgetTouchEvent::TouchArray& touches = touchEvent->mTouches;
> +      for (uint32_t i = 0; i < touches.Length(); ++i) {

If we make TouchArray ranged-for-loop aware, we write this kind of loop smarter...

@@ +1107,5 @@
> +      WidgetTouchEvent* touchEvent = aVisitor.mEvent->AsTouchEvent();
> +      WidgetTouchEvent::TouchArray& touches = touchEvent->mTouches;
> +      for (uint32_t i = 0; i < touches.Length(); ++i) {
> +        Touch* touch = touches[i];
> +        EventTarget* target = touch->mOriginalTarget;

I don't understand why do you use temporarily variable for mOriginalTarget. But it's fine if it's necessary to make following lines shorter.

But I like |originalTarget| better.

@@ +1113,5 @@
> +        nsCOMPtr<nsINode> targetAsNode = do_QueryInterface(target);
> +        if (targetAsNode) {
> +          EventTarget* retargeted = nsContentUtils::Retarget(targetAsNode, this);
> +          if (retargeted) {
> +            touchTarget = retargeted;

I'm not sure if this works as you expected when this and original target are both in different shadow DOM tree. If it works, I'm fine.

@@ +1120,5 @@
> +        aVisitor.mRetargetedTouchTargets.AppendElement(touchTarget);
> +        touch->mTarget = touchTarget;
> +      }
> +      MOZ_ASSERT(aVisitor.mRetargetedTouchTargets.Length() ==
> +                 touches.Length());

nit: Shouldn't need more 2 spaces?

::: dom/events/EventDispatcher.h
@@ +276,5 @@
> +   * If mEvent is a WidgetTouchEvent and its mTouches needs retargeting,
> +   * set the targets to this array. The array should contain one entry per
> +   * each object in WidgetTouchEvent::mTouches.
> +   */
> +  nsTArray<RefPtr<dom::EventTarget>> mRetargetedTouchTargets;

Looks like that this class is instantiated only in the stack. So, if you use AutoTArray<RefPtr<dom::EventTarget>, 10>, it won't allocate something in the heap. But up to you.

::: dom/events/Touch.cpp
@@ +223,5 @@
>  void
>  Touch::SetTarget(EventTarget* aTarget)
>  {
> +  if (!mOriginalTarget) {
> +    mOriginalTarget = aTarget;

Well, I don't like to make method stateful, though. I'm not sure this works as expected for each caller of ESM. (E.g., if caller needs to override target simply, they might want to update mOriginalTarget too.)

::: dom/events/TouchEvent.cpp
@@ +114,5 @@
> +  CopyTouchesToWidgetEvent(mChangedTouches, true);
> +}
> +
> +void
> +TouchEvent::CopyTouchesToWidgetEvent(TouchList* aList, bool aCheckDuplicates)

Hmm, this method name starts with "Copy", but actually not copying the instances, just sets the touches to WidgetTouchEvent... But TouchEvent::CopyTouches() uses "Copy". So, up to you whether this should be named as Copy or not.

@@ +125,5 @@
> +    Touch* touch = aList->Item(i);
> +    if (touch &&
> +        (!aCheckDuplicates ||
> +         !mEvent->AsTouchEvent()->mTouches.Contains(touch))) {
> +      mEvent->AsTouchEvent()->mTouches.AppendElement(touch);

Please cache the pointer to WidgetTouchEvent outside of this loop. It's virtual call, therefore, this wastes some unnecessary virtual calls.

@@ +164,5 @@
>      const WidgetTouchEvent::TouchArray& touches = touchEvent->mTouches;
>      for (uint32_t i = 0; i < touches.Length(); ++i) {
>        // for touchend/cancel events, don't append to the target list if this is a
>        // touch that is ending
>        if ((mEvent->mMessage != eTouchEnd && mEvent->mMessage != eTouchCancel) ||

Although, it is not scope of this bug, if the message is eTouchEnd or eTouchCancel, it does not make sense to run this for-loop. It should be checked before running this loop.

::: dom/events/TouchEvent.h
@@ +101,5 @@
>    CopyTouches(const Sequence<OwningNonNull<Touch>>& aTouches);
>  
>    TouchList* Touches();
>    TouchList* TargetTouches();
> +  TouchList* GetExistingTargetTouches()

Perhaps, for users of this class, those similar names methods should have explanation here.

::: dom/events/test/window_bug1429572.html
@@ +23,5 @@
> +        }
> +        return touchArray;
> +      }
> +
> +      function dispatchTouches(targets, xOffsets) {

synthesizeTouches is better for guessing what this does since dispatchTouches sounds like dispatching untrusted events.

@@ +72,5 @@
> +        var nonShadow = document.getElementById("nonshadow");
> +        var event;
> +        nonShadow.ontouchstart = function(e) {
> +          event = e;
> +          opener.is(e.targetTouches.length, 1, "Should have only one entry in targetTouches.");

Hmm, why don't you create is() in this file to refer opener's is()?

@@ +139,5 @@
> +
> +        var rect = date.getBoundingClientRect();
> +        var quarter = rect.width / 4;
> +        dispatchTouches([date, date, s1], [-quarter, quarter, 0]);
> +        opener.ok(hasDateAsTarget, "Should have seen touchstart with date element as the target.")

And also if there is ok() in this file to refer the opener's, this must look like simpler.

@@ +184,5 @@
> +        var s1 = document.getElementById("span1");
> +
> +        var hostHandled = false;
> +        host.ontouchstart = function(e) {
> +          if (e.target == host) {

Does host receive other touchstart event(s)? If yes, how about to check the other cases too?

@@ +189,5 @@
> +            hostHandled = true;
> +            opener.is(e.targetTouches.length, 2, "Should have two entries in targetTouches.");
> +            opener.is(e.targetTouches[0].target, e.target, "targetTouches should contain event.target.");
> +            opener.is(e.targetTouches[1].target, e.target, "targetTouches should contain event.target twice.");
> +            opener.is(e.touches.length, 3, "touches list should contain one touch object.");

Why don't you test if e.targetTouches[2].target is s1 (or something another value)?

@@ +209,5 @@
> +        host.ontouchstart = function(e) {
> +          opener.is(e.targetTouches.length, 2, "Should have all the shadow entries in targetTouches.");
> +          opener.is(e.targetTouches[0].target, host, "targetTouches shouldn't reveal shadow DOM.");
> +          opener.is(e.targetTouches[1].target, host, "targetTouches shouldn't reveal shadow DOM.");
> +          opener.is(e.touches.length, 3, "touches list should contain all the touch objects.");

Why don't you test if e.targetTouches[2].target is s1 (or something another value)?

@@ +216,5 @@
> +        shadowS1.ontouchstart = function(e) {
> +          opener.is(e.targetTouches.length, 3, "Should have all the in targetTouches.");
> +          opener.is(e.targetTouches[0].target, shadowS1, "targetTouches should contain two shadow elements.");
> +          opener.is(e.targetTouches[1].target, shadowS2, "targetTouches should contain two shadow elements.");
> +          opener.is(e.targetTouches[2].target, s1, "targetTouches should contain a slight element.");

Interesting. Shouldn't we check the case when there are two or more shadow DOM trees and user touches both shadow DOM trees?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 3 • 6 years ago

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

> If we make TouchArray ranged-for-loop aware, we write this kind of loop
> smarter...
I truly hate ranged-for ;) It has lead to so many security critical bugs.


> 
> @@ +1107,5 @@
> > +      WidgetTouchEvent* touchEvent = aVisitor.mEvent->AsTouchEvent();
> > +      WidgetTouchEvent::TouchArray& touches = touchEvent->mTouches;
> > +      for (uint32_t i = 0; i < touches.Length(); ++i) {
> > +        Touch* touch = touches[i];
> > +        EventTarget* target = touch->mOriginalTarget;
> 
> I don't understand why do you use temporarily variable for mOriginalTarget.
> But it's fine if it's necessary to make following lines shorter.
that.

> But I like |originalTarget| better.
ok

> > +        nsCOMPtr<nsINode> targetAsNode = do_QueryInterface(target);
> > +        if (targetAsNode) {
> > +          EventTarget* retargeted = nsContentUtils::Retarget(targetAsNode, this);
> > +          if (retargeted) {
> > +            touchTarget = retargeted;
> 
> I'm not sure if this works as you expected when this and original target are
> both in different shadow DOM tree. If it works, I'm fine.
yes it does. Retarget() is modeled per spec.

> 
> @@ +1120,5 @@
> > +        aVisitor.mRetargetedTouchTargets.AppendElement(touchTarget);
> > +        touch->mTarget = touchTarget;
> > +      }
> > +      MOZ_ASSERT(aVisitor.mRetargetedTouchTargets.Length() ==
> > +                 touches.Length());
> 
> nit: Shouldn't need more 2 spaces?
Hmm, where? before touches? I don't think so 


> ::: dom/events/EventDispatcher.h
> @@ +276,5 @@
> > +   * If mEvent is a WidgetTouchEvent and its mTouches needs retargeting,
> > +   * set the targets to this array. The array should contain one entry per
> > +   * each object in WidgetTouchEvent::mTouches.
> > +   */
> > +  nsTArray<RefPtr<dom::EventTarget>> mRetargetedTouchTargets;
> 
> Looks like that this class is instantiated only in the stack. So, if you use
> AutoTArray<RefPtr<dom::EventTarget>, 10>, it won't allocate something in the
> heap. But up to you.
It is not allocated on stack. EventTargetChainItems are heap allocated


> >  Touch::SetTarget(EventTarget* aTarget)
> >  {
> > +  if (!mOriginalTarget) {
> > +    mOriginalTarget = aTarget;
> 
> Well, I don't like to make method stateful, though. I'm not sure this works
> as expected for each caller of ESM. (E.g., if caller needs to override
> target simply, they might want to update mOriginalTarget too.)
I was thinking first to call it SetOriginalTarget and that would set mTarget too.
But basically something needs to set the target (and original target)

> > +TouchEvent::CopyTouchesToWidgetEvent(TouchList* aList, bool aCheckDuplicates)
> 
> Hmm, this method name starts with "Copy", but actually not copying the
> instances, just sets the touches to WidgetTouchEvent... 
Isn't that what copy means :)

> But
> TouchEvent::CopyTouches() uses "Copy". So, up to you whether this should be
> named as Copy or not.
Perhaps Assign would be easier to understand than Copy


> 
> Although, it is not scope of this bug, if the message is eTouchEnd or
> eTouchCancel, it does not make sense to run this for-loop. It should be
> checked before running this loop.
yeah, I noticed that too, but didn't want to fix in this bug



> ::: dom/events/test/window_bug1429572.html
> @@ +23,5 @@
> > +        }
> > +        return touchArray;
> > +      }
> > +
> > +      function dispatchTouches(targets, xOffsets) {
> 
> synthesizeTouches is better for guessing what this does since
> dispatchTouches sounds like dispatching untrusted events.
fine


> @@ +72,5 @@
> > +        var nonShadow = document.getElementById("nonshadow");
> > +        var event;
> > +        nonShadow.ontouchstart = function(e) {
> > +          event = e;
> > +          opener.is(e.targetTouches.length, 1, "Should have only one entry in targetTouches.");
> 
> Hmm, why don't you create is() in this file to refer opener's is()?
doesn't really matter. this way it is at least clear all the time that this isn't the real test file.


> @@ +184,5 @@
> > +        var s1 = document.getElementById("span1");
> > +
> > +        var hostHandled = false;
> > +        host.ontouchstart = function(e) {
> > +          if (e.target == host) {
> 
> Does host receive other touchstart event(s)? If yes, how about to check the
> other cases too?
it does receive


> Why don't you test if e.targetTouches[2].target is s1 (or something another
> value)?
because I just test the particularly interesting values. No other reason

> Interesting. Shouldn't we check the case when there are two or more shadow
> DOM trees and user touches both shadow DOM trees?
I could add such test yes.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 4 • 6 years ago

(In reply to Olli Pettay [:smaug] from comment #3)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> 
> > If we make TouchArray ranged-for-loop aware, we write this kind of loop
> > smarter...
> I truly hate ranged-for ;) It has lead to so many security critical bugs.

Is it caused by ranged-for of raw pointers and using auto&? If so, I think that it should be banned to use auto at ranged-for condition in coding style rules. Or the range length is changed during the ranged-for loop? Or something other?

> > > +        nsCOMPtr<nsINode> targetAsNode = do_QueryInterface(target);
> > > +        if (targetAsNode) {
> > > +          EventTarget* retargeted = nsContentUtils::Retarget(targetAsNode, this);
> > > +          if (retargeted) {
> > > +            touchTarget = retargeted;
> > 
> > I'm not sure if this works as you expected when this and original target are
> > both in different shadow DOM tree. If it works, I'm fine.
> yes it does. Retarget() is modeled per spec.

Okay, but I'd like you to adda a case into automated tests for testing two shadow DOM is touched once.

> > @@ +1120,5 @@
> > > +        aVisitor.mRetargetedTouchTargets.AppendElement(touchTarget);
> > > +        touch->mTarget = touchTarget;
> > > +      }
> > > +      MOZ_ASSERT(aVisitor.mRetargetedTouchTargets.Length() ==
> > > +                 touches.Length());
> > 
> > nit: Shouldn't need more 2 spaces?
> Hmm, where? before touches? I don't think so 

I think it's the last line. For example, looks like that following case becomes odd:

MOZ_ASSERT(foo &&
           bar ==
           baz);

Anyway, trivial, though.

> > ::: dom/events/EventDispatcher.h
> > @@ +276,5 @@
> > > +   * If mEvent is a WidgetTouchEvent and its mTouches needs retargeting,
> > > +   * set the targets to this array. The array should contain one entry per
> > > +   * each object in WidgetTouchEvent::mTouches.
> > > +   */
> > > +  nsTArray<RefPtr<dom::EventTarget>> mRetargetedTouchTargets;
> > 
> > Looks like that this class is instantiated only in the stack. So, if you use
> > AutoTArray<RefPtr<dom::EventTarget>, 10>, it won't allocate something in the
> > heap. But up to you.
> It is not allocated on stack. EventTargetChainItems are heap allocated

Ah, EventTargetChainItem, okay.

> > > +TouchEvent::CopyTouchesToWidgetEvent(TouchList* aList, bool aCheckDuplicates)
> > 
> > Hmm, this method name starts with "Copy", but actually not copying the
> > instances, just sets the touches to WidgetTouchEvent... 
> Isn't that what copy means :)

Hmm, I feel "copy" duplicates something or anyway, there will be another instance.

> > @@ +72,5 @@
> > > +        var nonShadow = document.getElementById("nonshadow");
> > > +        var event;
> > > +        nonShadow.ontouchstart = function(e) {
> > > +          event = e;
> > > +          opener.is(e.targetTouches.length, 1, "Should have only one entry in targetTouches.");
> > 
> > Hmm, why don't you create is() in this file to refer opener's is()?
> doesn't really matter. this way it is at least clear all the time that this
> isn't the real test file.

okay.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 5 • 6 years ago

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4)
> (In reply to Olli Pettay [:smaug] from comment #3)
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> > 
> > > If we make TouchArray ranged-for-loop aware, we write this kind of loop
> > > smarter...
> > I truly hate ranged-for ;) It has lead to so many security critical bugs.
> 
> Is it caused by ranged-for of raw pointers and using auto&? If so, I think
> that it should be banned to use auto at ranged-for condition in coding style
> rules. Or the range length is changed during the ranged-for loop?
I think both. Certainly the latter. We still explicitly crash if the size of the array changes during ranged-for - these days the crash just is safe and not security critical.

(auto itself is error prone and has lead to security critical bugs and memory leaks)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 6 • 6 years ago

Comment on attachment 8979128 [details] [diff] [review]
shadow_touch_target.diff

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

Perhaps, I should check new patch but must be fine.

::: dom/events/EventDispatcher.cpp
@@ +245,5 @@
> +          for (uint32_t i = 0; i < mInitialTargetTouches->Length(); ++i) {
> +            targetTouches->Append(mInitialTargetTouches->ElementAt(i));
> +          }
> +        } else {
> +          targetTouches->Clear();

Why don't you call targetTouches->Clear() before the if block and remove this else block?

@@ +596,5 @@
> +        for (uint32_t i = 0; i < touches.Length(); ++i) {
> +          touches[i]->mTarget = touches[i]->mOriginalTarget;
> +        }
> +      }
> +    }

Well, it is not good thing to have similar loop only for TouchEvent. I we could move those loops into other place, it'd be better though, but I don't have smart idea.

@@ +666,5 @@
>      // Setting back the original target of the event.
>      aVisitor.mEvent->mTarget = aVisitor.mEvent->mOriginalTarget;
>      aVisitor.mEvent->mRelatedTarget = aVisitor.mEvent->mOriginalRelatedTarget;
> +    if (firstTouchTargets) {
> +      WidgetTouchEvent* touchEvent = aVisitor.mEvent->AsTouchEvent();

AsTouchEvent() is virtual method different from the naming rules of dom (i.e., same as GetAsFoo()). So, if you worry the runtime cost, you should cache the point at start of this method only once.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 7 • 6 years ago

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> Comment on attachment 8979128 [details] [diff] [review]
> shadow_touch_target.diff
> 
> Review of attachment 8979128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Perhaps, I should check new patch but must be fine.
yeah, I'll upload a new one, with some more tests too

> 
> ::: dom/events/EventDispatcher.cpp
> @@ +245,5 @@
> > +          for (uint32_t i = 0; i < mInitialTargetTouches->Length(); ++i) {
> > +            targetTouches->Append(mInitialTargetTouches->ElementAt(i));
> > +          }
> > +        } else {
> > +          targetTouches->Clear();
> 
> Why don't you call targetTouches->Clear() before the if block and remove
> this else block?
Ah, indeed, that would be simpler.

> @@ +596,5 @@
> > +        for (uint32_t i = 0; i < touches.Length(); ++i) {
> > +          touches[i]->mTarget = touches[i]->mOriginalTarget;
> > +        }
> > +      }
> > +    }
> 
> Well, it is not good thing to have similar loop only for TouchEvent. I we
> could move those loops into other place, it'd be better though, but I don't
> have smart idea.
Well, we have similar loops for those events which have relatedTarget.


> 
> @@ +666,5 @@
> >      // Setting back the original target of the event.
> >      aVisitor.mEvent->mTarget = aVisitor.mEvent->mOriginalTarget;
> >      aVisitor.mEvent->mRelatedTarget = aVisitor.mEvent->mOriginalRelatedTarget;
> > +    if (firstTouchTargets) {
> > +      WidgetTouchEvent* touchEvent = aVisitor.mEvent->AsTouchEvent();
> 
> AsTouchEvent() is virtual method different from the naming rules of dom
> (i.e., same as GetAsFoo()). So, if you worry the runtime cost, you should
> cache the point at start of this method only once.
yup

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 8 • 6 years ago

> @@ +189,5 @@
> > +            hostHandled = true;
> > +            opener.is(e.targetTouches.length, 2, "Should have two entries in targetTouches.");
> > +            opener.is(e.targetTouches[0].target, e.target, "targetTouches should contain event.target.");
> > +            opener.is(e.targetTouches[1].target, e.target, "targetTouches should contain event.target twice.");
> > +            opener.is(e.touches.length, 3, "touches list should contain one touch object.");
> 
> Why don't you test if e.targetTouches[2].target is s1 (or something another
> value)?
Because targetTouches has just 2 entries, not 3

> @@ +209,5 @@
> > +        host.ontouchstart = function(e) {
> > +          opener.is(e.targetTouches.length, 2, "Should have all the shadow entries in targetTouches.");
> > +          opener.is(e.targetTouches[0].target, host, "targetTouches shouldn't reveal shadow DOM.");
> > +          opener.is(e.targetTouches[1].target, host, "targetTouches shouldn't reveal shadow DOM.");
> > +          opener.is(e.touches.length, 3, "touches list should contain all the touch objects.");
> 
> Why don't you test if e.targetTouches[2].target is s1 (or something another
> value)?
ditto

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 9 • 6 years ago

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/8ecf0babf1b10727761a3509422b39609232d4f4
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ecf0babf1b10727761a3509422b39609232d4f4
remote: recorded changegroup in replication log in 0.104s

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 11 • 6 years ago

(In reply to Olli Pettay [:smaug] from comment #9)
> Created attachment 8980001 [details] [diff] [review]
> shadow_touch_target_2.diff
> 
> remote: View your change here:
> remote:  
> https://hg.mozilla.org/try/rev/8ecf0babf1b10727761a3509422b39609232d4f4
> remote: 
> remote: Follow the progress of your build on Treeherder:
> remote:  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8ecf0babf1b10727761a3509422b39609232d4f4
> remote: recorded changegroup in replication log in 0.104s

It causes a lot of leaks.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 6 years ago

Lovely.
Hmm, I think that is actually existing leak, which the patch makes a lot worse.
Looking.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 13 • 6 years ago

ok, so as far as I see, there is at least some existing leak, but apparently I also managed to
push to try from a broken changeset.
Still investigating.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 14 • 6 years ago

Ok, so I had to change EventTargetChainItem::Create to not use placement new, since that first called ctor once, and then used the memory area to call ctor second time. That confused ctor counting in nsTArray.
I also added ctor/dtor counter to EventTargetChainItem.
And mRetargetedTouchTargets is now Maybe<nsTArray> since usually that array isn't used.

Since Touch::SetTarget set also original target, I changed its name to
SetTouchTarget so that it doesn't map exactly to GetTarget() method.

remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/f516cad72531ebcab1f64017bf53efa205f58325
remote:   https://hg.mozilla.org/try/rev/44be5d9e03a4946cf8fea69e547e8bf5b221a8ca
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=44be5d9e03a4946cf8fea69e547e8bf5b221a8ca
remote: recorded changegroup in replication log in 0.106s


There were some other unrelated leaks at the time when I pushed to try earlier.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 15 • 6 years ago

Comment on attachment 8980372 [details] [diff] [review]
shadow_touch_target_4.diff

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

::: dom/events/EventDispatcher.cpp
@@ +527,5 @@
> +    for (uint32_t i = 0; i < touches.Length(); ++i) {
> +      firstTouchTargets->AppendElement(touches[i]->mTarget);
> +    }
> +  }
> +  WidgetTouchEvent* touchEvent = aVisitor.mEvent->AsTouchEvent();

nit: above block calls aVisitor.mEvent->AsTouchEvent(). How about:

WidgetTouchEvent* touchEvent = nullptr;
if (aVisitor.mEvent->mClass == eTouchEventClass) {
  touchEvent = aVissitor.mEvent->AsTouchEvent();
  if (!aVisitor.mEvent->mFlags.mInSystemGroup) {
    ...
  }
}

Then, you don't add new virtual call for non-touch events.

::: dom/events/Touch.h
@@ +57,5 @@
>  
>    void InitializePoints(nsPresContext* aPresContext, WidgetEvent* aEvent);
>  
> +  // Note, this sets both mOriginalTarget and mTarget.
> +  void SetTouchTarget(EventTarget* aTarget);

Ah, looks like this is redundant name, *however*, I like this better since this is clearer even it the object's variant name is unclear.

::: dom/events/test/window_bug1429572.html
@@ +100,5 @@
> +        nonShadow.ontouchstart = function(e) {
> +          opener.is(e.targetTouches.length, 3, "Should have all the entries in targetTouches.");
> +          opener.is(e.targetTouches[0].target, s1, "targetTouches should contain s1 element.");
> +          opener.is(e.touches.length, 3, "touches list should contain all the touch objects.");
> +          opener.is(e.changedTouches.length, 3, "changedTouches list should contain all the touch objects.");

Don't you need to check if this event listener is actually called?

@@ +239,5 @@
> +          opener.is(e.targetTouches.length, 2, "Should have all the shadow entries in targetTouches.");
> +          opener.is(e.targetTouches[0].target, host, "targetTouches shouldn't reveal shadow DOM.");
> +          opener.is(e.targetTouches[1].target, host, "targetTouches shouldn't reveal shadow DOM.");
> +          opener.is(e.touches.length, 3, "touches list should contain all the touch objects.");
> +          opener.is(e.changedTouches.length, 3, "changedTouches list should contain all the touch objects.");

Don't you need to check if this event listener is actually called?

@@ +247,5 @@
> +          opener.is(e.targetTouches[0].target, shadowS1, "targetTouches should contain two shadow elements.");
> +          opener.is(e.targetTouches[1].target, shadowS2, "targetTouches should contain two shadow elements.");
> +          opener.is(e.targetTouches[2].target, s1, "targetTouches should contain a slight element.");
> +          opener.is(e.touches.length, 3, "touches list should contain all the touch objects.");
> +          opener.is(e.changedTouches.length, 3, "changedTouches list should contain all the touch objects.");

Don't you need to check if this event listener is actually called?

@@ +275,5 @@
> +          opener.is(e.touches.length, 3, "touches list should contain all the touch objects.");
> +          opener.is(e.touches[0].target, host, "Should have retargeted the first Touch object.");
> +          opener.is(e.touches[1].target, host2, "Should have retargeted the second Touch object.");
> +          opener.is(e.touches[2].target, s1, "Touch object targeted to light DOM should keep its target as is.");
> +          opener.is(e.changedTouches.length, 3, "changedTouches list should contain all the touch objects.");

Don't you need to check if this event listener is actually called?

@@ +280,5 @@
> +        }
> +        shadowS1.ontouchstart = function(e) {
> +          opener.is(e.targetTouches.length, 3, "Should have all the in targetTouches.");
> +          opener.is(e.targetTouches[0].target, shadowS1, "targetTouches should contain two shadow elements.");
> +          opener.is(e.targetTouches[1].target, shadowS2, "targetTouches should contain two shadow elements.");

Interesting. In a show tree, other elements in other shardow tree is visible.

@@ +283,5 @@
> +          opener.is(e.targetTouches[0].target, shadowS1, "targetTouches should contain two shadow elements.");
> +          opener.is(e.targetTouches[1].target, shadowS2, "targetTouches should contain two shadow elements.");
> +          opener.is(e.targetTouches[2].target, s1, "targetTouches should contain a slight element.");
> +          opener.is(e.touches.length, 3, "touches list should contain all the touch objects.");
> +          opener.is(e.changedTouches.length, 3, "changedTouches list should contain all the touch objects.");

Don't you need to check if this event listener is actually called?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 16 • 6 years ago

> Ah, looks like this is redundant name, *however*, I like this better since
> this is clearer even it the object's variant name is unclear.
> 
Yeah, I just didn't want to have SetTarget and GetTarget when they do actually a bit different things.


f this event listener is actually called?
I'll add checks everywhere.

> Interesting. In a show tree, other elements in other shardow tree is visible.
that is undefined in the spec, but the reason is that we're explicitly dispatching event to shadow DOM using targets from several shadow trees. So the caller already has access to all the shadow trees.
This is something to clarify in the touch event spec.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 17 • 6 years ago

In other words, especially the case when event is dispatched by JS, the behavior may change. But will need to file new bugs for that. And dispatching touch events from JS is rare.

	Pulsebot
	Comment 19 • 6 years ago

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b11b87405993
touch.target retargeting in shadow DOM, r=masayuki

	Dorel Luca [:dluca]
	Comment 20 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b11b87405993